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

Disallow duplicate windows. #6594

Merged
merged 1 commit into from Sep 8, 2015

Conversation

Projects
None yet
6 participants
@casey
Contributor

casey commented Aug 27, 2015

Fixes #3569.

This change disallows the creation of multiple instances of the command line options, used send addresses, and used receive addresses windows.

@laanwj laanwj added the GUI label Aug 28, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Aug 28, 2015

Member

Nice!
Tested a bit.

Two points:

  • why is the "Sign message" dialog is modal and the "Sending Addresses" and "Receiving Addresses" not? (indirectly related to this PR)
  • This PR works, but it would be nice, if the window would get the focus when a user selects "Sending Addresses (or "Receiving Addresses") if the window is already open in background.
Member

jonasschnelli commented Aug 28, 2015

Nice!
Tested a bit.

Two points:

  • why is the "Sign message" dialog is modal and the "Sending Addresses" and "Receiving Addresses" not? (indirectly related to this PR)
  • This PR works, but it would be nice, if the window would get the focus when a user selects "Sending Addresses (or "Receiving Addresses") if the window is already open in background.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 31, 2015

Member

Concept ack.

Member

laanwj commented Aug 31, 2015

Concept ack.

@casey

This comment has been minimized.

Show comment
Hide comment
@casey

casey Aug 31, 2015

Contributor

I tried adding calls to widget->activateWindow() and widget->raise(), but my weirdo window manager (xmonad) isn't respecting them so I can't test. @jonasschnelli, can you pull casey:duplicate-windows-foreground and see if that fixes it?

As far as modal vs non-modal, I can't really come up with a compelling argument for it either way. Do users leave up the sign/verify dialogs while they do other thing?

Contributor

casey commented Aug 31, 2015

I tried adding calls to widget->activateWindow() and widget->raise(), but my weirdo window manager (xmonad) isn't respecting them so I can't test. @jonasschnelli, can you pull casey:duplicate-windows-foreground and see if that fixes it?

As far as modal vs non-modal, I can't really come up with a compelling argument for it either way. Do users leave up the sign/verify dialogs while they do other thing?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 1, 2015

Member

