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

Backport Bitcoin Qt/Gui changes up to 0.14.x part 2 #1615

Merged
merged 26 commits into from
Sep 9, 2017

Conversation

codablock
Copy link

@codablock codablock commented Sep 7, 2017

This is the next batch of backported Qt/Gui related PRs. See #1614 for the first batch.

The PRs included in this batch should all work fine on their own, so if you feel one of them is too critical I can simply remove it.

The most critical here is probably bitcoin#8463, which removes the priority column from coin control. In the Bitcoin community this seemed to be mostly unused so they decided to remove it. I'd assume this is also true for Dash.

bitcoin#8371 adds the progress overlay (which was the initial reason to do all the backporting). With this batch of PRs the overlay misses some of the improvements made in later commits (which will be in the next batch). The most important ones are 2 commits that allow to bring up the overlay by clicking on the spinning icon or the progress label. If time runs out (due to feature freeze), I can limit the next batch to only include the overlay improvements.

EDIT: Just realized I used the wrong link for the modal overlay PR. Fixed the description.

laanwj and others added 21 commits September 7, 2017 18:08
…ction

d6cc6a1 Use CCoinControl selection in CWallet::FundTransaction (João Barbosa)
…adir"

fc737d1 [Qt] remove unused formatBuildDate method (Jonas Schnelli)
4856f1d [Qt] Debug window: replace "Build date" with "Datadir" (Jonas Schnelli)
8efed3b [Qt] Support for abandoned/abandoning transactions (Jonas Schnelli)
…e help

c3932b3 List solvability in listunspent output and improve help (Pieter Wuille)
8b0e497 Qt: Add option to hide the system tray icon (Tyler Hardin)
02ce2a3 qt: askpassphrasedialog: Clear pass fields on accept (Pavel Vasin)
… hidden during startup

b3e1348 [Qt] fix a bug where the SplashScreen will not be hidden during startup (Jonas Schnelli)
1acf1db Do not ask a UI question from bitcoind (Pieter Wuille)
fa8dd78 [qt] Remove Priority from coincontrol dialog (MarcoFalke)
… paying unexpected fee

0480293 [Qt][CoinControl] fix UI bug that could result in paying unexpected fee (Jonas Schnelli)
…window

c015634 qt: Adding transaction size to transaction details window (Hampus Sjöberg)
 \-- merge fix for s/size/total size/
fdf82fb Adding method GetTotalSize() to CTransaction (Hampus Sjöberg)
08827df [Qt] modalinfolayer: removed unused comments, renamed signal, code style overhaul (Jonas Schnelli)
d8b062e [Qt] only update "amount of blocks left" when the header chain is in-sync (Jonas Schnelli)
e3245b4 [Qt] add out-of-sync modal info layer (Jonas Schnelli)
e47052f [Qt] ClientModel add method to get the height of the header chain (Jonas Schnelli)
a001f18 [Qt] Always pass the numBlocksChanged signal for headers tip changed (Jonas Schnelli)
bd44a04 [Qt] make Out-Of-Sync warning icon clickable (Jonas Schnelli)
0904c3c [Refactor] refactor function that forms human readable text out of a timeoffset (Jonas Schnelli)
c9ce17b Trivial: Grammar and capitalization (Derek Miller)
cb78c60 gui: fix ban from qt console (Cory Fields)
fa85e86 [qt] sync-overlay: Don't show estimated number of headers left (MarcoFalke)
faa4de2 [qt] sync-overlay: Don't block during reindex (MarcoFalke)
21f5a63 Qt: Add "Copy URI" to payment request context menu (Luke Dashjr)
…n no ping received yet

62a6486 RPC: do not print ping info in getpeerinfo when no ping received yet, fix help (Pavel Janík)
1724a40 Display minimum ping in debug window. (R E Broadley)
ef0c9ee [Qt] make warnings label selectable (Jonas Schnelli)
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of changes...

  1. I'm by no means GUI guy :) but warning icon looks a bit ugly on modal overlay imo, could be made a bit prettier by adding css like that
diff --git a/src/qt/res/css/light.css b/src/qt/res/css/light.css
index ff9d3f862..560f5448e 100644
--- a/src/qt/res/css/light.css
+++ b/src/qt/res/css/light.css
@@ -1142,6 +1142,15 @@ margin-top:12px;
 margin-left:0px; /* CSS Voodoo - set to -66px to hide default transaction icons */
 }
 
+/* MODAL OVERLAY */
+
+QWidget#bgWidget .QPushButton#warningIcon {
+width:64px;
+height:64px;
+padding:5px;
+background-color:transparent;
+}
+
 /* SEND DIALOG */
 
 QDialog#SendCoinsDialog .QFrame#frameCoinControl { /* Coin Control Section */

to every .css or via adding similar styleSheet in modaloverlay.ui instead.

  1. see connect issues in inline comments

  2. modal overlay doesn't go away when blockchain sync is finished (unlike in bitcoin core) - is it smth that is kept for part3+ or are we missing some older PR here?

@@ -157,6 +157,9 @@ OverviewPage::OverviewPage(const PlatformStyle *platformStyle, QWidget *parent)

// start with displaying the "out of sync" warnings
showOutOfSyncWarning(true);
connect(ui->labelWalletStatus, SIGNAL(clicked()), this, SLOT(handleOutOfSyncWarningClicks()));
connect(ui->labelTransactionsStatus, SIGNAL(clicked()), this, SLOT(handleOutOfSyncWarningClicks()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two labels above are QLabels in Dash and are not clickable, corresponding connects should be removed.

diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp
index dd6bd6b27..08472a17c 100644
--- a/src/qt/overviewpage.cpp
+++ b/src/qt/overviewpage.cpp
@@ -157,9 +157,6 @@ OverviewPage::OverviewPage(const PlatformStyle *platformStyle, QWidget *parent)
 
     // start with displaying the "out of sync" warnings
     showOutOfSyncWarning(true);
-    connect(ui->labelWalletStatus, SIGNAL(clicked()), this, SLOT(handleOutOfSyncWarningClicks()));
-    connect(ui->labelTransactionsStatus, SIGNAL(clicked()), this, SLOT(handleOutOfSyncWarningClicks()));
-
 
     // that's it for litemode
     if(fLiteMode) return;

Some other related connects on overview screen are also useless due to this fact I guess, at least until this

The most important ones are 2 commits that allow to bring up the overlay by clicking on the spinning icon or the progress label.

is implemented.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to figure out what happened here. It looks like in bitcoin these are actually buttons. This change was reverted while merging in 0.12 changes. The PR is #698, the commit is ad5da28
Do you remember why you had to revert those?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we didn't want that warning icon all over the place back then and I doubt we want to change this now too. IMO clicking on the spinning icon or the progress bar/label would make more sense (I would maybe even go with spinning icon only).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You say "change this now too". Do you mean not change it forever or just not now? In case you want to change it someday in the future, I'd suggest to stick with the connect calls as they will then come in handy at that time.

I really like the clicking on the progress bar. When I've first seen the overlay in bitcoin and clicked on hide, it was kind of intuitional that I had to click on the progress bar to get it back. At that time I didn't even notice the small spinning icon on the bottom right. Also, the overlay moves smoothly to the bottom, so it's kind of clear that you have to bring it back up by clicking somewhere near where it vanished.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean "nothing changed, should be no icons here". Can't predict the future ;) but if we ever would think about changing this to buttons for some reason I think we will add required connects at that time. Currently they are just spawning errors in debug.log (and make no sense).

@codablock
Copy link
Author

codablock commented Sep 8, 2017

  1. I added a commit on top, thanks for the CSS snippets. I tried to figure out why this was not a problem in bitcoin, looks like it's because it's a button which in bitcoin has the same background color as the panel, while in Dash the buttons background is blue.
  2. See answer on inline comment
  3. Sorry, didn't realize it behaved different in bitcoin. Gonna check if the upcoming PRs would fix this, if not I'll look into a fix.

@codablock codablock force-pushed the backport_bitcoin_qt_2 branch from 5ea2cb7 to f8f3b0e Compare September 8, 2017 09:53
@codablock
Copy link
Author

The next batch of PRs includes one change where I'm not sure what to do. There is this small connection status icon on the bottom right. In Dash, it was changed to open the peers list when clicking on it, while in bitcoin they changed it to toggle network status (disconnect all peers, go full offline). I'm not sure which feature you see as more valuable. IMHO, clicking on a status icon causing disconnection from the network is kind of awkward, so I'd prefer to stay with the Dash version of behavior.

@UdjinM6
Copy link

UdjinM6 commented Sep 8, 2017

IMHO, clicking on a status icon causing disconnection from the network is kind of awkward

Agree. Though an easy way to quickly disconnect from the network might be a good idea... We could maybe replace instant click-to-action with a context menu for that icon (similar to the way units are selected) with two options:

  • Peers list
  • Go offline/online (could adjust the label text according to current state)

…atus

As both are really just labels, clicking on those is not possible.
This is different in Bitcoin, where these labels are actually buttons.
…h to time based check

Also don't use masternodeSync.IsBlockchainSynced() for now as it won't
report the blockchain being synced before the first block (or other MN
data?) arrives. This would otherwise give the impression that sync is
being stuck.
@codablock
Copy link
Author

Just added 2 commits on top. The first one removes the connect calls and the second one makes the overlay disappear when the blockchain catches up. I had to fall back to the old style time based check from Bitcoin as otherwise the overlay would always appear on startup even though we are pretty close to being synced.

@UdjinM6 UdjinM6 added this to the 12.2 milestone Sep 9, 2017
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested, works as expected

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants