ColorPaletteEditor: use theme's list-add and list-remove icons #2630
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Holzhaus
reviewed
Apr 5, 2020
|
This is the way it looks at my machine now, somehow chunky :-/. Compared to @Be-ing s screenshot there seems to be a scaling issue. If I change to list-add-symbolic and list-remove-symbolic it looks like that: So in my case I prefer the text labels. The rest of the OS, (GTK) uses symbolic icons for list-add and list-remove How does it look with symbolic and colorful icons on our other targets? |
|
list-add-symbolic/list-remove-symbolic do not look good in my Qt theme: |
|
The color palette editor causes a segfault for me: |
|
That segfault is only with this branch, not master? |
Yes, master works fine. |
Be-ing
commented
Apr 5, 2020
| m_pRemoveColorButton = new QPushButton("-", this); | ||
| QIcon removeIcon = QIcon::fromTheme("list-remove", QIcon()); | ||
| if (!removeIcon.isNull()) { | ||
| m_pRemoveColorButton = new QPushButton(removeIcon, "", this); |
There was a problem hiding this comment.
@Holzhaus can you use a debugger to check if this or line 38 is executing?
There was a problem hiding this comment.
That's bizarre. Line 38 is identical to the line in master that was removed.
There was a problem hiding this comment.
yeah, I also have no idea what causes that but it happens every time.
There was a problem hiding this comment.
Maybe we can just keep the old code and if the remove icon is not null, then set icon and label on the button afterwards?
There was a problem hiding this comment.
I'll check if that segfaults as well.
|
I don't like the colorful view. But I don't really mind for this rarearly used piece of gui. If you think this is an improvement go ahead. |
|
I tried to test on Win10 by running the Appveyor artifacts. Fail
|
Same for me, even on Linux. Very strange. I even commented out all of @Be-ing's code and it still happens |
|
I have no idea why @Be-ing's implementation doesn't work, but when I revert the last commit and add this, it works for me: diff --git a/src/preferences/colorpaletteeditor.cpp b/src/preferences/colorpaletteeditor.cpp
index 0a66cdafce..ca2004c972 100644
--- a/src/preferences/colorpaletteeditor.cpp
+++ b/src/preferences/colorpaletteeditor.cpp
@@ -32,6 +32,9 @@ ColorPaletteEditor::ColorPaletteEditor(QWidget* parent, bool showHotcueNumbers)
pColorButtonLayout->addWidget(pExpander);
m_pRemoveColorButton = new QPushButton(QIcon::fromTheme("list-remove"), "", this);
+ if (m_pRemoveColorButton->icon().isNull()) {
+ m_pRemoveColorButton->setText("-");
+ }
m_pRemoveColorButton->setFixedWidth(32);
m_pRemoveColorButton->setToolTip(tr("Remove Color"));
m_pRemoveColorButton->setDisabled(true);
@@ -42,6 +45,9 @@ ColorPaletteEditor::ColorPaletteEditor(QWidget* parent, bool showHotcueNumbers)
&ColorPaletteEditor::slotRemoveColor);
m_pAddColorButton = new QPushButton(QIcon::fromTheme("list-add"), "", this);
+ if (m_pAddColorButton->icon().isNull()) {
+ m_pAddColorButton->setText("+");
+ }
m_pAddColorButton->setFixedWidth(32);
m_pAddColorButton->setToolTip(tr("Add Color"));
pColorButtonLayout->addWidget(m_pAddColorButton); |
Ok. If you don't like your icons, you could also change your icon theme :-) |
|
I can't confirm this with Xenial. |
|
Thanks @Holzhaus, I just pushed a commit with those changes. |
|
This might have caused a crash on macOS: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Continuing from discussion on #2589
before:
after:
