[Qt] Add out-of-sync modal info layer #8371

Merged
merged 7 commits into from Sep 23, 2016

Projects

None yet
@jonasschnelli
Member

Adresses #8060 and #7235

This change will slide in a semi transparent modal info layer in out-of-sync state resulting in a more prominent warning. Users can hide the modal info layer by pressing on "Hide".
Clicking on the out-of-sync warning icons will re-display the modal info layer.

In the modal info layer, the user can get three new values:

  • amount of blocks left
  • estimated time left until in-sync
  • progress increase per hour

bildschirmfoto 2016-07-19 um 15 53 16

@jonasschnelli jonasschnelli added the GUI label Jul 19, 2016
@instagibbs
Contributor

"Progress increase per hour" is a bit weird sounding to me, maybe "Sync progress per hour"?

@laanwj
Member
laanwj commented Jul 20, 2016

Looks very nice!

@laanwj
Member
laanwj commented Jul 28, 2016
  • Looks like the font color should not depend on the theme?
    overlay
  • Maybe add some more text after "...but this process has not completed yet", to make it more concrete "this means that recent transactions will not be visible, and the balance will not be up-to-date until this process has completed."
@jonasschnelli
Member

Added the second info text and made sure, the font will be blackish (force push).

@jonasschnelli jonasschnelli added this to the 0.14 milestone Jul 28, 2016
@fanquake
Contributor
fanquake commented Aug 8, 2016

Testing this, and clicking hide doesn't seem to do anything?
Also, If your opening a close-to, or recently synced wallet, should the modal disappear automatically once it reaches 100%?

OS X screenshots:
modal
full-wallet

Also, this looks like a new compiler warning.

  CXX      qt/qt_libbitcoinqt_a-bitcoingui.o
qt/bitcoingui.cpp:118:5: warning: field 'spinnerFrame' will be initialized after field 'modalOverlay' [-Wreorder]
    spinnerFrame(0),
    ^
1 warning generated.
@jonasschnelli
Member

@fanquake: It should hide automatically if you are in sync,.. but only, If you haven't opened it manually (by pressing the warning icons).
Not sure if it should also hide in that case, but probably.

@Joukehofman

Look great! Maybe add to the warning text that it's also not possible to send bitcoins?

@jonasschnelli
Member

Fixed warning. Added a short text passage "Spending bitcoins is not possible during that phase!".

@sipa
Member
sipa commented Aug 26, 2016

Does the "This process is not complete yet" disappear when 100% is reached, but the window is still open?

jonasschnelli added some commits Jul 19, 2016
@jonasschnelli jonasschnelli [Refactor] refactor function that forms human readable text out of a …
…timeoffset
0904c3c
@jonasschnelli jonasschnelli [Qt] make Out-Of-Sync warning icon clickable bd44a04
@jonasschnelli jonasschnelli [Qt] Always pass the numBlocksChanged signal for headers tip changed a001f18
@jonasschnelli jonasschnelli [Qt] ClientModel add method to get the height of the header chain e47052f
@jonasschnelli
Member

