Replace save|restoreWindowGeometry with Qt functions #11335

pull meshcollider wants to merge 1 commits into bitcoin:master from meshcollider:201709_fix_offscreen_2 changing 4 files +15 −36
  1. meshcollider commented at 4:42 AM on September 15, 2017: contributor

    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

  2. fanquake added the label GUI on Sep 15, 2017
  3. meshcollider renamed this:
    [WIP] Replace save|restoreWindowGeometry with Qt functions
    Replace save|restoreWindowGeometry with Qt functions
    on Sep 15, 2017
  4. laanwj commented at 8:43 AM on September 15, 2017: member

    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...

  5. laanwj added the label Needs backport on Sep 15, 2017
  6. laanwj added this to the milestone 0.15.1 on Sep 15, 2017
  7. promag commented at 9:06 AM on September 15, 2017: member

    utACK e70a784.

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

  8. promag commented at 9:18 AM on September 15, 2017: member

    re-utACK after squash.

  9. meshcollider commented at 9:20 AM on September 15, 2017: contributor

    Squashed

  10. promag commented at 9:20 AM on September 15, 2017: member

    utACK 011e31b.

  11. laanwj referenced this in commit f2e0ff687d on Sep 15, 2017
  12. achow101 commented at 1:54 PM on September 15, 2017: member

    ~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.

  13. in src/qt/rpcconsole.cpp:477 in 011e31bd66 outdated 
     474 |  
 475 |  RPCConsole::~RPCConsole()
 476 |  {
 477 | -    GUIUtil::saveWindowGeometry("nRPCConsoleWindow", this);
 478 | +    QSettings settings;
 479 | +    settings.setValue("RPCConsoleWindowGeometry", this->saveGeometry());
    promag commented at 2:15 PM on September 15, 2017:

    Remove this->.

  14. in src/qt/rpcconsole.cpp:432 in 011e31bd66 outdated 
     427 | @@ -428,7 +428,8 @@ RPCConsole::RPCConsole(const PlatformStyle *_platformStyle, QWidget *parent) :
 428 |      consoleFontSize(0)
 429 |  {
 430 |      ui->setupUi(this);
 431 | -    GUIUtil::restoreWindowGeometry("nRPCConsoleWindow", this->size(), this);
 432 | +    QSettings settings;
 433 | +    this->restoreGeometry(settings.value("RPCConsoleWindowGeometry").toByteArray());
    promag commented at 2:16 PM on September 15, 2017:

    No need to this->.

  15. promag commented at 2:18 PM on September 15, 2017: member

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

  16. meshcollider commented at 7:52 AM on September 16, 2017: contributor

    @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.

  17. in src/qt/bitcoingui.cpp:127 in 011e31bd66 outdated 
     122 | @@ -123,7 +123,8 @@ BitcoinGUI::BitcoinGUI(const PlatformStyle *_platformStyle, const NetworkStyle *
 123 |      spinnerFrame(0),
 124 |      platformStyle(_platformStyle)
 125 |  {
 126 | -    GUIUtil::restoreWindowGeometry("nWindow", QSize(850, 550), this);
 127 | +    QSettings settings;
 128 | +    this->restoreGeometry(settings.value("MainWindowGeometry").toByteArray());
    promag commented at 7:57 AM on September 16, 2017:

    Remove this :)

    meshcollider commented at 8:00 AM on September 16, 2017:

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

  18. KanoczTomas commented at 8:12 AM on September 16, 2017: none

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

  19. achow101 commented at 4:02 PM on September 16, 2017: member

    @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.

  20. meshcollider commented at 3:34 AM on September 17, 2017: contributor

    @achow101 could you try on your OS with something like https://github.com/bitcoin/bitcoin/pull/11335/commits/e1166d5dad2a146cea57d0f74a886178fe33f7dd? 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?

  21. achow101 commented at 3:49 AM on September 17, 2017: member

    @MeshCollider e1166d5dad2a146cea57d0f74a886178fe33f7dd centers it for me.

  22. laanwj commented at 3:42 PM on September 19, 2017: member

    utACK after squash

  23. achow101 commented at 7:37 PM on September 20, 2017: member

    utACK d6e8c6687227f96f846210a3c0a8dc6118141528 after squash

  24. meshcollider commented at 8:26 PM on September 20, 2017: contributor

    Squashed

  25. promag commented at 8:32 PM on September 20, 2017: member

    I'll review in a hour or so.

  26. in src/qt/bitcoingui.cpp:126 in ff090c0cf4 outdated 
     122 | @@ -123,7 +123,11 @@ BitcoinGUI::BitcoinGUI(const PlatformStyle *_platformStyle, const NetworkStyle *
 123 |      spinnerFrame(0),
 124 |      platformStyle(_platformStyle)
 125 |  {
 126 | -    GUIUtil::restoreWindowGeometry("nWindow", QSize(850, 550), this);
 127 | +    QSettings settings;
    promag commented at 9:13 AM on September 21, 2017:

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

    meshcollider commented at 10:38 AM on September 21, 2017:

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

  27. in src/qt/bitcoingui.cpp:129 in ff090c0cf4 outdated 
     122 | @@ -123,7 +123,11 @@ BitcoinGUI::BitcoinGUI(const PlatformStyle *_platformStyle, const NetworkStyle *
 123 |      spinnerFrame(0),
 124 |      platformStyle(_platformStyle)
 125 |  {
 126 | -    GUIUtil::restoreWindowGeometry("nWindow", QSize(850, 550), this);
 127 | +    QSettings settings;
 128 | +    if (!restoreGeometry(settings.value("MainWindowGeometry").toByteArray())) {
 129 | +        // Restore failed (perhaps missing setting), center the window
 130 | +        move(QApplication::desktop()->screen()->rect().center() - rect().center());
    promag commented at 9:19 AM on September 21, 2017:

    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()).

    promag commented at 9:25 AM on September 21, 2017:

    Should be something like?

    move(QApplication::desktop()->availableGeometry().center() - geometry().center());
    meshcollider commented at 10:37 AM on September 21, 2017:

    Fixed, thanks :-)

  28. in src/qt/rpcconsole.cpp:481 in ff090c0cf4 outdated 
     478 |  
 479 |  RPCConsole::~RPCConsole()
 480 |  {
 481 | -    GUIUtil::saveWindowGeometry("nRPCConsoleWindow", this);
 482 | +    QSettings settings;
 483 | +    settings.setValue("RPCConsoleWindowGeometry", saveGeometry());
    promag commented at 9:30 AM on September 21, 2017:

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

    promag commented at 9:30 AM on September 21, 2017:

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

    meshcollider commented at 10:36 AM on September 21, 2017:

    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?

    laanwj commented at 11:58 AM on September 22, 2017:

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

  29. Replace save|restoreWindowGeometry with Qt functions 13baf7217b
  30. jonasschnelli commented at 5:34 AM on September 22, 2017: contributor

    utACK 13baf7217bf8394ae02efc376208ae86eac4d0f6 Will test on Windows soon.

  31. jonasschnelli commented at 8:47 PM on September 22, 2017: contributor

    Tested ACK 13baf7217bf8394ae02efc376208ae86eac4d0f6 (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).
  32. luke-jr commented at 8:58 PM on September 22, 2017: member

    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.)

  33. meshcollider commented at 10:58 PM on September 22, 2017: contributor

    @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.

  34. luke-jr commented at 11:34 PM on September 22, 2017: member

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

  35. meshcollider commented at 2:44 AM on September 23, 2017: contributor

    @luke-jr I mentioned that above (https://github.com/bitcoin/bitcoin/pull/11335#issuecomment-330013950 and #11335 (comment)), but it seemed the consensus to center it

  36. laanwj merged this on Sep 25, 2017
  37. laanwj closed this on Sep 25, 2017
  38. laanwj referenced this in commit 8cf88b4aae on Sep 25, 2017
  39. MarcoFalke referenced this in commit 978629efb9 on Oct 3, 2017
  40. MarcoFalke referenced this in commit 07ddaf4725 on Oct 3, 2017
  41. MarcoFalke referenced this in commit 8d13b4298c on Oct 3, 2017
  42. MarcoFalke removed the label Needs backport on Oct 4, 2017
  43. meshcollider deleted the branch on Oct 13, 2017
  44. codablock referenced this in commit b6e7a4b612 on Sep 25, 2019
  45. barrystyle referenced this in commit fbc3c58b1d on Jan 22, 2020
  46. andrewtookay referenced this in commit 0cd763d9d4 on Jul 24, 2021
  47. DrahtBot locked this on Sep 8, 2021
Contributors
meshcolliderlaanwjpromagachow101KanoczTomasjonasschnelliluke-jr
Labels
GUI

Milestone
0.15.2

Linked (view graph)
#11207 [Windows] GUI window can still appear off screen.#11208 Fixing offscreen GUI issue
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me