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

[gui] Make proxy icon from statusbar clickable #13248

Merged
merged 1 commit into from Aug 20, 2018

Conversation

Projects
None yet
7 participants
@mess110
Copy link
Contributor

commented May 16, 2018

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

#11491 (comment)

@fanquake fanquake added the GUI label May 16, 2018

src/qt/bitcoingui.cpp Outdated
dlg.exec();
void BitcoinGUI::proxyIconClicked()
{
openOptionsDialogWithTabIndex(2);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 25, 2018

Member

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

This comment has been minimized.

Copy link
@mess110

mess110 May 28, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@promag

promag May 28, 2018

Member

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);
    }
}

This comment has been minimized.

Copy link
@mess110

mess110 May 30, 2018

Author Contributor

Added the enum, makes the code more readable, thx

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 25, 2018

utACK a73098955f119690d270f8d4c872445873c41cc5

@sipa

This comment has been minimized.

Copy link
Member

commented May 26, 2018

Concept ACK

@promag

This comment has been minimized.

Copy link
Member

commented May 28, 2018

Concept ACK.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 30, 2018

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

This comment has been minimized.

Copy link
Member

commented May 30, 2018

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

@mess110 mess110 force-pushed the mess110:make_proxy_icon_clickable branch May 30, 2018

@mess110

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

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

src/qt/bitcoingui.cpp Outdated
@@ -250,6 +250,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
subscribeToCoreSignals();

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 22, 2018

Member

Mind to use the new connect syntax here?

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

This comment has been minimized.

Copy link
@promag

promag Jul 22, 2018

Member

Must change type of connectionsControl too

This comment has been minimized.

@mess110 mess110 force-pushed the mess110:make_proxy_icon_clickable branch Jul 23, 2018

src/qt/bitcoingui.cpp Outdated
@@ -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()));

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 23, 2018

Member

Could use new syntax here as well?

This comment has been minimized.

Copy link
@mess110

mess110 Jul 23, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@promag

promag Jul 23, 2018

Member

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.

This comment has been minimized.

Copy link
@mess110

mess110 Jul 23, 2018

Author Contributor

squashed

@mess110 mess110 force-pushed the mess110:make_proxy_icon_clickable branch 2 times, most recently Jul 23, 2018

@promag
Copy link
Member

left a comment

utACK 1081f76.

src/qt/bitcoingui.cpp Outdated
@@ -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);

This comment has been minimized.

Copy link
@promag

promag Jul 23, 2018

Member

nit, I now have a preference for:

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

for these kind of private slots.

This comment has been minimized.

Copy link
@mess110

mess110 Jul 23, 2018

Author Contributor

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

src/qt/optionsdialog.h Outdated
@@ -50,7 +56,7 @@ private Q_SLOTS:
void on_openBitcoinConfButton_clicked();
void on_okButton_clicked();
void on_cancelButton_clicked();

This comment has been minimized.

Copy link
@promag

promag Jul 23, 2018

Member

nit, unnecessary change.

This comment has been minimized.

Copy link
@mess110

mess110 Jul 23, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@promag

promag Jul 24, 2018

Member

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

This comment has been minimized.

Copy link
@mess110

mess110 Jul 24, 2018

Author Contributor

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

@mess110 mess110 force-pushed the mess110:make_proxy_icon_clickable branch Jul 23, 2018

@promag
Copy link
Member

left a comment

utACK 76082cf.

@@ -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;

This comment has been minimized.

Copy link
@promag

promag Jul 24, 2018

Member

Suggestion:

if (tab == OptionsDialog::Tab::TAB_NETWORK) {
    ui->tabWidget->setCurrentWidget(ui->tabNetwork);
} else {
    ui->tabWidget->setCurrentWidget(ui->tabMain);
}
src/qt/bitcoingui.cpp Outdated
@@ -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())

This comment has been minimized.

Copy link
@promag

promag Jul 24, 2018

Member

nit, add space after if.

This comment has been minimized.

Copy link
@mess110

mess110 Jul 24, 2018

Author Contributor

added

@mess110 mess110 force-pushed the mess110:make_proxy_icon_clickable branch 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

@mess110 mess110 force-pushed the mess110:make_proxy_icon_clickable branch to 6d5fcad Jul 24, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

works-for-me ACK 6d5fcad

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 20, 2018

Merge #13248: [gui] Make proxy icon from statusbar clickable
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

@mess110 mess110 deleted the mess110:make_proxy_icon_clickable branch Aug 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.