Fix window title of wallet loading window by shaavan · Pull Request #409 · bitcoin-core/gui

shaavan · 2021-08-23T18:43:10Z

Throughout the GUI, the title of the window, tells about the purpose of the window. This was not true for the title of wallet loading wallet.
This PR fixes this issue by renaming the wallet loading window title to 'Open Wallet'

Changes introduced in this PR (Runned Bitcoin-GUI on signet network)

Master	PR

maflcko · 2021-08-23T18:47:00Z

This will also break the settings in the registry, IIRC

hebasto · 2021-08-23T19:22:17Z

Considering the following

gui/src/qt/bitcoin.cpp

Line 516 in d3203a9

QApplication::setApplicationName(QAPP_APP_NAME_DEFAULT);

user could end with two installed versions of Bitcoin Core, no?

hebasto · 2021-08-23T19:32:56Z

Approach NACK.

Why not modify WalletControllerActivity::showProgressDialog?

Or use QGuiApplication::applicationDisplayName? But not sure about the latter...

jarolrod

This is quite an invasive change, why not just do:

diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp
index 3cceb5ca5..875dd0c14 100644
--- a/src/qt/walletcontroller.cpp
+++ b/src/qt/walletcontroller.cpp
@@ -206,6 +206,7 @@ void WalletControllerActivity::showProgressDialog(const QString& label_text)
     m_progress_dialog->setLabelText(label_text);
     m_progress_dialog->setRange(0, 0);
     m_progress_dialog->setCancelButton(nullptr);
+    m_progress_dialog->setWindowTitle(PACKAGE_NAME);
     m_progress_dialog->setWindowModality(Qt::ApplicationModal);
     GUIUtil::PolishProgressDialog(m_progress_dialog);
     // The setValue call forces QProgressDialog to start the internal duration estimation.

shaavan · 2021-08-24T14:42:04Z

Updated from bb7aae1 to abcc83a (pr409.01 -> pr409.02)
Addressed Comments by @hebasto and @jarolrod.

Changes made:

Resetted QAPP_APP_NAME variables to their original value
Changed title of loading window, by updating WalletControllerActivity::showProgressDialog, as suggested by @hebasto and @jarolrod
Updated PR description accordingly.

hebasto

~~ACK abcc83a~~ (see #409 (comment)), tested on Linux Mint 20.2 (Qt 5.12.8):

master	this PR

@ShaMan239
Before merging, could you update the PR title and description to describe use cases more precise:

gui/src/qt/walletcontroller.cpp

Line 254 in 774a4f5

    
           showProgressDialog(tr("Creating Wallet <b>%1</b>…").arg(m_create_wallet_dialog->walletName().toHtmlEscaped()));

gui/src/qt/walletcontroller.cpp

Line 349 in 774a4f5

    
           showProgressDialog(tr("Opening Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));

jarolrod

tACK abcc83a

tested on macOS 12 and Ubuntu 20.04, thanks for picking up the suggestion!

It should be noted that the behavior on master for macOS was a slightly different problem. For macOS on master, there was no window title at all. Now there is one. This is shown in the screenshots below.

Master	PR

hebasto · 2021-08-26T20:48:15Z

Hmm, is this approach consistent with our code base? Consider git grep -n setWindowTitle.

jarolrod · 2021-08-26T20:56:56Z

Hmm, is this approach consistent with our code base? Consider git grep -n setWindowTitle.

Why isn't it?

gui/src/qt/utilitydialog.cpp

Line 41 in b8d45a3

setWindowTitle(tr("About %1").arg(PACKAGE_NAME));

Here is is essentially the same, just appends more information to PACKAGE_NAME

gui/src/qt/bitcoingui.cpp

Lines 1375 to 1387 in b8d45a3

    
               QString window_title = PACKAGE_NAME; 
        
           #ifdef ENABLE_WALLET 
        
               if (walletFrame) { 
        
                   WalletModel* const wallet_model = walletFrame->currentWalletModel(); 
        
                   if (wallet_model && !wallet_model->getWalletName().isEmpty()) { 
        
                       window_title += " - " + wallet_model->getDisplayName(); 
        
                   } 
        
               } 
        
           #endif 
        
               if (!m_network_style->getTitleAddText().isEmpty()) { 
        
                   window_title += " - " + m_network_style->getTitleAddText(); 
        
               } 
        
               setWindowTitle(window_title);

hebasto · 2021-08-26T21:45:11Z

Hmm, is this approach consistent with our code base? Consider git grep -n setWindowTitle.

Why isn't it?

Consider other short-living auxiliary windows:

$ git grep -n setWindowTitle
src/qt/addressbookpage.cpp:84:        case SendingTab: setWindowTitle(tr("Choose the address to send coins to")); break;
src/qt/addressbookpage.cpp:85:        case ReceivingTab: setWindowTitle(tr("Choose the address to receive coins with")); break;
src/qt/addressbookpage.cpp:96:        case SendingTab: setWindowTitle(tr("Sending addresses")); break;
src/qt/addressbookpage.cpp:97:        case ReceivingTab: setWindowTitle(tr("Receiving addresses")); break;
src/qt/askpassphrasedialog.cpp:51:            setWindowTitle(tr("Encrypt wallet"));
src/qt/askpassphrasedialog.cpp:59:            setWindowTitle(tr("Unlock wallet"));
src/qt/askpassphrasedialog.cpp:62:            setWindowTitle(tr("Change passphrase"));
src/qt/bitcoingui.cpp:1387:    setWindowTitle(window_title);
src/qt/editaddressdialog.cpp:29:        setWindowTitle(tr("New sending address"));
src/qt/editaddressdialog.cpp:32:        setWindowTitle(tr("Edit receiving address"));
src/qt/editaddressdialog.cpp:36:        setWindowTitle(tr("Edit sending address"));
src/qt/psbtoperationsdialog.cpp:28:    setWindowTitle("PSBT Operations");
src/qt/receiverequestdialog.cpp:49:    setWindowTitle(tr("Request payment to %1").arg(info.label.isEmpty() ? info.address : info.label));
src/qt/sendcoinsdialog.cpp:1037:    setWindowTitle(title); // On macOS, the window title is ignored (as required by the macOS Guidelines).
src/qt/splashscreen.cpp:122:    setWindowTitle(titleText + " " + titleAddText);
src/qt/transactiondescdialog.cpp:18:    setWindowTitle(tr("Details for %1").arg(idx.data(TransactionTableModel::TxHashRole).toString()));
src/qt/utilitydialog.cpp:41:        setWindowTitle(tr("About %1").arg(PACKAGE_NAME));
src/qt/utilitydialog.cpp:60:        setWindowTitle(tr("Command-line options"));
src/qt/utilitydialog.cpp:158:    shutdownWindow->setWindowTitle(window->windowTitle());
src/qt/walletcontroller.cpp:87:    box.setWindowTitle(tr("Close wallet"));

promag

Concept ACK.

src/qt/walletcontroller.cpp

shaavan · 2021-09-07T13:16:33Z

Updated from abcc83a to 58dca47 (pr409.02 -> pr409.03)
Addressed Comments by @hebasto and @promag. Thanks, @hebasto, for pointing out the problem, and thanks, @promag, for the suggestion.

Changes made:

Updated the showProgressDialog function to accept two arguments.
Updated all the occurrences of showProgressDialog with the new title as “Open Wallet.”

src/qt/walletcontroller.cpp

shaavan · 2021-09-07T15:14:50Z

Updated from 58dca47 to d9e421c (pr409.03 -> pr409.04)
Addressed @jarolrod comment.

Changes:

Changed title of Create Wallet loading window from “Open Wallet” to “Create Wallet.”

hebasto

ACK d9e421c

In commit message maybe
s/"qt: Fix window title of wallet loading window"/"qt: Fix WalletControllerActivity progress dialog title"/
?

hebasto · 2021-09-14T13:24:51Z

src/qt/walletcontroller.cpp

 void CreateWalletActivity::createWallet()
 {
-    showProgressDialog(tr("Creating Wallet <b>%1</b>…").arg(m_create_wallet_dialog->walletName().toHtmlEscaped()));
+    showProgressDialog(tr("Create Wallet"), tr("Creating Wallet <b>%1</b>…").arg(m_create_wallet_dialog->walletName().toHtmlEscaped()));


nit

... which uses the same text as the menu item.

Maybe mention this in the translator comment?

shaavan · 2021-09-22T13:24:06Z

Updated from d9e421c to 8e11d89 (pr409.04 -> pr409.05)
Addressed @hebasto suggestions

Changes:

Updated commit message to better explain the intent and changes done in this PR.
Added Translator comments. I would request reviewers to review the comments and provide their valuable suggestions.

hebasto

ACK 8e11d89

shaavan · 2021-09-30T19:20:42Z

Updated from 8e11d89 to e4bf580 (pr409.05 -> pr409.06)

Updates:

Rebased on current Master

hebasto · 2021-09-30T19:26:19Z

Updated from 8e11d89 to e4bf580 (pr409.05 -> pr409.06)

Updates:
* Rebased on current Master

It seems wrong rebase...

shaavan · 2021-10-01T12:37:41Z

Updated from e4bf580 to a086a37 (pr409.06 -> pr409.07)

Changes:

Corrected the rebasing of PR on the current Master.
Used “Load Wallet” as the title for the progress window, which is displayed when wallets are being loaded at the startup of GUI.
Added translator’s comment for the “Load Wallet” progress window accordingly.

shaavan · 2021-10-02T13:30:07Z

@hebasto @jarolrod
Would you mind reviewing the PR again after the recent commit? Thanks

src/qt/walletcontroller.cpp

shaavan · 2021-10-04T08:06:46Z

Updated from a086a37 to ccecfa8 (pr409.07 -> pr409.08)

Addressed @hebasto and @jarolrod comments

Changes:

Load Wallet -> Load Wallets
Clang format corrected
Separated HTML tags from strings to avoid the risk of error
Translator comments have been expanded upon and are now highly descriptive and clear thanks to @jarolrod suggestions
Updated commit message to account for changes in PR.

luke-jr · 2021-10-05T01:08:55Z

I agree with hebasto. The strings shouldn't be broken up. Some languages might need to reorder the sentence.

shaavan · 2021-10-05T13:01:33Z

Updated from ccecfa8 to 9de84f6 (pr409.08 -> pr409.09)

Addressed @hebasto and @luke-jr suggestions

Changes:

Rejoined the separated HTML tags with the strings.

hebasto

ACK 9de84f6

src/qt/walletcontroller.cpp

shaavan · 2021-10-05T16:45:11Z

Updated from 9de84f6 to f86fe19 (pr409.09 -> pr409.10)

Addressed @hebasto suggestion

Changes:

Corrected a grammatical mistake in the translator's comments.

luke-jr

utACK f86fe19

jarolrod · 2021-10-06T03:41:53Z

Almost there, just need to update one of the translator comments: #409 (comment)

Tested on macOS 12, here are all the new window titles in action:

open wallet	create wallet	load wallet

The WalletControllerActivity progress dialog had title of "Bitcoin-Qt". The window title for opening wallet window should be "Open Wallet", for creating wallet window should be "Create Wallet", and for the window that is displayed when wallets are being loaded at startup should be "Load Wallets". This PR fixes that.

shaavan · 2021-10-06T12:13:40Z

Updated from f86fe19 to 01bff8f (pr409.10 -> pr409.11)

Addressed @jarolrod’s suggestion

Changes:

Added the translator’s comment suggestion which was not pushed earlier in the PR.

hebasto

ACK 01bff8f, tested on Linux Mint 20.2 (Qt 5.12.8).

jarolrod · 2021-10-06T18:27:28Z

ACK 01bff8f

verified that the only change since my last review is updating the missing translator comment by comparing the pr409.10 and pr409.11 branches: diff

shaavan · 2021-10-06T18:42:45Z

Thank you very much to everyone who took out time to review this PR. Especially, thanks to @jarolrod and @hebasto for sticking out till last in reviewing this PR and helping in improving it.

01bff8f qt: Fix WalletControllerActivity progress dialog title (Shashwat) Pull request description: Throughout the GUI, the title of the window, tells about the purpose of the window. This was not true for the title of wallet loading wallet. This PR fixes this issue by renaming the wallet loading window title to 'Open Wallet' Changes introduced in this PR (Runned Bitcoin-GUI on signet network) |Master|PR| |---|---| |![Screenshot from 2021-08-24 00-02-18](https://user-images.githubusercontent.com/85434418/130500309-2f0af2c9-55f0-4609-a92b-3156800fa92e.png)|![Screenshot from 2021-09-07 18-19-10](https://user-images.githubusercontent.com/85434418/132351394-1ee4a36c-3ba9-4d1a-a8f3-f17804fb856a.png)| ACKs for top commit: jarolrod: ACK 01bff8f hebasto: ACK 01bff8f, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: cd21c40752eb1c0afb5ec61b8a40e900bc3aa05749963f7957ece6024e4957f5bb37e0eb4f95aac488f5e08aea51fe13b023b05d8302a08c88dcc6790410ba64

hebasto added the UI All about "look and feel" label Aug 23, 2021

jarolrod reviewed Aug 24, 2021

View reviewed changes

shaavan force-pushed the wallet-window-title branch from bb7aae1 to abcc83a Compare August 24, 2021 14:38

hebasto approved these changes Aug 26, 2021

View reviewed changes

jarolrod reviewed Aug 26, 2021

View reviewed changes

promag reviewed Sep 3, 2021

View reviewed changes

src/qt/walletcontroller.cpp Outdated Show resolved Hide resolved

shaavan force-pushed the wallet-window-title branch from abcc83a to 58dca47 Compare September 7, 2021 13:15

jarolrod reviewed Sep 7, 2021

View reviewed changes

src/qt/walletcontroller.cpp Outdated Show resolved Hide resolved

shaavan force-pushed the wallet-window-title branch from 58dca47 to d9e421c Compare September 7, 2021 15:14

hebasto approved these changes Sep 14, 2021

View reviewed changes

shaavan force-pushed the wallet-window-title branch from d9e421c to 8e11d89 Compare September 22, 2021 13:22

hebasto approved these changes Sep 29, 2021

View reviewed changes

DrahtBot added the Needs rebase label Sep 30, 2021

shaavan force-pushed the wallet-window-title branch from 8e11d89 to e4bf580 Compare September 30, 2021 19:20

DrahtBot removed the Needs rebase label Sep 30, 2021

shaavan force-pushed the wallet-window-title branch from e4bf580 to a086a37 Compare October 1, 2021 12:32

hebasto reviewed Oct 2, 2021

View reviewed changes

src/qt/walletcontroller.cpp Outdated Show resolved Hide resolved

jarolrod reviewed Oct 4, 2021

View reviewed changes

shaavan force-pushed the wallet-window-title branch from a086a37 to ccecfa8 Compare October 4, 2021 08:05

shaavan force-pushed the wallet-window-title branch from ccecfa8 to 9de84f6 Compare October 5, 2021 13:01

hebasto approved these changes Oct 5, 2021

View reviewed changes

src/qt/walletcontroller.cpp Outdated Show resolved Hide resolved

shaavan force-pushed the wallet-window-title branch from 9de84f6 to f86fe19 Compare October 5, 2021 16:44

luke-jr approved these changes Oct 5, 2021

View reviewed changes

shaavan force-pushed the wallet-window-title branch from f86fe19 to 01bff8f Compare October 6, 2021 12:12

hebasto approved these changes Oct 6, 2021

View reviewed changes

hebasto merged commit 577b0ff into bitcoin-core:master Oct 6, 2021

bitcoin-core locked as resolved and limited conversation to collaborators Oct 6, 2022

Conversation

shaavan commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 23, 2021

Uh oh!

hebasto commented Aug 23, 2021

Uh oh!

hebasto commented Aug 23, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Aug 24, 2021

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Aug 26, 2021

Uh oh!

jarolrod commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Aug 26, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shaavan commented Sep 7, 2021

Uh oh!

Uh oh!

shaavan commented Sep 7, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

shaavan commented Sep 22, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Sep 30, 2021

Uh oh!

hebasto commented Sep 30, 2021

Uh oh!

shaavan commented Oct 1, 2021

Uh oh!

shaavan commented Oct 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Oct 4, 2021

Uh oh!

luke-jr commented Oct 5, 2021

Uh oh!

shaavan commented Oct 5, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shaavan commented Oct 5, 2021

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Aug 23, 2021 •

edited

Loading

hebasto left a comment •

edited

Loading

jarolrod commented Aug 26, 2021 •

edited

Loading