Relates to #7734
Please ignore the wrong alpha in the screenshot, I couldn’t get the screenshot alpha right :(
I plan to extend this feature in future PRs to include:
Old proposals, dropped in favor of current
277@@ -278,6 +278,8 @@ RES_ICONS = \
278 qt/res/icons/network_disabled.png \
279 qt/res/icons/open.png \
280 qt/res/icons/overview.png \
281+ qt/res/icons/proxy_enabled.png \
1091@@ -1084,6 +1092,30 @@ void BitcoinGUI::setEncryptionStatus(int status)
1092 }
1093 #endif // ENABLE_WALLET
1094
1095+void BitcoinGUI::updateProxyIcon()
1096+{
1097+ bool proxyEnabled = false;
1098+ proxyType proxy;
1099+ UniValue networks(UniValue::VARR);
#include above too.
1108+ proxyEnabled = true;
1109+ break;
1110+ }
1111+ }
1112+
1113+ labelProxyIcon->show();
1110+ }
1111+ }
1112+
1113+ labelProxyIcon->show();
1114+ labelProxyIcon->setPixmap(platformStyle->SingleColorIcon(proxyEnabled ? ":/icons/proxy_enabled" : ":/icons/proxy_disabled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));
1115+ labelProxyIcon->setToolTip(proxyEnabled ? tr("Proxy is <b>enabled</b>: %1").arg(QString::fromStdString(proxy.proxy.ToStringIPPort())) : tr("Proxy is <b>disabled</b>"));
Proxy: %1 else Proxy disabled.
1091@@ -1084,6 +1092,30 @@ void BitcoinGUI::setEncryptionStatus(int status)
1092 }
1093 #endif // ENABLE_WALLET
1094
1095+void BitcoinGUI::updateProxyIcon()
1096+{
1097+ bool proxyEnabled = false;
proxy_enabled.
1096+{
1097+ bool proxyEnabled = false;
1098+ proxyType proxy;
1099+ UniValue networks(UniValue::VARR);
1100+
1101+ for(int n=0; n<NET_MAX; ++n)
for (int n = 0; n < NET_MAX; ++n) {
1099+ UniValue networks(UniValue::VARR);
1100+
1101+ for(int n=0; n<NET_MAX; ++n)
1102+ {
1103+ enum Network network = static_cast<enum Network>(n);
1104+ if(network == NET_UNROUTABLE || network == NET_INTERNAL)
if. continue in this line.
1102+ {
1103+ enum Network network = static_cast<enum Network>(n);
1104+ if(network == NET_UNROUTABLE || network == NET_INTERNAL)
1105+ continue;
1106+ GetProxy(network, proxy);
1107+ if(proxy.IsValid()) {
if.
1109+ break;
1110+ }
1111+ }
1112+
1113+ labelProxyIcon->show();
1114+ labelProxyIcon->setPixmap(platformStyle->SingleColorIcon(proxyEnabled ? ":/icons/proxy_enabled" : ":/icons/proxy_disabled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));
Space after ,.
Nit, split in 2 lines? Or something like:
0if (proxy_enabled) {
1 ...
2} else {
3 ...
4}
contrib/debian/copyright?
52@@ -53,6 +53,8 @@
53 <file alias="hd_enabled">res/icons/hd_enabled.png</file>
54 <file alias="hd_disabled">res/icons/hd_disabled.png</file>
55 <file alias="network_disabled">res/icons/network_disabled.png</file>
56+ <file alias="proxy_enabled">res/icons/proxy_enabled.png</file>
1090@@ -1084,6 +1091,26 @@ void BitcoinGUI::setEncryptionStatus(int status)
1091 }
1092 #endif // ENABLE_WALLET
1093
1094+void BitcoinGUI::updateProxyIcon()
1095+{
1096+ bool proxy_enabled = false;
1097+ proxyType proxy;
1098+
1099+ for (int n = 0; n < NET_MAX; ++n) {
GetProxy returns IsValid so something like this should work:
0for (int n = 0; !proxy_enabled && n < NET_MAX; ++n) {
1 proxy_enabled = GetProxy((Network) n, proxy);
2}
189@@ -189,6 +190,9 @@ public Q_SLOTS:
190 void incomingTransaction(const QString& date, int unit, const CAmount& amount, const QString& type, const QString& address, const QString& label);
191 #endif // ENABLE_WALLET
192
193+/** Set the proxy-enabled icon as shown in the UI. */
194+void updateProxyIcon();
1095+{
1096+ bool proxy_enabled = false;
1097+ proxyType proxy;
1098+
1099+ for (int n = 0; !proxy_enabled && n < NET_MAX; ++n) {
1100+ proxy_enabled = GetProxy((Network) n, proxy);
@wodry guess I wasn’t 100% clear. I attempted to construct a proxy icon from 3 copies of https://github.com/bitcoin/bitcoin/blob/master/src/qt/res/icons/debugwindow.png but it was too much to fit in the space and the icon ended up looking too different from the others. So I chose the icon you see because it fits and is public domain @jonasschnelli I checked the link you provided. Indeed it would be nice to include from the same set, but I couldn’t find one that matches. Imo, the current icon is better. However, here are a few long shots:
Tested a bit:
The new icon is made from 3 device-desktop and 1 rotated chevron from http://s-ings.com/typicons/
After talking with @jonasschnelli, I will do the clickable icon in a different PR.
9@@ -10,6 +10,7 @@
10 #endif
11
12 #include "amount.h"
13+#include <univalue.h>
336@@ -336,3 +337,20 @@ void ClientModel::unsubscribeFromCoreSignals()
337 uiInterface.NotifyBlockTip.disconnect(boost::bind(BlockTipChanged, this, _1, _2, false));
338 uiInterface.NotifyHeaderTip.disconnect(boost::bind(BlockTipChanged, this, _1, _2, true));
339 }
340+
341+UniValue ClientModel::getProxyInfo() const
Suggestion to remove UniValue dependency:
0bool ClientModel::GetProxyInfo(std::string& ip_port) const
1{
2 for (int n = 0; n < NET_MAX; ++n) {
3 proxyType proxy;
4 if (GetProxy((Network) n, proxy)) {
5 ip_port = proxy.proxy.ToStringIPPort();
6 return true;
7 }
8 }
9 return false;
10}
Haven’t tested.
@rebroad UI evolve because of these changes, eventually recognising less is more. I also don’t appreciate the icon there, but this is for sure something that can be improved. I would only show mutable state information in the status bar, and settings is not state.
IMO icon is unrecognisable in small size. Beside doesn’t feel related to proxy at all (personal opinion).
1098+ if (proxy_enabled) {
1099+ QString ip_port_q = QString::fromStdString(ip_port);
1100+
1101+ labelProxyIcon->setPixmap(platformStyle->SingleColorIcon(":/icons/proxy").pixmap(STATUSBAR_ICONSIZE, STATUSBAR_ICONSIZE));
1102+ labelProxyIcon->setToolTip(tr("Proxy is <b>enabled</b>: %1").arg(ip_port_q));
1103+ labelProxyIcon->setEnabled(proxy_enabled);
1096+ bool proxy_enabled = clientModel->getProxyInfo(ip_port);
1097+
1098+ if (proxy_enabled) {
1099+ QString ip_port_q = QString::fromStdString(ip_port);
1100+
1101+ labelProxyIcon->setPixmap(platformStyle->SingleColorIcon(":/icons/proxy").pixmap(STATUSBAR_ICONSIZE, STATUSBAR_ICONSIZE));
Although toggling proxy requires a restart, I prefer it if the UI doesn’t assume that (to prevent a regression later). Not a show-stopper perse, but this code could confuse people.
Suggestion: add an if statement that checks if the icon has already been drawn. If so, call show().
336@@ -336,3 +337,15 @@ void ClientModel::unsubscribeFromCoreSignals()
337 uiInterface.NotifyBlockTip.disconnect(boost::bind(BlockTipChanged, this, _1, _2, false));
338 uiInterface.NotifyHeaderTip.disconnect(boost::bind(BlockTipChanged, this, _1, _2, true));
339 }
340+
341+bool ClientModel::getProxyInfo(std::string& ip_port) const
std::pair<bool, std::string> ClientModel::getProxyInfo() const;, both work for me.
IMO the icon could be improved. Not sure if it should be visible only when the proxy is enabled.
Edit: Feels better without the disabled proxy icon.
The icons at the bottom are already quite confusing. Some allow the user to take action (e.g. BTC), another does not allow action and never chances (HD). The network icon allows the user to take action, but unlike the BTC icon doesn’t first show a dropdown menu, but instead disables your network without warning.
Instead of adding another icon, I suggest turning the connection icon into something that opens a dialog, where you inspect and change network settings (including disconnecting, using a proxy or tor). The shape or color of the icon could give a hint as to the current state (e.g. green for Tor), but once the user clicks on it they would get the full information. That way the icon doesn’t need to be perfect.
t icon for future Tor PR.
@Sjors yea, the icons can be a bit confusing. This is why in the first version of this PR, I created proxy enabled/disabled similar to how the network icon behaves. However, each icon has slightly different behavior and needs. I think the security benefit of seeing the proxy icon is worth it though.
-onion).
I wonder if a small “P” (and later “T” for Tor) in the connection icon would do the trick…
Implemented this. It’s too small. :(
I think the security benefit of seeing the proxy icon is worth it though.
I tend to agree. Let’s be pragmatic about ACKing this when the functionality works, aesthetics can always be improved later.
@jonasschnelli agree and thanks. I rebased and added a proxy icon (as a simple P)
Should have some time over the weekend to do the “T” for Tor
This indicates proxy for me, even though I don’t use a proxy (but I have Tor enabled with -onion).
Reminder this bug persists (but in theory can be easiest solved by having the Tor icon).
The way I was thinking of detecting if Tor is enabled is if the n variable from https://github.com/bitcoin/bitcoin/pull/11491/files#diff-2c51f64a3430117d2f6c7cb55355be66R343 is equal to https://github.com/bitcoin/bitcoin/blob/master/src/netaddress.h#L24 (in this case 3).
Would that be enough?
If not, for this PR I can just check that all n are set which does not happen for -onion (thx @luke-jr for the tip)
A possible kludgey check if tor is actually up and running is to just send a dummy HTTP request directly to the tor socks proxy port. If tor is running, it will politely respond with a HTTP/1.0 501 Tor is not an HTTP Proxy, if it isn’t, it won’t.
This misses the possibility that the tor client is running but unable to connect, however that would result in a broken connection (as opposed to an accidental clearnet connection). However, I can’t think of any way to verify tor is actually working other than either asking tor-arm (which might not be there) or requesting https://check.torproject.org/ through the proxy (like the TBB does).
NET_MAX is 5. I checked to see what I get from GetProxy for different options:
| 0 | 1 | 2 | 3 | 4 | |
|---|---|---|---|---|---|
| with no options | false | false | false | false | false |
| with -proxy | false | true | true | true | false |
| with gui default proxy | false | true | true | true | false |
| with gui separate proxy | false | false | false | true | false |
| with both gui options | false | true | true | true | false |
| with -onion | false | false | false | true | false |
| with -onion and all gui options | false | true | true | true | false |
| with -onion and gui default proxy | false | true | true | true | false |
Looking at the table above, I can not reliably determine if the proxy icon should be shown by checking the return value of GetProxy. Any advice on how to proceed?
Note that the network ids aren’t just arbitrary numbers - they have meanings (defined in netaddress.h).
Anyhow, “gui separate proxy” is a Tor-specific option. So we shouldn’t show “proxy” for that either.
“Proxy” icon should probably only be shown if NET_IPV4 and NET_IPV6 have a GetProxy result.
The Tor icon might be more complicated. Ideally, clicking on it would show a popup with the hidden service address and QR Code, so mobile wallets can autoconfigure. But I guess that’s out-of-scope for this PR.
Rebased and made sure the proxy icon is shown if NET_IPV4 and NET_IPV6 have a result.
For the tooltip, I am showing ToStringIPPort for NET_IPV4. I couldn’t get an ipv6 to work with the -proxy argument. Also, ToStringIPPort for NET_IPV6 returns an ipv4 format.
75@@ -76,8 +76,8 @@ Comment:
76
77 Files: src/qt/res/icons/clock*.png
78 src/qt/res/icons/eye_*.png
79- src/qt/res/icons/verify.png
80 src/qt/res/icons/tx_in*.png
81+ src/qt/res/icons/verify.png
Tested through QT settings and -proxy launch flag.
The current silent failure (e.g. if you set a non existent proxy address) is suboptimal. Perhaps a future PR can put a red cross through the P icon to show there’s a problem.
Adding a test would be nice.
Tested ACK 35d50aa833cea53f9c6195987256351f4b1c710a
I don’t think it’s a good idea to use a “P” icon because the word for “Proxy” is going to be different between languages. It should be replaced with a symbol. But that can be changed later, it should not hold up the merge.
268@@ -268,3 +269,13 @@ void ClientModel::unsubscribeFromCoreSignals()
269 m_handler_notify_block_tip->disconnect();
270 m_handler_notify_header_tip->disconnect();
271 }
272+
273+bool ClientModel::getProxyInfo(std::string& ip_port) const
274+{
275+ proxyType ipv4, ipv6;
276+ if (GetProxy((Network) 1, ipv4) && GetProxy((Network) 2, ipv6)) {
netbase.h is still needed for proxyType, but I can change to m_node.getProxy
82@@ -83,6 +83,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
83 unitDisplayControl(0),
84 labelWalletEncryptionIcon(0),
85 labelWalletHDStatusIcon(0),
86+ labelProxyIcon(0),
1137+ bool proxy_enabled = clientModel->getProxyInfo(ip_port);
1138+
1139+ if (proxy_enabled) {
1140+ QString ip_port_q = QString::fromStdString(ip_port);
1141+
1142+ if (labelProxyIcon->pixmap() == 0) {
labelWalletHDStatusIcon, so I will prefer the approach taken
1135+{
1136+ std::string ip_port;
1137+ bool proxy_enabled = clientModel->getProxyInfo(ip_port);
1138+
1139+ if (proxy_enabled) {
1140+ QString ip_port_q = QString::fromStdString(ip_port);
utACK 35d50aa.
I have a couple of nits for you to consider or feel free to ignore them and keep the above ACKs.
Thanks for the reviews.
I inlined the string creation and using m_node.getProxy