Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Colour definitions as enumeration classes #4405

Closed

Conversation

paulj49457
Copy link
Contributor

@paulj49457 paulj49457 commented Oct 15, 2023

This PR aims to improve the type safety using enumeration classes rather #defines within the GC Color definitions, this PR makes no change to GC's Colors or appearance. The following describes the changes and a few defects detected & corrected by this PR:

Note: The main changes are within Colors.[h& cpp] and Pages.cpp, all the other file changes are related #defines changing to enumerated class definitions.

⦁ GCColor has been made a singleton class, this enables the duplicate code within ColorsPage::applyThemeIndex(int index) & GCColor::applyTheme(int index) functions to be extracted a placed in a single function detemineThemeQColor() within GCColor, and this detected the following issue:

The two "duplicate" functionalities handled the Hover case differently, this PR implements the code below as it more closely matches the description, but I'm not sure which is the right answer:

case GCol::HOVER:
// stealthy themes use overview card background for hover color since they are close
// all other themes get a boring default
//qColor = theme.stealth ? colorTable[gColor].qColor : (theme.dark ? QColor(50, 50, 50) : QColor(200, 200, 200));
qColor = theme.stealth ? theme.colors[GThmCol::OVERVIEWTILEBACKGROUND1] : (theme.dark ? QColor(50, 50, 50) : QColor(200, 200, 200));
break;
`

⦁ Replaces the #defines for Colors with an enumeration class (GCol) to improve type safety, this change has led to numerous trivial changes in many files, but also detected the following issues fixed by this PR:

OverviewItems.h

    `BPointF() : x(0), y(0), z(0), xoff(0), yoff(0), fill(GColor(Qt::gray)), item(NULL) {}`

A gc color should be passed to the GColor macro not a qt color, so this is now:

    `BPointF() : x(0), y(0), z(0), xoff(0), yoff(0), fill(GColor(GCol::RIDEPLOTXAXIS)), item(NULL) {}`

as Qt::gray has the value of 5, so its replaced with RIDEPLOTXAXIS which has the same value.

ExtendedCriticalPower.cpp

    `QPen e2pen(GColor(Qt::lightGray));`

A gc color should be passed to the GColor macro not a qt color, so this is now:

    `QPen e2pen(GColor(GCol::RIDEPLOTYAXIS));`

as Qt::lightGray has the value of 6, so its replaced with RIDEPLOTYAXIS which has the same value.

⦁ Replaces the theme number indexes with an enumeration class (GThmCol) to improve type safety and makes it easy to determine whether the wrong index is being used.

`case GCol::SPEED:`
`    qColor = theme.colors[6];`
`break;`

becomes:

`case GCol::SPEED:`
`    qColor = theme.colors[GThmCol::SPEED];`
`    break;`

⦁ Error found in GColor function readConfig, the name field is a text desciption of the gc color, but it is being compared against the setting value, therefore these defaults will never be applied:

        `// set sensible defaults for any not set (as new colors are added)`
        `if (ColorList[i].name == "CTOOLBAR") {`
        `    QPalette def;`
        `    ColorList[i].color = def.color(QPalette::Window);`
        `}`
        `if (ColorList[i].name == "CCHARTBAR") {`
        `    ColorList[i].color = ColorList[CTOOLBAR].color;`
        `}`
        `if (ColorList[i].name == "CCALCURRENT") {`
        `    QPalette def;`
        `    ColorList[i].color = def.color(QPalette::Highlight);`

This has been corrected to:

        // set sensible defaults for any not set (as new colors are added)
        if (colorElemt.first == GCol::TOOLBAR) {
            QPalette def;
            colorElemt.second.qColor = def.color(QPalette::Window);
        }
        if (colorElemt.first == GCol::CHARTBAR) {
            colorElemt.second.qColor = colorTable[GCol::TOOLBAR].qColor;
        }
        if (colorElemt.first == GCol::CALCURRENT) {
            QPalette def;
            colorElemt.second.qColor = def.color(QPalette::Highlight);

⦁ The three Lists: ColorList, LightDefaultColorList & DarkDefaultColorList have been combined into a single colorTable definition within GCColor using a std::map<GCol, Colors>, where all fields in the Colors class except the qColor entry are constants. This removes the previous list duplication, and simplifies checking the differences between light & dark for each defined gc color.

          class Colors {
          public:
              static unsigned long fingerprint(const std::map<GCol, Colors>& colorTable);

              // all public
              const QString group;
              const QString name;
              const QString setting;
              const QColor qDarkColor;
              const QColor qLightColor;
              QColor  qColor;
          };

⦁ The RGB conversion macros have been converted to functions to enforce type safety:

            `QColor gColorToRGBColor(const GCol& gColor);`
            `bool isRGBColor(const QColor& qColor);`
            `QColor rgbColorToQColor(const QColor& qColor);`

⦁ The "standard" Colors have been moved within GCColor.
⦁ Functions within GCColor class have been logically grouped in the header file.
⦁ Variable names within GCColors.[h&cpp] have been consistently named either gColor or qColor to clarify their type.
⦁ The unused "struct SizeSettings" in GColor.h has been removed.
⦁ The GCColor::isFlat() function only ever returned true, so has been removed
⦁ All the swatches within a theme are now displayed in the Colors Page window rather the first five, allowing the differences between similar themes to be visualised.

@amtriathlon
Copy link
Member

I am closing this basically for the same reason as #4241

@paulj49457 paulj49457 deleted the Color_Enum_Classes branch May 24, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants