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.

Sign up for GitHub

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

Color enhance #104

Merged
merged 12 commits into from
Oct 20, 2018
Merged

Color enhance #104

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4de8a95
ColorBox replaced by ColorButton.
gbm19 Oct 8, 2018
291e77c
Define a macro to save color values including alpha
gbm19 Oct 8, 2018
da5e955
Set basic colors in QColorDialog from internal color list
gbm19 Oct 8, 2018
a3a19f4
Remove redundant numPredefinedColors
gbm19 Oct 10, 2018
d745453
Avoid defining additional color variables
gbm19 Oct 10, 2018
1cd7434
Replace macro by inline function
gbm19 Oct 10, 2018
ea7252c
Additional basic colors
gbm19 Oct 10, 2018
7a2ea46
Avoid possible crash
gbm19 Oct 11, 2018
d3bee93
Remove references to ColorBox
gbm19 Oct 11, 2018
bd0f884
Remove use of colorIndex where possible
gbm19 Oct 12, 2018
1363d50
Save colors as hex values in .sciproj files
gbm19 Oct 12, 2018
52acd14
Save color settings as hex values
gbm19 Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions libscidavis/src/ColorButton.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,31 @@ const QColor ColorButton::colors[] = {
QColor("#80FFFF"),
QColor("#FF80FF"),
QColor(Qt::darkGray),
// additional colors from figure 6 in doi:10.1016/j.csda.2008.11.033
QColor("#023fa5"),
QColor("#4a6fe3"),
QColor("#11c638"),
QColor("#0fcfc0"),
QColor("#8e063b"),
QColor("#d33f6a"),
QColor("#ef9708"),
QColor("#f79cd4"),
QColor("#7d87b9"),
QColor("#8595e1"),
QColor("#8dd593"),
QColor("#9cded6"),
QColor("#bb7784"),
QColor("#e07b91"),
QColor("#f0b98d"),
QColor("#f6c4e1"),
QColor("#bec1d4"),
QColor("#b5bbe3"),
QColor("#c6dec7"),
QColor("#d5eae7"),
QColor("#d6bcc0"),
QColor("#e6afb9"),
QColor("#ead3c6"),
QColor("#f3e1eb"),
};

const unsigned int ColorButton::colors_count=sizeof(colors)/sizeof(colors[0]);
Expand All @@ -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.
Copy link
Owner

Choose a reason for hiding this comment

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

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.

for (int i=0; i<8; i++)
{
for (int j=0; j<3; j++)
for (int j=0; j<6; j++)
{
QColorDialog::setStandardColor(6*i+j, colors[i+8*j].rgb());
QColorDialog::setStandardColor(j+6*i, colors[i+8*j].rgb());
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

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, ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@highperformancecoder highperformancecoder Oct 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

const int btn_size = 28;
Expand Down