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

Qt: Delay user confirmation of send #8012

Merged
merged 1 commit into from May 10, 2016

Conversation

Projects
None yet
6 participants
@Tyler-Hardin
Contributor

Tyler-Hardin commented May 5, 2016

I made a subclass of QMessageBox that disables the send button in exec() and starts a timer that calls a slot to re-enable it after a configurable delay.

I went with 3s for the delay.

This PR attempts to implement this feature request: #785.

@jonasschnelli jonasschnelli added the GUI label May 6, 2016

@jonasschnelli

View changes

Show outdated Hide outdated src/qt/sendcoinsdialog.cpp
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 6, 2016

Member

Concept ACK 2190e07

Member

laanwj commented May 6, 2016

Concept ACK 2190e07

@jonasschnelli

View changes

Show outdated Hide outdated src/qt/sendcoinsdialog.cpp
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 6, 2016

Member

Somehow I like this but I could imagine confused users during the 3 second timeout ("why the heck is 'Ok' button disabled?!").
What about displaying the remaining seconds? Overkill?

Anyways: this is great work! Thanks.

Member

jonasschnelli commented May 6, 2016

Somehow I like this but I could imagine confused users during the 3 second timeout ("why the heck is 'Ok' button disabled?!").
What about displaying the remaining seconds? Overkill?

Anyways: this is great work! Thanks.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 6, 2016

Member

Needs squashing (at least remove last merge commit).

Member

jonasschnelli commented May 6, 2016

Needs squashing (at least remove last merge commit).

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 6, 2016

Member

displaying the remaining seconds?

Maybe just a clock icon?

Member

MarcoFalke commented May 6, 2016

displaying the remaining seconds?

Maybe just a clock icon?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 6, 2016

Member

Maybe just a clock icon?

Good idea!

Member

jonasschnelli commented May 6, 2016

Maybe just a clock icon?

Good idea!

@Tyler-Hardin

This comment has been minimized.

Show comment
Hide comment
@Tyler-Hardin

Tyler-Hardin May 6, 2016

Contributor

@jonasschnelli @MarcoFalke what about a wait cursor? That would be the easiest to implement by far. Only 2 lines.

Contributor

Tyler-Hardin commented May 6, 2016

@jonasschnelli @MarcoFalke what about a wait cursor? That would be the easiest to implement by far. Only 2 lines.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 6, 2016

Member

@Tyler-Hardin Not sure if wait cursors are still make since today (haven't seem them on my OSX system a while ago IIRC). But it would be better then noting.

Member

jonasschnelli commented May 6, 2016

@Tyler-Hardin Not sure if wait cursors are still make since today (haven't seem them on my OSX system a while ago IIRC). But it would be better then noting.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 6, 2016

Member

Agree with @jonasschnelli. Today, changing the mouse icon is not a thing to do anymore. Instead, the spinner/clock icon is displayed in place (inside the button). This makes clear what exactly is blocking the ui.

Member

MarcoFalke commented May 6, 2016

Agree with @jonasschnelli. Today, changing the mouse icon is not a thing to do anymore. Instead, the spinner/clock icon is displayed in place (inside the button). This makes clear what exactly is blocking the ui.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 6, 2016

Contributor

Concept ACK
Visual indication of the "sleep" would be nice.

Contributor

paveljanik commented May 6, 2016

Concept ACK
Visual indication of the "sleep" would be nice.

@Tyler-Hardin

This comment has been minimized.

Show comment
Hide comment
@Tyler-Hardin

Tyler-Hardin May 6, 2016

Contributor

Squashed.

And I added an animated clock on the yes button.

Contributor

Tyler-Hardin commented May 6, 2016

Squashed.

And I added an animated clock on the yes button.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 6, 2016

Member

Nice. Looks good!

Member

jonasschnelli commented May 6, 2016

Nice. Looks good!

@jonasschnelli

View changes

Show outdated Hide outdated src/qt/sendcoinsdialog.cpp
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 6, 2016

Member

Tested ACK 59e23c63a14d275b6b787256b53f782f0911e8a1
We could argue about the icon.

Member

jonasschnelli commented May 6, 2016

Tested ACK 59e23c63a14d275b6b787256b53f782f0911e8a1
We could argue about the icon.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 6, 2016

Contributor

Can you hide the icon at the end of animation? Or what is the UI style for this?

But anyway, looks good 👍

Contributor

paveljanik commented May 6, 2016

Can you hide the icon at the end of animation? Or what is the UI style for this?

But anyway, looks good 👍

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 6, 2016

Member

Maybe just steal some MIT spinner from elsewhere? https://github.com/snowwlex/QtWaitingSpinner

Putting such a spinner in a /util dir could make it easier to re-use later?

Member

MarcoFalke commented May 6, 2016

Maybe just steal some MIT spinner from elsewhere? https://github.com/snowwlex/QtWaitingSpinner

Putting such a spinner in a /util dir could make it easier to re-use later?

@Tyler-Hardin

This comment has been minimized.

Show comment
Hide comment
@Tyler-Hardin

Tyler-Hardin May 6, 2016

Contributor

@MarcoFalke that would look good, but I have no idea where to start in terms of getting that integrated into the build process.

@paveljanik I'll change it. I had left it in because the button shrinks when it goes away. No style. (I'm guessing you're hinting that I could prevent the shrink with a well designed style. I don't have any idea where to start with that either.)

Contributor

Tyler-Hardin commented May 6, 2016

@MarcoFalke that would look good, but I have no idea where to start in terms of getting that integrated into the build process.

@paveljanik I'll change it. I had left it in because the button shrinks when it goes away. No style. (I'm guessing you're hinting that I could prevent the shrink with a well designed style. I don't have any idea where to start with that either.)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 6, 2016

Member

Lets not bloat the codebase with a progress indicator (should be provided by Qt/OS IMO).
We should try to keep the scope of this PR and add graphical improvements over a different PR (if required).

Member

jonasschnelli commented May 6, 2016

Lets not bloat the codebase with a progress indicator (should be provided by Qt/OS IMO).
We should try to keep the scope of this PR and add graphical improvements over a different PR (if required).

@fcbga

This comment has been minimized.

Show comment
Hide comment
@fcbga

fcbga May 6, 2016

Firefox used to have a similar feature in a dialog that asked the user to confirm they accepted the installation of a plugin from the internet. The "Install" button text was replaced with a timer that counted down while the button was disabled, making it obvious to the user something was going to happen without the need of any other visual indicator.

firefox

fcbga commented May 6, 2016

Firefox used to have a similar feature in a dialog that asked the user to confirm they accepted the installation of a plugin from the internet. The "Install" button text was replaced with a timer that counted down while the button was disabled, making it obvious to the user something was going to happen without the need of any other visual indicator.

firefox

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 7, 2016

Member

Good porint @fcbga . Yes I think a countdown on the button is fine. No strong need for an icon.

Reusing the transaction confirmation icons as a progress indicator is inventive, but it may also be confusing.

Member

laanwj commented May 7, 2016

Good porint @fcbga . Yes I think a countdown on the button is fine. No strong need for an icon.

Reusing the transaction confirmation icons as a progress indicator is inventive, but it may also be confusing.

Qt: Delay user confirmation of send
I made a subclass of QMessageBox that disables the send button in
exec() and starts a timer that calls a slot to re-enable it after a
configurable delay.

It also has a countdown in the send/yes button while it is disabled
to hint to the user why the send button is disabled (and that it is
actually supposed to be disabled).
@Tyler-Hardin

This comment has been minimized.

Show comment
Hide comment
@Tyler-Hardin

Tyler-Hardin May 10, 2016

Contributor

@laanwj, done. Changed to have a text countdown.

Contributor

Tyler-Hardin commented May 10, 2016

@laanwj, done. Changed to have a text countdown.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 10, 2016

Member

Works for me, tested (but not deeply code-reviewed) ACK 3902a29

Member

laanwj commented May 10, 2016

Works for me, tested (but not deeply code-reviewed) ACK 3902a29

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 10, 2016

Member

Tested ACK 3902a29

Member

jonasschnelli commented May 10, 2016

Tested ACK 3902a29

@jonasschnelli jonasschnelli merged commit 3902a29 into bitcoin:master May 10, 2016

1 check passed

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

jonasschnelli added a commit that referenced this pull request May 10, 2016

Merge #8012: Qt: Delay user confirmation of send
3902a29 Qt: Delay user confirmation of send (Tyler Hardin)
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 10, 2016

Member

Looks great. Thanks for sticking with this.

Tested post-merge ACK

Member

MarcoFalke commented May 10, 2016

Looks great. Thanks for sticking with this.

Tested post-merge ACK

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #8012: Qt: Delay user confirmation of send
3902a29 Qt: Delay user confirmation of send (Tyler Hardin)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8012: Qt: Delay user confirmation of send
3902a29 Qt: Delay user confirmation of send (Tyler Hardin)

codablock added a commit to codablock/dash that referenced this pull request Dec 21, 2017

Merge #8012: Qt: Delay user confirmation of send
3902a29 Qt: Delay user confirmation of send (Tyler Hardin)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment