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

Copy paste bug in BaseStyle.cpp #4765

Closed
qarmin opened this issue May 20, 2020 · 7 comments · Fixed by #4752
Closed

Copy paste bug in BaseStyle.cpp #4765

qarmin opened this issue May 20, 2020 · 7 comments · Fixed by #4752
Assignees

Comments

@qarmin
Copy link

qarmin commented May 20, 2020

Probably second occurrence of colors[S_text] should be removed

colors[S_text] = pal.color(QPalette::Text);
colors[S_text] = pal.color(QPalette::WindowText);
colors[S_windowText] = pal.color(QPalette::WindowText);

@qarmin qarmin added the bug label May 20, 2020
@droidmonkey
Copy link
Member

@randrew
Copy link

randrew commented May 30, 2020

I've fixed this upstream at randrew/phantomstyle@22c58ee

@randrew
Copy link

randrew commented May 30, 2020

You should be able to apply this fix without any visual changes occurring in your UI.

I think these are the palette values you're using:

palette.setColor(QPalette::Active, QPalette::WindowText, QStringLiteral("#1D1D20"));
palette.setColor(QPalette::Inactive, QPalette::WindowText, QStringLiteral("#252528"));
palette.setColor(QPalette::Disabled, QPalette::WindowText, QStringLiteral("#8C8C92"));
palette.setColor(QPalette::Active, QPalette::Text, QStringLiteral("#1D1D20"));
palette.setColor(QPalette::Inactive, QPalette::Text, QStringLiteral("#252528"));
palette.setColor(QPalette::Disabled, QPalette::Text, QStringLiteral("#8C8C92"));

palette.setColor(QPalette::Active, QPalette::WindowText, QStringLiteral("#CACBCE"));
palette.setColor(QPalette::Inactive, QPalette::WindowText, QStringLiteral("#C8C8C6"));
palette.setColor(QPalette::Disabled, QPalette::WindowText, QStringLiteral("#707070"));
palette.setColor(QPalette::Active, QPalette::Text, QStringLiteral("#CACBCE"));
palette.setColor(QPalette::Inactive, QPalette::Text, QStringLiteral("#C8C8C6"));
palette.setColor(QPalette::Disabled, QPalette::Text, QStringLiteral("#707070"));

For these palettes, and my own test palettes, and all of the palettes I've seen in the wild, QPalette::Text and QPalette::WindowText ColorRoles use the same color value.

Thanks for finding this bug.


Unrelated aside: I saw that you're using QStringLiteral to populate the color values. QStringLiteral tends to generate a fair bit of binary bloat, at least with the compilers I've used. You might instead want to consider using integer literals, if that sort of thing is a priority in your project.

@phoerious
Copy link
Member

Text and WindowText are the same, yes.

It's true that QStringLiteral is not optimal for strings which are not necessarily unique, perhaps we could be using QLatin1String or so instead. The reason why these are not numerical colour values is because hex codes are easier to copy-paste in and from graphics applications, where we can tweak and optimise them.

@droidmonkey
Copy link
Member

BTW, @randrew, we just tried Qt 5.15 and it completely fails to apply the palette. I believe this to be a Qt bug, rather major one.

@randrew
Copy link

randrew commented May 30, 2020

@phoerious you can use hex literals, which is what those strings are emulating -- just replace QStringLiteral("#1D1D20") with 0x1D1D20.

@droidmonkey I don't have access to a Qt 5.15 installation right now. Are you saying you tried the KeePassXC code and the palettes didn't apply, or you tried the PhantomStyle code (probably the 'funhouse' project) and it didn't work? (Or both?)

@phoerious
Copy link
Member

phoerious commented May 30, 2020

you can use hex literals, which is what those strings are emulating -- just replace QStringLiteral("#1D1D20") with 0x1D1D20.

Good point. We should do that.

(Or both?)

That.

droidmonkey added a commit that referenced this issue May 30, 2020
droidmonkey added a commit that referenced this issue May 30, 2020
droidmonkey added a commit that referenced this issue May 30, 2020
droidmonkey added a commit that referenced this issue May 31, 2020
droidmonkey added a commit that referenced this issue Jun 2, 2020
droidmonkey added a commit that referenced this issue Jun 2, 2020
droidmonkey added a commit that referenced this issue Jun 2, 2020
droidmonkey added a commit that referenced this issue Jun 3, 2020
droidmonkey added a commit that referenced this issue Jun 3, 2020
droidmonkey added a commit that referenced this issue Jun 4, 2020
droidmonkey added a commit that referenced this issue Jun 4, 2020
lerignoux pushed a commit to lerignoux/keepassxc that referenced this issue Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants