Skip to content

Conversion functions for RgbColor#2518

Merged
Holzhaus merged 10 commits intomixxxdj:masterfrom
uklotzde:trackcolor-fix
Feb 28, 2020
Merged

Conversion functions for RgbColor#2518
Holzhaus merged 10 commits intomixxxdj:masterfrom
uklotzde:trackcolor-fix

Conversation

@uklotzde
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde commented Feb 26, 2020

Add reusable functions for formatting colors as strings.

The tests verify that an optional/invalid color produces an empty null string which is not the case for QColor().name().

Update:

  • Conversion functions from/to various Qt types: QRgb = RgbColor::code_t, QQColor, QString, QVariant (QRgb, QColor, QString)
  • Conversion functions for control values (double)

@uklotzde uklotzde requested a review from Holzhaus February 26, 2020 10:26
@uklotzde uklotzde added this to the 2.3.0 milestone Feb 26, 2020
Comment thread src/util/color/rgbcolor.h Outdated
@uklotzde
Copy link
Copy Markdown
Contributor Author

Anything else to do? Because these extensions will be needed for the cue color work.

Comment thread src/util/color/rgbcolor.h Outdated
@uklotzde uklotzde changed the title Fix track color library inline editing (Part 2) [WiP] Fix track color library inline editing (Part 2) Feb 27, 2020
@uklotzde uklotzde changed the title [WiP] Fix track color library inline editing (Part 2) Fix track color library inline editing (Part 2) Feb 27, 2020
Comment thread src/library/dao/trackdao.cpp Outdated
@uklotzde
Copy link
Copy Markdown
Contributor Author

@Holzhaus Switching the typedef of the internal code from quint32 to QRgb will hopefully improve compatibility with Qt code.

@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Feb 28, 2020

To avoid spreading the control value conversions all over the place I have added the required functions and tests. Not sure about the placement and naming of the corresponding header file, but this can easily be adjusted later as needed.

Copy link
Copy Markdown
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds find and tests pass. Waiting for CI builds.

Comment thread src/control/convert.h Outdated

namespace control {

typedef double value_t;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the typedef? Is this supposed to replace all occurrences of double for CO signals/slots? If not I'd remove this and and rename the functions below to doubleFromRgbColor, because this would be confusing IMHO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you don't need to use it.

I was aiming for clean code, at least within this header file. Hard-coding primitive types into the domain layer is usually not desirable. Type aliases don't provide any type safety, so this is just a zero-cost semantic abstraction on the source code level.

But if it is confusing in contrast to the large amount of legacy code I will remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not arbitrary double values, they have a special semantic in the context of control values. The standalone function name doubleFromRgbColor() is therefore slightly misleading. But if it is invoked as control::doubleFromRgbColor() the correlation might be obvious enough.

@uklotzde uklotzde changed the title Fix track color library inline editing (Part 2) Conversion functions for RgbColor Feb 28, 2020
@uklotzde
Copy link
Copy Markdown
Contributor Author

@Holzhaus Ready

Copy link
Copy Markdown
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Very handy indeed. Code looks good and tests pass. LGTM.

@Holzhaus
Copy link
Copy Markdown
Member

CI builds pass, I'll merge this.

@Holzhaus Holzhaus merged commit 0cc55fa into mixxxdj:master Feb 28, 2020
@uklotzde uklotzde deleted the trackcolor-fix branch February 28, 2020 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants