-
Notifications
You must be signed in to change notification settings - Fork 271
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
Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog #762
base: master
Are you sure you want to change the base?
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
7398ef4
to
30c235b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may help to show screenshots before and after
src/qt/utilitydialog.cpp
Outdated
void HelpMessageDialog::setAboutWindowTitle(const NetworkStyle *networkStyle) | ||
{ | ||
QString aboutTitle = tr("About %1").arg(PACKAGE_NAME); | ||
if ((networkStyle) && (Params().GetChainType() != ChainType::MAIN)) aboutTitle.append(" " + networkStyle->getTitleAddText()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this be nullptr? Seems easier to use a reference that is never null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to touch src/qt/bitcoin.cpp
:
Line 568 in db19a7e
HelpMessageDialog help(nullptr, gArgs.IsArgSet("-version")); |
And I also needed to move where the networkStyle
is initialised/ set before the HelpMessageDialog
constructor:
Line 607 in db19a7e
QScopedPointer<const NetworkStyle> networkStyle(NetworkStyle::instantiate(Params().GetChainType())); |
But I can do it if that's better.
Thanks for your review @MarcoFalke, the before screenshot is on the issue #761 but I can bring it here if that makes it clearer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tack 30c235b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 30c235b
@GBKS What do you think from a designer's point of view? |
Not sure. All info displayed on the About page is fixed details about the software itself, not about the current (test)chain. So modulating the icon or title bar seems confusing, but no strong opinion. |
Definitely more consistent this way. |
In that case, shouldn't the logo on the About page be black-toned? |
I'm less sure about black-toned icons, in that specific case, it's more common usage I guess, users are already used to the orange one. I think the most notable use case is when you have different instances open and you want to make sure you are standing on the correct one but more importantly the consistency regarding other components such as the splash screen and task-bar icons as pointed by @GBKS. |
Thank you @GBKS for your designer's opinion! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you will touch this branch, mind applying clang-format-diff.py
?
30c235b
to
577d500
Compare
Updates:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested 577d500.
On Windows, there remains some inconsistency. Running bitcoin-qt.exe -signet -about
ends with a dialog with an orange-coloured icon.
Let me take a look... |
Adding the networkStyle parameter to the HelpMessageDialog creator on utilitydialog, updating all calls where its instance is being created from bitcoingui.cpp. In the object itself, use this new parameter object to build the about window title and set the icon of the about logo widget. Also, set th correct chain type icon at the very beginning of GuiMain() where command line arguments were validated and most of the app settings were not initialised yet.
577d500
to
5556860
Compare
Updates:
|
🐙 This pull request conflicts with the target branch and needs rebase. |
It fixes #761.
About logo icon is updated now with the correct colour indicating the chain type of the QT instance that user is running (as the splash screen), plus the title of the about window also is updated with the chain type string as in the main window.
Before PR changes screenshot
After PR changes screenshot
Current splash screen screenshot
(this hasn't been touched, just to show consistency with this PR changes)Tested it also with:
bitcoin-qt
orbitcoin-qt -chain=main
),-regtest
and-testnet
.