Replace save|restoreWindowGeometry with Qt functions #11335

meshcollider · 2017-09-15T04:42:21Z

Alternative to #11208, closes #11207

According to the Qt documentation, restoreGeometry does all the checks we need, so it would be better to rely on them instead of doing it ourselves.

~~Haven't tested this properly yet, hence the WIP.~~
Gives expected behavior exactly as the other system apps do based on my tests. Only potential issue is the case when the GUI is almost entirely offscreen with only a single strip of pixels, its not really possible to see the GUI, but if you know it's there you can bring it back onscreen with just the mouse. And that's exactly how notepad behaves on Windows so I don't think its a real issue.

This also gives much better behavior when closing a maximized window, currently (0.15.0 release) a maximized window will save the window size on close, and then reopen as a not-maximized but still that size, which is really annoying. This reopens as maximized.

Gitian build here: https://bitcoin.jonasschnelli.ch/build/305

laanwj · 2017-09-15T08:43:37Z

Nice find! So we've spend versions after version trying to iterate and get something working that was already in Qt.

It isn't even remotely new either:

This function was introduced in Qt 4.2.

Concept ACK. As there might be OS-specific magic involved, delegating this to Qt is the right thing to do. And from here on we can just blame upstream if it bugs out again...

promag · 2017-09-15T09:06:33Z

utACK e70a784.

Indeed Qt does more: http://code.qt.io/cgit/qt/qtbase.git/tree/src/widgets/kernel/qwidget.cpp#n7423.

promag · 2017-09-15T09:18:52Z

re-utACK after squash.

meshcollider · 2017-09-15T09:20:00Z

Squashed

promag · 2017-09-15T09:20:51Z

utACK 011e31b.

achow101 · 2017-09-15T13:54:26Z

Currently this will segfault for anyone who does not have the QSetting values of MainWindowGeometry and RPCConsoleWindowGeometry. You will need to check that those values exist and, if they don't, write default values for them.

Nevermind that. That was an unclean working directory during my testing.

The window will open in the bottom righthand corner of the screen if those setting values do not exist. I don't really think that is what we want, we want it to be centered if there is no position saved from previous runs.

promag

@achow101 is right, maybe we should keep GUIUtil::restoreWindowGeometry to center the window when necessary.

promag · 2017-09-15T14:15:20Z

src/qt/rpcconsole.cpp

Remove this->.

promag · 2017-09-15T14:16:21Z

src/qt/rpcconsole.cpp

No need to this->.

meshcollider · 2017-09-16T07:52:26Z

@achow101 on what OS? It opens center screen by default for me

Edit: I've done some digging, and Qt apparently uses the OS default position if there is no setting (reference) which seems sensible anyway, that's completely in line with the idea of letting Qt handle OS specific window behavior.

promag · 2017-09-16T07:57:18Z

src/qt/bitcoingui.cpp

Remove this :)

Done, and the one on line 266 as well which you missed ;)

KanoczTomas · 2017-09-16T08:12:34Z

ah cool, that bug is really annoying! Cool it gets fixe!

achow101 · 2017-09-16T16:02:43Z

@meshcollider I'm using Ubuntu 17.04 it just opens the window in some random corner (it's apparently inconsistent) if the geometry settings don't exist.

meshcollider · 2017-09-17T03:34:02Z

@achow101 could you try on your OS with something like e1166d5? Although if we're going to leave window positioning up to Qt, I'm not sure we should try and interfere to fix where it positions the window, is it really an issue if it decides to not center it on some operating systems?

achow101 · 2017-09-17T03:49:08Z

@meshcollider e1166d5dad2a146cea57d0f74a886178fe33f7dd centers it for me.

laanwj · 2017-09-19T15:42:41Z

utACK after squash

achow101 · 2017-09-20T19:37:11Z

utACK d6e8c6687227f96f846210a3c0a8dc6118141528 after squash

meshcollider · 2017-09-20T20:26:49Z

Squashed

promag · 2017-09-20T20:32:06Z

I'll review in a hour or so.

promag · 2017-09-21T09:13:13Z

src/qt/bitcoingui.cpp

Honestly I think it's better to keep restoreWindowGeometry and move this there.

Same as below, I think its a lot cleaner inline, will change if others agree though 👍

promag · 2017-09-21T09:19:12Z

src/qt/bitcoingui.cpp

I believe this is wrong (don't have 2nd screen to test) since rect() is relative to widget as in QRect(0, 0, width(), height()).

Should be something like?

move(QApplication::desktop()->availableGeometry().center() - geometry().center());

Fixed, thanks :-)

promag · 2017-09-21T09:30:02Z

src/qt/rpcconsole.cpp

Funny fact, in Qt documentation this is done by overriding QWidget::closeEvent. Is the geometry available in the destructor in all platforms?

The same as above, I would keep GUIUtil::saveWindowGeometry, maybe add some logging there too.

I disagree, it's pretty excessive to leave one line of code in its own function, and this is pretty inconsequential behavior so I don't think its worth logging, unless others think it is?

No, IMO, no need to have utility functions for something that is a one-liner. That just makes code harder to follow.

jonasschnelli · 2017-09-22T05:34:50Z

utACK 13baf72
Will test on Windows soon.

jonasschnelli · 2017-09-22T20:47:37Z

Tested ACK 13baf72 (macOS / Window 8.1).
nits:

current window position (in current master) is lost after running Core with this PR.
Unused settings (nWindow, ...) remain in QSettings container (I guess that okay).

luke-jr · 2017-09-22T20:58:56Z

restoreWindowGeometry should probably be retained only temporarily, for compatibility with current saved positions. Although not a huge deal if we lose that, IMO.

Centering the window by default seems like a bad idea, but maybe that discussion belongs elsewhere. (If there's no saved position, the window manager should position it.)

meshcollider · 2017-09-22T22:58:21Z

@luke-jr it seems most WMs center by default (tested on windows and Mac) but achow101 said his Ubuntu system was randomly placing it on screen, hence forced centering (which is the current behaviour) makes sense imo.

And I think losing saved position is a non-issue when upgrading to a new version of the software, it would be an unnecessary mess in the code to check for legacy position/size values, migrate and then remove them.

luke-jr · 2017-09-22T23:34:02Z

@meshcollider If the OS/WM decides to place randomly, that's what it should be...

meshcollider · 2017-09-23T02:44:58Z

@luke-jr I mentioned that above (#11335 (comment) and #11335 (comment)), but it seemed the consensus to center it

13baf72 Replace save|restoreWindowGeometry with Qt functions (MeshCollider) Pull request description: Alternative to #11208, closes #11207 According to the [Qt documentation](https://doc.qt.io/qt-4.8/qwidget.html#restoreGeometry), restoreGeometry does all the checks we need, so it would be better to rely on them instead of doing it ourselves. ~Haven't tested this properly yet, hence the WIP.~ Gives expected behavior exactly as the other system apps do based on my tests. Only potential issue is the case when the GUI is almost entirely offscreen with only a single strip of pixels, its not really possible to see the GUI, but if you know it's there you can bring it back onscreen with just the mouse. And that's exactly how notepad behaves on Windows so I don't think its a real issue. This also gives much better behavior when closing a maximized window, currently (0.15.0 release) a maximized window will save the window size on close, and then reopen as a not-maximized but still that size, which is really annoying. This reopens as maximized. Gitian build here: https://bitcoin.jonasschnelli.ch/build/305 Tree-SHA512: a8bde14793b4316192df1fa2eaaeb32b44d5ebc5219c35252379840056cd737a9fd162625fd715987f275fec8375334ec1ec328dbc671563f084c611a938985c

Github-Pull: bitcoin#11335 Rebased-From: 13baf72

…ions 13baf72 Replace save|restoreWindowGeometry with Qt functions (MeshCollider) Pull request description: Alternative to bitcoin#11208, closes bitcoin#11207 According to the [Qt documentation](https://doc.qt.io/qt-4.8/qwidget.html#restoreGeometry), restoreGeometry does all the checks we need, so it would be better to rely on them instead of doing it ourselves. ~Haven't tested this properly yet, hence the WIP.~ Gives expected behavior exactly as the other system apps do based on my tests. Only potential issue is the case when the GUI is almost entirely offscreen with only a single strip of pixels, its not really possible to see the GUI, but if you know it's there you can bring it back onscreen with just the mouse. And that's exactly how notepad behaves on Windows so I don't think its a real issue. This also gives much better behavior when closing a maximized window, currently (0.15.0 release) a maximized window will save the window size on close, and then reopen as a not-maximized but still that size, which is really annoying. This reopens as maximized. Gitian build here: https://bitcoin.jonasschnelli.ch/build/305 Tree-SHA512: a8bde14793b4316192df1fa2eaaeb32b44d5ebc5219c35252379840056cd737a9fd162625fd715987f275fec8375334ec1ec328dbc671563f084c611a938985c

fanquake added the GUI label Sep 15, 2017

meshcollider changed the title ~~[WIP] Replace save|restoreWindowGeometry with Qt functions~~ Replace save|restoreWindowGeometry with Qt functions Sep 15, 2017

meshcollider mentioned this pull request Sep 15, 2017

Fixing offscreen GUI issue #11208

Closed

laanwj added the Needs backport label Sep 15, 2017

laanwj added this to the 0.15.1 milestone Sep 15, 2017

promag reviewed Sep 15, 2017

View reviewed changes

promag reviewed Sep 16, 2017

View reviewed changes

promag reviewed Sep 21, 2017

View reviewed changes

Replace save|restoreWindowGeometry with Qt functions

13baf72

laanwj merged commit 13baf72 into bitcoin:master Sep 25, 2017

dagurval mentioned this pull request Sep 26, 2017

Maximizing from system tray does not restore window Bitcoin-ABC/bitcoin-abc#91

Closed

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017

Replace save|restoreWindowGeometry with Qt functions

8d13b42

Github-Pull: bitcoin#11335 Rebased-From: 13baf72

maflcko removed the Needs backport label Oct 4, 2017

fanquake mentioned this pull request Oct 7, 2017

RC2 Exits After Initialization #11171

Closed

meshcollider deleted the 201709_fix_offscreen_2 branch October 13, 2017 21:29

hebasto mentioned this pull request Apr 25, 2021

doc: Naming convention for new QSettings setting names bitcoin-core/gui#295

Open

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Replace save|restoreWindowGeometry with Qt functions #11335

Replace save|restoreWindowGeometry with Qt functions #11335

Uh oh!

Conversation

meshcollider commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 15, 2017

Uh oh!

promag commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 15, 2017

Uh oh!

meshcollider commented Sep 15, 2017

Uh oh!

promag commented Sep 15, 2017

Uh oh!

achow101 commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanoczTomas commented Sep 16, 2017

Uh oh!

achow101 commented Sep 16, 2017

Uh oh!

meshcollider commented Sep 17, 2017

Uh oh!

achow101 commented Sep 17, 2017

Uh oh!

laanwj commented Sep 19, 2017

Uh oh!

achow101 commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Sep 20, 2017

Uh oh!

promag commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Sep 22, 2017

Uh oh!

jonasschnelli commented Sep 22, 2017

Uh oh!

luke-jr commented Sep 22, 2017

Uh oh!

meshcollider commented Sep 22, 2017

Uh oh!

luke-jr commented Sep 22, 2017

Uh oh!

meshcollider commented Sep 15, 2017 •

edited

Loading

promag commented Sep 15, 2017 •

edited

Loading

achow101 commented Sep 15, 2017 •

edited

Loading

meshcollider commented Sep 16, 2017 •

edited

Loading

achow101 commented Sep 20, 2017 •

edited

Loading