@sipa: yes. It will (https://github.com/bitcoin/bitcoin/pull/8371/files#diff-0db7dd184df07a48c307ccc182021a68R776). But only if you not have manually opened the "window" by clicking on the alert icons (then it will stay intentionally).

@sipa
Member
sipa commented Aug 26, 2016
@jonasschnelli
Member

Ah. I get your point. Maybe force closing then would make sense (or changing the text in the upper section [more complicate]).

@sipa
Member
sipa commented Aug 26, 2016
@MarcoFalke
Member

I think force closing is fine.

Agree

@jonasschnelli jonasschnelli [Qt] add out-of-sync modal info layer e3245b4
@jonasschnelli
Member

Updated so that the layer/window does force close when the we detect in-sync state.

@sipa
Member
sipa commented Aug 26, 2016

The "number of blocks left" goes up and down for a while (presumably because the headers aren't processed yet?)... if the tip header is too far in the past, it may be better to say "unknown" there.

@fanquake
Contributor

Updated OS X screenshot, the hiding issue seems to be fixed now.
modal

@luke-jr
Member
luke-jr commented Sep 10, 2016

Looks like the font color should not depend on the theme?

Rather, the background colour should depend on the theme also? Why is this overriding the theme?

@flack
flack commented Sep 12, 2016

Shouldn't this dialog also have a headline of some sort (e.g. "Wallet out of sync")?

Because if I look at the screenshot, I only see the progress data, and the text starts with "The displayed information may be out of date", but in fact, it's not the currently displayed data (progress information) that is out of date, but the stuff in the wallet, which is obscured by the overlay.

@jonasschnelli
Member

@luke-jr: I think creating nice visualizations is difficult with the presumption to only use theme based colors.

@flack: Indeed. The term "the displayed information... " can look like its referred to the data below (in the same overlay). I have plans to overhaul this PR soon.

@jonasschnelli jonasschnelli [Qt] only update "amount of blocks left" when the header chain is in-…
…sync
d8b062e
@jonasschnelli
Member

Updated with recommendation from @sipa to hide the remaining blocks during headers-first during IBD.

bildschirmfoto 2016-09-13 um 18 01 34

@MarcoFalke

Tested-only ACK. Found a few style nits. (You can fix them in a separate fixup commit)

ACK d8b062e

src/qt/modaloverlay.cpp
+ // show progress speed if we have more then one sample
+ if (blockProcessTime.size() >= 2)
+ {
+ // try to get the window from the last 500 seconds or at least 10 samples
@MarcoFalke
MarcoFalke Sep 21, 2016 Member

Where does it say 10 in the source code?

src/qt/modaloverlay.cpp
+ ui->expectedTimeLeft->setText(GUIUtil::formateNiceTimeOffset(remainingMSecs/1000.0));
+
+ // keep maximal 5000 samples
+ static int maxSamples = 5000;
@MarcoFalke
MarcoFalke Sep 21, 2016 Member

nit

static const int MAX_SAMPLES = 5000;
src/qt/walletframe.h
@@ -38,6 +38,10 @@ class WalletFrame : public QFrame
void showOutOfSyncWarning(bool fShow);
+Q_SIGNALS:
+ /** Notify that the user has requested more information about the out-of-sync warning */
+ void requestedOfSyncWarningInfo();
@MarcoFalke
MarcoFalke Sep 21, 2016 Member

nit: I don't think this is proper English. Idk, get rid of the Of?

src/qt/bitcoingui.cpp
+ modalOverlay->setKnownBestHeight(clientModel->getHeaderTipHeight(), QDateTime::fromTime_t(clientModel->getHeaderTipTime()));
+ }
+ else
+ modalOverlay->tipUpdate(count, blockDate, nVerificationProgress);
@MarcoFalke
MarcoFalke Sep 21, 2016 Member

style nit: brackets are "required" for the else when they are used for the if.

@MarcoFalke
Member

Looks good to merge

screenshot from 2016-09-21 10-20-23

@jonasschnelli
Member

Fixed nits reported by @MarcoFalke.
@MarcoFalke: can you re-ack?

@luke-jr
Member
luke-jr commented Sep 21, 2016

@jonasschnelli On the contrary, violating the theme contradicts the goal of nice visualisations.

@jonasschnelli
Member

@luke-jr: I think adding a black overlay with white text does not directly violates the theme. It just will result in a more controllable look and feel.

@luke-jr
Member
luke-jr commented Sep 21, 2016

Software controlling look-and-feel means the user's chosen look-and-feel is being ignored. Presumably the user has chosen them because they like it, so violating them simply won't be nice.

@jonasschnelli
Member

I think themes are there for a reason.
Using the theme colors might result in loosing the optical effect which is there for a reason. The back background with the white info layer should imply that the informations within the layer are on a different level of importance.

But happy if someone likes to try migrating this to the theme colors.

@luke-jr
Member
luke-jr commented Sep 21, 2016

(BTW, I don't mean to imply theme concerns need to interfere in merging this. By all means, feel free. We can work on theming later.)

@gmaxwell
Member

Too much text, esp too much bolded text. I automatically don't read the top part (though I did in the original version). Also "Spending bitcoins is not possible during that phase!" is not true.

@sipa
Member
sipa commented Sep 23, 2016
@gmaxwell
Member

@sipa Yes, but you can spend anything you can see... This is a common point of confusion already. And in the common case where you had bitcoin off for the last month, but received no payment since then, you're good to go for sending.

@jonasschnelli
Member

@gmaxwell: would you be willing providing a better text?

I agree that "spending Bitcoins is not possible" is not true and can lead to confusion, especially if one catches up just a couple of days (the info-layer will also be shown then).

src/qt/forms/modaloverlay.ui
+ </font>
+ </property>
+ <property name="text">
+ <string>This means that recent transactions will not be visible, and the balance will not be up-to-date until this process has completed. Spending bitcoins is not possible during that phase!</string>
@MarcoFalke
MarcoFalke Sep 23, 2016 Member

Maybe

s/possible/recommended/

?

@jonasschnelli
jonasschnelli Sep 23, 2016 Member

Or what about [...]Spending bitcoins may not be possible[...]?

@MarcoFalke
MarcoFalke Sep 23, 2016 Member

Fine, I think either is better.

@jonasschnelli
Member

Force pushed a text- and bold-section-change.
This is how it looks now:
bildschirmfoto 2016-09-23 um 15 31 38

@MarcoFalke
Member

Are you sure you pushed the text change?

@jonasschnelli jonasschnelli [Qt] modalinfolayer: removed unused comments, renamed signal, code st…
…yle overhaul
08827df
@jonasschnelli
Member

Pushed now..

@MarcoFalke
Member

ACK 08827df

@jonasschnelli jonasschnelli merged commit 08827df into bitcoin:master Sep 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonasschnelli jonasschnelli added a commit that referenced this pull request Sep 23, 2016
@jonasschnelli jonasschnelli Merge #8371: [Qt] Add out-of-sync modal info layer
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)
24f72e9
@@ -234,7 +250,7 @@ static void BlockTipChanged(ClientModel *clientmodel, bool initialSync, const CB
int64_t& nLastUpdateNotification = fHeader ? nLastHeaderTipUpdateNotification : nLastBlockTipUpdateNotification;
// if we are in-sync, update the UI regardless of last update time
- if (!initialSync || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) {
+ if (fHeader || !initialSync || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) {
@MarcoFalke
MarcoFalke Sep 25, 2016 edited Member

Why is this needed?

Oh, nvm. It is needed so a header update is not accidentally lost. Still, this makes the gui unresponsive when -reindex is used...

@Xekyo
Contributor
Xekyo commented Sep 26, 2016 edited

Instead of "this process" and "that phase" I would repeat "synchronization":

The displayed information may be out of date.
Your wallet automatically synchronizes with the Bitcoin network while connected, but synchronization has not completed yet. Recent transactions will not be visible until your wallet catches up to the network. Your balance will update as transactions are processed.
Spending bitcoins may not be possible until synchronization has finished.

@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Refactor] refactor function that forms human readable text out of a …
…timeoffset

Github-Pull: #8371
Rebased-From: 0904c3c
cb80e00
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Qt] make Out-Of-Sync warning icon clickable
Github-Pull: #8371
Rebased-From: bd44a04
8e325b4
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Qt] Always pass the numBlocksChanged signal for headers tip changed
Github-Pull: #8371
Rebased-From: a001f18
4f4d641
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Qt] ClientModel add method to get the height of the header chain
Github-Pull: #8371
Rebased-From: e47052f
88c4ec6
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Qt] add out-of-sync modal info layer
Github-Pull: #8371
Rebased-From: e3245b4
4d7ccc5
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Qt] only update "amount of blocks left" when the header chain is in-…
…sync

Github-Pull: #8371
Rebased-From: d8b062e
d1bc374
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Qt] modalinfolayer: removed unused comments, renamed signal, code st…
…yle overhaul

Github-Pull: #8371
Rebased-From: 08827df
752f57a
This was referenced Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment