[gui] Make proxy icon from statusbar clickable

mess110 commented at 9:45 pm on May 16, 2018: contributor

Clicking on the proxy icon will open settings showing the network tab

fanquake added the label GUI on May 16, 2018

in src/qt/bitcoingui.cpp:670 in a73098955f outdated

670-    OptionsDialog dlg(this, enableWallet);
671-    dlg.setModel(clientModel->getOptionsModel());
672-    dlg.exec();
673+void BitcoinGUI::proxyIconClicked()
674+{
675+    openOptionsDialogWithTabIndex(2);

jonasschnelli commented at 8:50 am on May 25, 2018:

nit: not sure if setting by the object name would make more sense. Something like setCurrentWidget(ui->tabNetwork).

mess110 commented at 12:55 pm on May 28, 2018:

Would need to get ui->tabNetwork and ui->tabMain at BitcoinGUI level and in my opinion that is not worth it. The main concern is related to magic values or if the tabs change. Magic values are not a real concern because they are “labeled” by the method calling them (ex: proxyIconClicked). I don’t think the order will change. Will it?

I tried something similar to this:

0     OptionsDialog dlg(this, enableWallet);
1-    dlg.setCurrentTabIndex(tabIndex);
2+    if (tabIndex == 0) {
3+      dlg.setTabMain();
4+    } else {
5+      dlg.setTabNetwork();
6+    }

It adds 2 new methods to OptionsDialog, but still needed to pass tabIndex because I would need to make a methods which return ui->tabNetwork and ui->tabMain.

So I think I prefer the current implementation. Opinions?

promag commented at 10:10 pm on May 28, 2018:

Could create an enum for tab indexes? So it would be something like

 0void BitcoinGUI::openOptionsDialogWithTab(OptionsDialog::Tab tab)
 1
 2// and
 3
 4void OptionsDialog::setCurrentTab(OptionsDialog::Tab tab)
 5{
 6    int index = tab;
 7    if (ui->tabWidget->currentIndex() != index) {
 8        ui->tabWidget->setCurrentIndex(index);
 9    }
10}

mess110 commented at 0:18 am on May 30, 2018:

Added the enum, makes the code more readable, thx

jonasschnelli commented at 8:51 am on May 25, 2018: contributor

utACK a73098955f119690d270f8d4c872445873c41cc5

sipa commented at 0:41 am on May 26, 2018: member

Concept ACK

promag commented at 12:58 pm on May 28, 2018: member

Concept ACK.

jonasschnelli commented at 7:58 am on May 30, 2018: contributor

Sorry for nitpicking…and thanks, it’s better, but we still have static index positions (that is what I’m trying to avoid) which violates MVC (need to change the code if we change the tab order).

After this (see diff), it would be independent:

 0diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
 1index 8c9fc0def..5aeb659e3 100644
 2--- a/src/qt/optionsdialog.cpp
 3+++ b/src/qt/optionsdialog.cpp
 4@@ -172,9 +172,11 @@ void OptionsDialog::setModel(OptionsModel *_model)
 5 
 6 void OptionsDialog::setCurrentTab(OptionsDialog::Tab tab)
 7 {
 8-    int index = tab;
 9-    if (ui->tabWidget->currentIndex() != index) {
10-        ui->tabWidget->setCurrentIndex(index);
11+    QWidget *tab_widget = NULL;
12+    if (tab == OptionsDialog::Tab::TAB_NETWORK) tab_widget = ui->tabNetwork;
13+    if (tab == OptionsDialog::Tab::TAB_MAIN) tab_widget = ui->tabMain;
14+    if (tab_widget && ui->tabWidget->currentWidget() != tab_widget) {
15+        ui->tabWidget->setCurrentWidget(tab_widget);
16     }
17 }
18 
19diff --git a/src/qt/optionsdialog.h b/src/qt/optionsdialog.h
20index ca537d92c..99b2fa7a2 100644
21--- a/src/qt/optionsdialog.h
22+++ b/src/qt/optionsdialog.h
23@@ -41,8 +41,8 @@ public:
24     ~OptionsDialog();
25 
26     enum Tab {
27-        TAB_MAIN = 0,
28-        TAB_NETWORK = 2,
29+        TAB_MAIN,
30+        TAB_NETWORK,
31     };
32 
33     void setModel(OptionsModel *model);

promag commented at 9:59 am on May 30, 2018: member

Agree with @jonasschnelli suggestion. nit s/NULL/nullptr above.

mess110 force-pushed on May 30, 2018

mess110 commented at 7:27 pm on May 30, 2018: contributor

@jonasschnelli with me at least, don’t be sorry for nitpicking. My goal is to write quality code and your suggestion is indeed an improvement. And it helps me learn. Thanks for the suggestion (@promag as well).

in src/qt/bitcoingui.cpp:196 in e937ff3518 outdated

249@@ -250,6 +250,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
250     subscribeToCoreSignals();
251 
252     connect(connectionsControl, SIGNAL(clicked(QPoint)), this, SLOT(toggleNetworkActive()));

MarcoFalke commented at 3:21 pm on July 22, 2018:

Mind to use the new connect syntax here?

0connect(connectionsControl, &GUIUtil::ClickableLabel::clicked, this, &BitcoinGUI::toggleNetworkActive);

promag commented at 3:47 pm on July 22, 2018:

Must change type of connectionsControl too

promag commented at 11:12 am on July 23, 2018:

Either wait for #13529 merge or do like https://github.com/bitcoin/bitcoin/pull/13529/files#diff-827883cef269471106f7a18fd8199c47R102

mess110 force-pushed on Jul 23, 2018

in src/qt/bitcoingui.cpp:197 in f63e742670 outdated

192@@ -193,7 +193,8 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
193     // Subscribe to notifications from core
194     subscribeToCoreSignals();
195 
196-    connect(connectionsControl, SIGNAL(clicked(QPoint)), this, SLOT(toggleNetworkActive()));
197+    connect(connectionsControl, &GUIUtil::ClickableLabel::clicked, this, &BitcoinGUI::toggleNetworkActive);
198+    connect(labelProxyIcon, SIGNAL(clicked(QPoint)), this, SLOT(proxyIconClicked()));

MarcoFalke commented at 9:56 pm on July 23, 2018:

Could use new syntax here as well?

mess110 commented at 10:05 pm on July 23, 2018:

ops, I changed the wrong one and assumed the other ones will be merged in #13529. Fixed now @MarcoFalke @promag I had to add GUIUtil namespace in the header file, hope the other PR will merge easily.

Should I squash the commits?

promag commented at 10:25 pm on July 23, 2018:

LGTM

Should I squash the commits?

I guess you could separate in 2 commits, the 1st add tab, 2nd make it clickable. However 1 commit should be just fine.

mess110 commented at 10:35 pm on July 23, 2018:

squashed

mess110 force-pushed on Jul 23, 2018

in src/qt/bitcoingui.cpp:197 in 1081f76913 outdated

192@@ -193,7 +193,8 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
193     // Subscribe to notifications from core
194     subscribeToCoreSignals();
195 
196-    connect(connectionsControl, SIGNAL(clicked(QPoint)), this, SLOT(toggleNetworkActive()));
197+    connect(connectionsControl, &GUIUtil::ClickableLabel::clicked, this, &BitcoinGUI::toggleNetworkActive);
198+    connect(labelProxyIcon, &GUIUtil::ClickableLabel::clicked, this, &BitcoinGUI::proxyIconClicked);

promag commented at 10:53 pm on July 23, 2018:

nit, I now have a preference for:

0connect(labelProxyIcon, &GUIUtil::ClickableLabel::clicked, [this] {
1    openOptionsDialogWithTab(OptionsDialog::TAB_NETWORK);
2});

for these kind of private slots.

mess110 commented at 11:23 pm on July 23, 2018:

Yea, now that I know of it, I like it more as well :smile:

in src/qt/optionsdialog.h:53 in 1081f76913 outdated

55@@ -50,7 +56,7 @@ private Q_SLOTS:
56     void on_openBitcoinConfButton_clicked();
57     void on_okButton_clicked();
58     void on_cancelButton_clicked();
59-

promag commented at 10:54 pm on July 23, 2018:

nit, unnecessary change.

mess110 commented at 11:24 pm on July 23, 2018:

I would call that unnecessary addition. If I were to make a PR which removes all whitespaces it would most likely be rejected, so instead, when I make a change to a file and I see whitespace on the screen, I remove it.

If its ok, I would like to keep it this way

promag commented at 9:55 pm on July 24, 2018:

Could you remove this change to avoid conflict with #13753.

mess110 commented at 10:20 pm on July 24, 2018:

I removed it, even though intentionally adding whitespaces (the ones I removed) caused a bit of sadness inside. Even if it was for a good cause.

There was another instance, I removed that as well.

At the end of the day, all trailing whitespaces will be removed which gives me a warm fuzzy feeling.

Thx for the reviews btw

promag commented at 10:54 pm on July 23, 2018: member

utACK 1081f76.

mess110 force-pushed on Jul 23, 2018

in src/qt/optionsdialog.cpp:175 in 76082cfc42 outdated

169@@ -170,6 +170,16 @@ void OptionsDialog::setModel(OptionsModel *_model)
170     connect(ui->thirdPartyTxUrls, SIGNAL(textChanged(const QString &)), this, SLOT(showRestartWarning()));
171 }
172 
173+void OptionsDialog::setCurrentTab(OptionsDialog::Tab tab)
174+{
175+    QWidget *tab_widget = nullptr;

promag commented at 2:30 pm on July 24, 2018:

Suggestion:

0if (tab == OptionsDialog::Tab::TAB_NETWORK) {
1    ui->tabWidget->setCurrentWidget(ui->tabNetwork);
2} else {
3    ui->tabWidget->setCurrentWidget(ui->tabMain);
4}

in src/qt/bitcoingui.cpp:769 in 76082cfc42 outdated

763@@ -764,6 +764,17 @@ void BitcoinGUI::updateHeadersSyncProgressLabel()
764         progressBarLabel->setText(tr("Syncing Headers (%1%)...").arg(QString::number(100.0 / (headersTipHeight+estHeadersLeft)*headersTipHeight, 'f', 1)));
765 }
766 
767+void BitcoinGUI::openOptionsDialogWithTab(OptionsDialog::Tab tab)
768+{
769+    if(!clientModel || !clientModel->getOptionsModel())

promag commented at 2:32 pm on July 24, 2018:

nit, add space after if.

mess110 commented at 9:49 pm on July 24, 2018:

added

promag commented at 2:33 pm on July 24, 2018: member

utACK 76082cf.

mess110 force-pushed on Jul 24, 2018

[gui] Make proxy icon from statusbar clickable

Clicking on the proxy icon will open settings showing the network tab
Create enum Tab in OptionsModel
Use new connect syntax
Use lambda for private slots

6d5fcad576

mess110 force-pushed on Jul 24, 2018

laanwj commented at 11:40 am on August 20, 2018: member

works-for-me ACK 6d5fcad576962e5950641f7e7b113a6ac6f397e5

laanwj merged this on Aug 20, 2018

laanwj closed this on Aug 20, 2018

laanwj referenced this in commit 2a583406c0 on Aug 20, 2018

mess110 deleted the branch on Aug 21, 2018

jasonbcox referenced this in commit 2d293e74c9 on Oct 25, 2019

Munkybooty referenced this in commit 1127b318ef on Jun 30, 2021

Munkybooty referenced this in commit 3cc0bd45e7 on Sep 8, 2021

MarcoFalke locked this on Sep 8, 2021

[gui] Make proxy icon from statusbar clickable #13248