@casey I also don't have an strong opinion on modal/non-modal. Just tested on Ubuntu and OSX and the focus issue is still unsolved.
Could you not just call activateWindow() (http://doc.qt.io/qt-4.8/qwidget.html#activateWindow). IIRC I once have implemented that also in https://github.com/bitcoin/bitcoin/blob/master/src/qt/macdockiconhandler.mm#L129.

Member

jonasschnelli commented Sep 1, 2015

@casey I also don't have an strong opinion on modal/non-modal. Just tested on Ubuntu and OSX and the focus issue is still unsolved.
Could you not just call activateWindow() (http://doc.qt.io/qt-4.8/qwidget.html#activateWindow). IIRC I once have implemented that also in https://github.com/bitcoin/bitcoin/blob/master/src/qt/macdockiconhandler.mm#L129.

@casey

This comment has been minimized.

Show comment
Hide comment
@casey

casey Sep 1, 2015

Contributor

@jonasschnelli Which version of ubuntu and which desktop environment are you testing with? When I test with ubuntu vivid with unity, I can't actually get the main window to the foreground when the "sending addresses" or "receiving addresses" windows are up, they stay in the foreground, although I can focus the main window.

Contributor

casey commented Sep 1, 2015

@jonasschnelli Which version of ubuntu and which desktop environment are you testing with? When I test with ubuntu vivid with unity, I can't actually get the main window to the foreground when the "sending addresses" or "receiving addresses" windows are up, they stay in the foreground, although I can focus the main window.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 2, 2015

Member

@casey: Right. On Ubuntu the "sending addresses" or "receiving addresses" windows are always in front. On OSX (and probably also on Windows), the window does not keep in front always. There it would be good if the window gets the focus if one selects "sending addresses" or "receiving addresses" again from the menu. On Ubuntu focusing the window (even if it is always in front but un-focused) would also be good.

Member

jonasschnelli commented Sep 2, 2015

@casey: Right. On Ubuntu the "sending addresses" or "receiving addresses" windows are always in front. On OSX (and probably also on Windows), the window does not keep in front always. There it would be good if the window gets the focus if one selects "sending addresses" or "receiving addresses" again from the menu. On Ubuntu focusing the window (even if it is always in front but un-focused) would also be good.

@casey

This comment has been minimized.

Show comment
Hide comment
@casey

casey Sep 3, 2015

Contributor

@jonasschnelli I still can't get the windows to come to the foreground on all platforms; even with calls to activeateWindow(), behavior is still inconsistent.

What about just making all three dialogues modal, so that they're just always in the foreground, and users have to dismiss them to continue? This would make everything consistent.

Contributor

casey commented Sep 3, 2015

@jonasschnelli I still can't get the windows to come to the foreground on all platforms; even with calls to activeateWindow(), behavior is still inconsistent.

What about just making all three dialogues modal, so that they're just always in the foreground, and users have to dismiss them to continue? This would make everything consistent.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 3, 2015

Member

I think the "* addresses"-windows are non-modal so the user can easily copy more than one address into the "Send"-tab. But then there is also the "Choose previously used address"-button in the "Send"-tab providing an alternative to copy-paste sending addresses... So, making all three dialogues modal sounds ok to me.

Member

MarcoFalke commented Sep 3, 2015

I think the "* addresses"-windows are non-modal so the user can easily copy more than one address into the "Send"-tab. But then there is also the "Choose previously used address"-button in the "Send"-tab providing an alternative to copy-paste sending addresses... So, making all three dialogues modal sounds ok to me.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 3, 2015

Member

I personally prefer being able to open multiple windows at a time (not of the same type obviously - I agree with this pull). Modal dialogs are meant for when some action has to be completed before another, but if there is no sequential control flow, there is no reason to not allow interacting with them at the same time.

Member

laanwj commented Sep 3, 2015

I personally prefer being able to open multiple windows at a time (not of the same type obviously - I agree with this pull). Modal dialogs are meant for when some action has to be completed before another, but if there is no sequential control flow, there is no reason to not allow interacting with them at the same time.

@casey

This comment has been minimized.

Show comment
Hide comment
@casey

casey Sep 3, 2015

Contributor

In that case I think we should merge the PR as is. I haven't been able to get the right behavior in a cross platform way, no matter how many calls to activateWindow() and raise() that I insert :-P

Contributor

casey commented Sep 3, 2015

In that case I think we should merge the PR as is. I haven't been able to get the right behavior in a cross platform way, no matter how many calls to activateWindow() and raise() that I insert :-P

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 4, 2015

Member

Tested ACK on merging this as it is.
The non-focus while reselecting over the menu is a nitpick.

Member

jonasschnelli commented Sep 4, 2015

Tested ACK on merging this as it is.
The non-focus while reselecting over the menu is a nitpick.

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Sep 4, 2015

Member

utACK

On Friday, September 4, 2015, Jonas Schnelli notifications@github.com
wrote:

ACK on merging this as it is.
The non-focus while reselecting over the menu is a nitpick.


Reply to this email directly or view it on GitHub
#6594 (comment).

Member

fanquake commented Sep 4, 2015

utACK

On Friday, September 4, 2015, Jonas Schnelli notifications@github.com
wrote:

ACK on merging this as it is.
The non-focus while reselecting over the menu is a nitpick.


Reply to this email directly or view it on GitHub
#6594 (comment).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 4, 2015

Member

@casey: This commit (jonasschnelli@a272c80) would fix the focus issue. Mind cherry pick this commit?

Member

jonasschnelli commented Sep 4, 2015

@casey: This commit (jonasschnelli@a272c80) would fix the focus issue. Mind cherry pick this commit?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 4, 2015

Member

This shouldn't include the commit "rpc-tests: re-enable rpc-tests for Windows".
(nor the two commits before that)

Member

laanwj commented Sep 4, 2015

This shouldn't include the commit "rpc-tests: re-enable rpc-tests for Windows".
(nor the two commits before that)

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 5, 2015

Member

Tested ACK (5ffaaba, Fedora)

@jonasschnelli If you are happy with 5ffaaba maybe you could also create a windows build at https://builds.jonasschnelli.ch/pulls/6594/ ?

Member

MarcoFalke commented Sep 5, 2015

Tested ACK (5ffaaba, Fedora)

@jonasschnelli If you are happy with 5ffaaba maybe you could also create a windows build at https://builds.jonasschnelli.ch/pulls/6594/ ?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 5, 2015

Member

@MarcoFalke: just triggered a gitian build of this PR: https://builds.jonasschnelli.ch/pulls/6594/

Member

jonasschnelli commented Sep 5, 2015

@MarcoFalke: just triggered a gitian build of this PR: https://builds.jonasschnelli.ch/pulls/6594/

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 6, 2015

Member

Great, thanks for the build!

Tested ACK (Win 7)

Member

MarcoFalke commented Sep 6, 2015

Great, thanks for the build!

Tested ACK (Win 7)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 7, 2015

Member

Tested ACK.

Member

jonasschnelli commented Sep 7, 2015

Tested ACK.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Sep 7, 2015

Member

@casey just a little github trick, if a PR fixed another issue, you can put "fixes #3569" in your commit message and it will close the ticket when this gets the PR gets merged.

Member

btcdrak commented Sep 7, 2015

@casey just a little github trick, if a PR fixed another issue, you can put "fixes #3569" in your commit message and it will close the ticket when this gets the PR gets merged.

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Sep 7, 2015

Member

@btcdrak I'm pretty sure that also works with when you have it in the PR description.

Member

fanquake commented Sep 7, 2015

@btcdrak I'm pretty sure that also works with when you have it in the PR description.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Sep 7, 2015

Member

@fanquake Maybe, they are always adding new features but it's not documented if it does. https://help.github.com/articles/closing-issues-via-commit-messages/

Member

btcdrak commented Sep 7, 2015

@fanquake Maybe, they are always adding new features but it's not documented if it does. https://help.github.com/articles/closing-issues-via-commit-messages/

@laanwj laanwj merged commit 5ffaaba into bitcoin:master Sep 8, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Sep 8, 2015

Merge pull request #6594
5ffaaba Disallow duplicate windows. (Casey Rodarmor)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment