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

Allow prompt icon to be colorized #330

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

jarolrod
Copy link
Member

Opening the console on macOS, while in dark mode, the console prompt icon will not be colorized white like other icons. This applies the platformStyle to the icon so that It can be colorized white.

While here, refactor the promptIcon widget from a QPushButton to QLabel; which is more appropriate, per Qt Docs:

QLabel is used for displaying text or an image. No user interaction functionality is provided.

Master PR
Screen Shot 2021-05-14 at 11 46 33 PM Screen Shot 2021-05-14 at 11 45 41 PM

@hebasto hebasto added the UI All about "look and feel" label May 15, 2021
@hebasto
Copy link
Member

hebasto commented May 15, 2021

Just stating a fact: this PR and #270 (comment) are competing.

@jarolrod
Copy link
Member Author

@hebasto
This and #270 are independent.

Using a new prompt icon is independent from making sure it's colorized white in dark mode.

@hebasto
Copy link
Member

hebasto commented May 15, 2021

@hebasto
This and #270 are independent.

Using a new prompt icon is independent from making sure it's colorized white in dark mode.

Not a new icon, but text:

If you want to have a symbol to indicate user input vs. console output, why not keep it text-based, like in the example below? It uses the ">" character to indicate user input, to match the icon next to the input field.

@jarolrod
Copy link
Member Author

@hebasto

That's in regards to how we should change the console input vs output icons. Not about the prompt icon.

@hebasto
Copy link
Member

hebasto commented May 15, 2021

@hebasto

That's in regards to how we should change the console input vs output icons. Not about the prompt icon.

You're right. Sorry for noise.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK 2162b3e.

src/qt/forms/debugwindow.ui Outdated Show resolved Hide resolved
@jarolrod
Copy link
Member Author

updated from 2162b3e -> f5e5511 (pr330.01 -> pr330.02, diff)

Changes:

@hebasto in regard to your comment
There is no icon property for a QLabel. To address this, I instead also set the pixmap in the *.ui file. Here, I also set the scaled property so that the pixmap will actually show in a designer tool. Because of this, I change the dimension from 16 x 24 to 14 x 14. Here's why:

If I keep the original dimensions, the pixmap will look warped in the designer tool:
Screen Shot 2021-05-21 at 4 33 49 PM

It is ok to set the dimensions of the QLabel to the exact dimensions we set for the pixmap itself. Doing so means the pixmap will look normal in the designer tool:

ui->promptIcon->setPixmap(platformStyle->SingleColorIcon(":/icons/prompticon").pixmap(14, 14));

Screen Shot 2021-05-21 at 4 33 26 PM

There is no change in the size of this pixmap(previously icon) comparing master, pr330.01, or pr330.02:

Master PR330.01 PR330.02 (this)
original old-pr new-pr

@jarolrod jarolrod force-pushed the prompt-icon-colorized branch from f5e5511 to be85c7f Compare May 21, 2021 21:49
@jarolrod
Copy link
Member Author

jarolrod commented May 21, 2021

updated from f5e5511 -> be85c7f (pr330.02 -> pr330.03, diff)

Changes:

  • Address whitespace linter failure
    • The linter does not like a chunk that only adds whitespace, this chunk came to be after rebasing over master. Previously, the same chunk where I added the whitespace added our promptIcon. To fix this, I now set promptIcon below clearButton instead of below fontSmallerButton

@hebasto
Copy link
Member

hebasto commented May 28, 2021

Opening the console on macOS, while in dark mode, the console prompt icon will not be colorized white like other icons. This applies the platformStyle to the icon so that It can be colorized white.

While here, refactor the promptIcon widget from a QPushButton to QLabel; which is more appropriate, per Qt Docs:

QLabel is used for displaying text or an image. No user interaction functionality is provided.

Master PR
Screen Shot 2021-05-14 at 11 46 33 PM Screen Shot 2021-05-14 at 11 45 41 PM

Does this line (from #275)

ui->promptIcon->setIcon(platformStyle->SingleColorIcon(QStringLiteral(":/icons/prompticon")));
change anything?

@jarolrod
Copy link
Member Author

@hebasto
It seems that we've uncovered the first "bug" since the merge of 275. If you start off with dark mode on the current master, which includes 275, the promptIcon is colored black. It will become colorized if you switch to light mode then back to dark mode. Below are screenshots showing this behavior

Start with Dark Mode on Master Switch to Light then Back to Dark
Screen Shot 2021-05-28 at 3 11 34 PM Screen Shot 2021-05-28 at 3 10 01 PM

Another thing to notice is that because this is currently a disabled QPushButton, the promptIcon is colored gray instead of white.

Applying this PR on top of master (which requires changes to do), fixes this "bug", makes the promptIcon class appropriate, and lets it be colorized white.

Screen Shot 2021-05-28 at 3 47 28 PM

There's currently a silent merge conflict with #275 and will be pushing changes to fix this.

@jarolrod jarolrod force-pushed the prompt-icon-colorized branch from be85c7f to 82d430d Compare June 2, 2021 20:20
@jarolrod
Copy link
Member Author

jarolrod commented Jun 2, 2021

updated from be85c7f -> 82d430d (pr330.03 -> pr330.04)

Changes:

src/qt/forms/debugwindow.ui Outdated Show resolved Hide resolved
@hebasto hebasto removed the macOS label Jun 2, 2021
@hebasto
Copy link
Member

hebasto commented Jun 5, 2021

I think the current approach is simpler

What about a minimal change:

--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -525,6 +525,8 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
     //: Secondary shortcut to decrease the RPC console font size.
     GUIUtil::AddButtonShortcut(ui->fontSmallerButton, tr("Ctrl+_"));
 
+    ui->promptIcon->setIcon(platformStyle->SingleColorIcon(QStringLiteral(":/icons/prompticon")));
+
     // Install event filter for up and down arrow
     ui->lineEdit->installEventFilter(this);
     ui->lineEdit->setMaxLength(16 * 1024 * 1024);

?

@jarolrod
Copy link
Member Author

jarolrod commented Jun 7, 2021

updated from 82d430d -> 8095c81 (pr330.04 -> pr330.05)

Changed:

@luke-jr
Copy link
Member

luke-jr commented Jun 25, 2021

Prefer to fix with b942216 or such.

It'd be nice to magically make theming work everywhere, but this approach doesn't seem worth it.

@hebasto hebasto added this to the 22.0 milestone Jun 29, 2021
@jarolrod jarolrod force-pushed the prompt-icon-colorized branch from 8095c81 to 2f23ad2 Compare June 30, 2021 20:15
@jarolrod
Copy link
Member Author

Updated from 8095c81 -> 2f23ad2 (pr330.05 -> pr330.06)

Changes:

  • Use minimal diff solution to accomplish the goal of this PR, as mentioned here and here

Tested on macOS 11.3, macOS 10.15.7, and macOS 10.14.6. Below are screenshots showing the prompt icon properly re-colorized on the mentioned platforms:

macOS 11.3

Start: Dark Mode Switch: Light Mode
Screen Shot 2021-06-30 at 1 33 09 PM Screen Shot 2021-06-30 at 1 33 46 PM
Start: Light Mode Switch: Dark Mode
Screen Shot 2021-06-30 at 1 33 46 PM Screen Shot 2021-06-30 at 1 33 09 PM

macOS 10.15.7

Start: Dark Mode Switch: Light Mode
Screen Shot 2021-06-30 at 1 33 09 PM Screen Shot 2021-06-30 at 2 50 06 PM
Start: Light Mode Switch: Dark Mode
Screen Shot 2021-06-30 at 2 51 25 PM Screen Shot 2021-06-30 at 2 51 41 PM

macOS 10.14.6

Start: Dark Mode Switch: Light Mode
Screen Shot 2021-06-30 at 2 24 58 PM Screen Shot 2021-06-30 at 2 25 22 PM
Start: Light Mode Switch: Dark Mode
Screen Shot 2021-06-30 at 2 26 35 PM Screen Shot 2021-06-30 at 2 27 06 PM

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 2f23ad2

@hebasto hebasto merged commit 333ec8b into bitcoin-core:master Jul 1, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 1, 2021
2f23ad2 qt: allow prompt icon to be colorized (Jarol Rodriguez)

Pull request description:

  Opening the console on macOS, while in dark mode, the console prompt icon will not be colorized white like other icons. This applies the `platformStyle` to the icon so that It can be colorized white.

  While here, refactor the `promptIcon` widget from a `QPushButton` to `QLabel`; which is more appropriate, per [Qt Docs](https://doc.qt.io/qt-5/qlabel.html#details):
  > QLabel is used for displaying text or an image. No user interaction functionality is provided.

  | Master        | PR               |
  | ----------- | ----------- |
  | ![Screen Shot 2021-05-14 at 11 46 33 PM](https://user-images.githubusercontent.com/23396902/118347462-8f689780-b511-11eb-8335-329f7d2a9992.png) | ![Screen Shot 2021-05-14 at 11 45 41 PM](https://user-images.githubusercontent.com/23396902/118347463-92638800-b511-11eb-9044-073f51ef27ff.png) |

ACKs for top commit:
  hebasto:
    ACK 2f23ad2

Tree-SHA512: 21f8b1610e4820c9064bbd08608b5467e5b9499e2a3b149ff223e37b60e7d560497255c733eafa5434628a84b9f7b7c91d8b0f34b02be2f9ceb3ab21a4d555a8
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
UI All about "look and feel"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants