[gui] Make proxy icon from statusbar clickable #13248

mess110 · 2018-05-16T21:45:52Z

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

jonasschnelli · 2018-05-25T08:50:54Z

src/qt/bitcoingui.cpp

-    dlg.exec();
+void BitcoinGUI::proxyIconClicked()
+{
+    openOptionsDialogWithTabIndex(2);


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

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:

OptionsDialog dlg(this, enableWallet); - dlg.setCurrentTabIndex(tabIndex); + if (tabIndex == 0) { + dlg.setTabMain(); + } else { + dlg.setTabNetwork(); + }

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?

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

void BitcoinGUI::openOptionsDialogWithTab(OptionsDialog::Tab tab) // and void OptionsDialog::setCurrentTab(OptionsDialog::Tab tab) { int index = tab; if (ui->tabWidget->currentIndex() != index) { ui->tabWidget->setCurrentIndex(index); } }

Added the enum, makes the code more readable, thx

jonasschnelli · 2018-05-25T08:51:07Z

utACK a73098955f119690d270f8d4c872445873c41cc5

sipa · 2018-05-26T00:41:26Z

Concept ACK

promag · 2018-05-28T12:58:44Z

Concept ACK.

jonasschnelli · 2018-05-30T07:58:54Z

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:

diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
index 8c9fc0def..5aeb659e3 100644
--- a/src/qt/optionsdialog.cpp
+++ b/src/qt/optionsdialog.cpp
@@ -172,9 +172,11 @@ void OptionsDialog::setModel(OptionsModel *_model)
 
 void OptionsDialog::setCurrentTab(OptionsDialog::Tab tab)
 {
-    int index = tab;
-    if (ui->tabWidget->currentIndex() != index) {
-        ui->tabWidget->setCurrentIndex(index);
+    QWidget *tab_widget = NULL;
+    if (tab == OptionsDialog::Tab::TAB_NETWORK) tab_widget = ui->tabNetwork;
+    if (tab == OptionsDialog::Tab::TAB_MAIN) tab_widget = ui->tabMain;
+    if (tab_widget && ui->tabWidget->currentWidget() != tab_widget) {
+        ui->tabWidget->setCurrentWidget(tab_widget);
     }
 }
 
diff --git a/src/qt/optionsdialog.h b/src/qt/optionsdialog.h
index ca537d92c..99b2fa7a2 100644
--- a/src/qt/optionsdialog.h
+++ b/src/qt/optionsdialog.h
@@ -41,8 +41,8 @@ public:
     ~OptionsDialog();
 
     enum Tab {
-        TAB_MAIN = 0,
-        TAB_NETWORK = 2,
+        TAB_MAIN,
+        TAB_NETWORK,
     };
 
     void setModel(OptionsModel *model);

promag · 2018-05-30T09:59:53Z

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

mess110 · 2018-05-30T19:27:55Z

@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).

maflcko · 2018-07-22T15:21:33Z

src/qt/bitcoingui.cpp

@@ -250,6 +250,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
    subscribeToCoreSignals();

    connect(connectionsControl, SIGNAL(clicked(QPoint)), this, SLOT(toggleNetworkActive()));


Mind to use the new connect syntax here?

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

Must change type of connectionsControl too

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

maflcko · 2018-07-23T21:56:32Z

src/qt/bitcoingui.cpp

@@ -193,7 +193,8 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
    // Subscribe to notifications from core
    subscribeToCoreSignals();

-    connect(connectionsControl, SIGNAL(clicked(QPoint)), this, SLOT(toggleNetworkActive()));
+    connect(connectionsControl, &GUIUtil::ClickableLabel::clicked, this, &BitcoinGUI::toggleNetworkActive);
+    connect(labelProxyIcon, SIGNAL(clicked(QPoint)), this, SLOT(proxyIconClicked()));


Could use new syntax here as well?

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?

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.

promag

utACK 1081f76.

promag · 2018-07-23T22:53:45Z

src/qt/bitcoingui.cpp

@@ -193,7 +193,8 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
    // Subscribe to notifications from core
    subscribeToCoreSignals();

-    connect(connectionsControl, SIGNAL(clicked(QPoint)), this, SLOT(toggleNetworkActive()));
+    connect(connectionsControl, &GUIUtil::ClickableLabel::clicked, this, &BitcoinGUI::toggleNetworkActive);
+    connect(labelProxyIcon, &GUIUtil::ClickableLabel::clicked, this, &BitcoinGUI::proxyIconClicked);


nit, I now have a preference for:

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

for these kind of private slots.

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

promag · 2018-07-23T22:54:06Z

src/qt/optionsdialog.h

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


nit, unnecessary change.

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

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

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

utACK 76082cf.

promag · 2018-07-24T14:30:54Z

src/qt/optionsdialog.cpp

@@ -170,6 +170,16 @@ void OptionsDialog::setModel(OptionsModel *_model)
    connect(ui->thirdPartyTxUrls, SIGNAL(textChanged(const QString &)), this, SLOT(showRestartWarning()));
 }

+void OptionsDialog::setCurrentTab(OptionsDialog::Tab tab)
+{
+    QWidget *tab_widget = nullptr;


Suggestion:

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

promag · 2018-07-24T14:32:06Z

src/qt/bitcoingui.cpp

@@ -764,6 +764,17 @@ void BitcoinGUI::updateHeadersSyncProgressLabel()
        progressBarLabel->setText(tr("Syncing Headers (%1%)...").arg(QString::number(100.0 / (headersTipHeight+estHeadersLeft)*headersTipHeight, 'f', 1)));
 }

+void BitcoinGUI::openOptionsDialogWithTab(OptionsDialog::Tab tab)
+{
+    if(!clientModel || !clientModel->getOptionsModel())


nit, add space after if.

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

laanwj · 2018-08-20T11:40:03Z

works-for-me ACK 6d5fcad

6d5fcad [gui] Make proxy icon from statusbar clickable (Cristian Mircea Messel) Pull request description: Clicking on the proxy icon will open settings showing the network tab #11491 (comment) Tree-SHA512: c3549749296918818694a371326d1a3b1075478918aaee940b5c7119a7e2cb991dcfda78f20d44d6d001157b9b82951f0d5157b17f4f0d1a0a242795efade036

Summary: 6d5fcad576962e5950641f7e7b113a6ac6f397e5 [gui] Make proxy icon from statusbar clickable (Cristian Mircea Messel) Pull request description: Clicking on the proxy icon will open settings showing the network tab bitcoin/bitcoin#11491 (comment) Tree-SHA512: c3549749296918818694a371326d1a3b1075478918aaee940b5c7119a7e2cb991dcfda78f20d44d6d001157b9b82951f0d5157b17f4f0d1a0a242795efade036 Backport of Core PR13248 bitcoin/bitcoin#13248 Depends on D4269 Test Plan: make check ./bitcoin-qt -proxy=<valid proxy> Clicking the `P` icon in the bottom right corner should open up the `network` tab in the `options` menu. Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4270

6d5fcad [gui] Make proxy icon from statusbar clickable (Cristian Mircea Messel) Pull request description: Clicking on the proxy icon will open settings showing the network tab bitcoin#11491 (comment) Tree-SHA512: c3549749296918818694a371326d1a3b1075478918aaee940b5c7119a7e2cb991dcfda78f20d44d6d001157b9b82951f0d5157b17f4f0d1a0a242795efade036 # Conflicts: # src/qt/bitcoingui.cpp # src/qt/bitcoingui.h

6d5fcad [gui] Make proxy icon from statusbar clickable (Cristian Mircea Messel) Pull request description: Clicking on the proxy icon will open settings showing the network tab bitcoin#11491 (comment) Tree-SHA512: c3549749296918818694a371326d1a3b1075478918aaee940b5c7119a7e2cb991dcfda78f20d44d6d001157b9b82951f0d5157b17f4f0d1a0a242795efade036

fanquake added the GUI label May 16, 2018

jonasschnelli reviewed May 25, 2018

View reviewed changes

mess110 force-pushed the make_proxy_icon_clickable branch from 5fc5a53 to e937ff3 Compare May 30, 2018 19:12

This was referenced Jul 16, 2018

Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue #13217) #13674

Closed

Use new Qt5 connect syntax #13529

Merged

maflcko reviewed Jul 22, 2018

View reviewed changes

mess110 force-pushed the make_proxy_icon_clickable branch from e937ff3 to 0b54e89 Compare July 23, 2018 21:32

maflcko reviewed Jul 23, 2018

View reviewed changes

mess110 force-pushed the make_proxy_icon_clickable branch 2 times, most recently from 82e6fc7 to 1081f76 Compare July 23, 2018 22:35

promag reviewed Jul 23, 2018

View reviewed changes

mess110 force-pushed the make_proxy_icon_clickable branch from 1081f76 to 76082cf Compare July 23, 2018 23:20

promag reviewed Jul 24, 2018

View reviewed changes

promag mentioned this pull request Jul 24, 2018

scripted-diff: Remove trailing whitespaces #13753

Merged

mess110 force-pushed the make_proxy_icon_clickable branch from 76082cf to dd4b263 Compare July 24, 2018 21:49

[gui] Make proxy icon from statusbar clickable

6d5fcad

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

mess110 force-pushed the make_proxy_icon_clickable branch from dd4b263 to 6d5fcad Compare July 24, 2018 22:04

laanwj merged commit 6d5fcad into bitcoin:master Aug 20, 2018

mess110 deleted the make_proxy_icon_clickable branch August 21, 2018 21:08

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -250,6 +250,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
		subscribeToCoreSignals();

		connect(connectionsControl, SIGNAL(clicked(QPoint)), this, SLOT(toggleNetworkActive()));

[gui] Make proxy icon from statusbar clickable #13248

[gui] Make proxy icon from statusbar clickable #13248

Uh oh!

Conversation

mess110 commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mess110 May 28, 2018 • 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!

promag May 28, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 25, 2018

Uh oh!

sipa commented May 26, 2018

Uh oh!

promag commented May 28, 2018

Uh oh!

jonasschnelli commented May 30, 2018

Uh oh!

promag commented May 30, 2018

Uh oh!

mess110 commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 20, 2018

Uh oh!

Uh oh!

mess110 May 28, 2018 •

edited

Loading

promag May 28, 2018 •

edited

Loading