Color enhance #104
Color enhance #104
Conversation
Add alpha channel to all colors. Coordinate changes in color alpha and opacity.
| { | ||
| QColorDialog::setStandardColor(6*i+j, colors[i+8*j].rgb()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The magic numbers 8 and 3 bother me a bit. I assume that maybe the dialog is perhaps 8x3 or something? At very least, there should be a static_assert that 8*3 <= colors_count.
Also what happens to standard colours 3,4,5, 9,10,11, ...?
There was a problem hiding this comment.
QColorDialog has 48 basic colors in a 6x8 grid.
Using:
for (int i=0; i < colors_count; i++) {QColorDialog::setStandardColor(i, colors[i].rgb());};
arranges the colors in columns of 6. I prefer to arrange them in rows of 8 colors each
There was a problem hiding this comment.
Maybe a comment along the lines of rearranging the stand ard layout of standard colours might be useful. It puzzled me why you were only allocating half the colours.
The static_assert might also be useful, in case someone decides to remove some from the list, or monkey around with the magic numbers.
There was a problem hiding this comment.
I'm still keen on the static_assert:
static_assert(8*6 <= colours_count, "not enough default colors");
Better still, name the 8 and the 6, and use the names both in the loop and the assert. That way, as soon as anyone touches this in an invalid way, it will thow up a compiler error.
libscidavis/src/ColorButton.cpp
Outdated
| bool ColorButton::isValidColor(const QColor& c) | ||
| { | ||
| const QColor *ite = std::find(std::begin(colors), std::end(colors), c); | ||
| return (ite->isValid()); |
There was a problem hiding this comment.
Dereferencing the end of a sequence (as what happens if the colour is not found) has undefined behaviour, and could even cause a null dereference crash.
The return test could be as simple as ite-color < colors_count, or to be more std libraryish ite!=std::end(colors)
There was a problem hiding this comment.
Qt takes care of all that trouble.
There was a problem hiding this comment.
I don't think so. std::find returns an iterator pointing to the end of the sequence, which in this case will be a pointer to one past the end the colour array. ite->isValid() is completely undefined - the compiler could initialise the memory past the end of the array to zero, or some garbage pattern, or it may put a properly initialised default QColor, or it may remain uninitialised to some random value.
There was a problem hiding this comment.
I guess Qt appends a properly initialised QColor(Invalid) at end of array at the constructor, because that is what I get when I test the code.
There was a problem hiding this comment.
It's not Qt. It's your compiler. Someone else's compiler may do it differently. Next year, your compiler may do it differently. It is not good to rely on it.
libscidavis/src/PlotDialog.cpp
Outdated
|
|
||
| boxCanvasColor->setColor ( c ) ; | ||
|
|
||
| QColor color = c; |
There was a problem hiding this comment.
Why not rename the argument color?
There was a problem hiding this comment.
If color needs to be mutable, which is the likely reason, why not make the argument a QColor, and pass by value?
| @@ -70,11 +95,14 @@ ColorButton::ColorButton(QWidget *parent) : QWidget(parent) | |||
|
|
|||
| void ColorButton::init() | |||
| { | |||
| // QColorDialog has 48 basic colors in a 6x8 grid. Using: | |||
| // for (int i=0; i < colors_count; i++) {QColorDialog::setStandardColor(i, colors[i].rgb());}; | |||
| // arranges the colors in columns of 6. Instead arrange them in rows of 8 colors each. | |||
There was a problem hiding this comment.
Your comment is still slightly confusing - given a 6x8 grid, columns of 6 is the same thing as rows of 8.
You could simply say "transpose the 6x8 grid", which is what you're actually doing.
|
Notice that last commit modifies the format of saved settings file (SciDAVis.conf) |
|
Re SciDAVis.conf, is the software backwardly compatible with the previous .conf file? |
|
No. Code in this PR does not properly read color values in previous version .conf file. Correction: |
This PR systematizes the use of ColorButton to define all colors in graphics.
It also allows to choose an alpha value (transparency/opacity) for each color.
Color values (including alpha) are stored in .sciproj files as #AARRGGBB.
Thus, files saved with this version can not be properly read with previous SciDAVis versions that stored colors as #RRGGBB.
It should solve github issue #64, and sf #278.