Skip to content
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

Replace save|restoreWindowGeometry with Qt functions #11335

Merged
merged 1 commit into from Sep 25, 2017

Conversation

Projects
None yet
9 participants
@MeshCollider
Copy link
Member

MeshCollider commented Sep 15, 2017

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

@fanquake fanquake added the GUI label Sep 15, 2017

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 15, 2017

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

@laanwj laanwj added this to the 0.15.1 milestone Sep 15, 2017

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 15, 2017

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 15, 2017

re-utACK after squash.

@MeshCollider MeshCollider force-pushed the MeshCollider:201709_fix_offscreen_2 branch Sep 15, 2017

@MeshCollider

This comment has been minimized.

Copy link
Member Author

MeshCollider commented Sep 15, 2017

Squashed

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 15, 2017

utACK 011e31b.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Sep 15, 2017

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
Copy link
Member

promag left a comment

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

src/qt/rpcconsole.cpp Outdated
consoleFontSize = settings.value(fontSizeSettingsKey, QFontInfo(QFont()).pointSize()).toInt();
clear();
}

RPCConsole::~RPCConsole()
{
GUIUtil::saveWindowGeometry("nRPCConsoleWindow", this);
QSettings settings;
settings.setValue("RPCConsoleWindowGeometry", this->saveGeometry());

This comment has been minimized.

@promag

promag Sep 15, 2017

Member

Remove this->.

src/qt/rpcconsole.cpp Outdated
@@ -428,7 +428,8 @@ RPCConsole::RPCConsole(const PlatformStyle *_platformStyle, QWidget *parent) :
consoleFontSize(0)
{
ui->setupUi(this);
GUIUtil::restoreWindowGeometry("nRPCConsoleWindow", this->size(), this);
QSettings settings;
this->restoreGeometry(settings.value("RPCConsoleWindowGeometry").toByteArray());

This comment has been minimized.

@promag

promag Sep 15, 2017

Member

No need to this->.

@MeshCollider

This comment has been minimized.

Copy link
Member Author

MeshCollider commented Sep 16, 2017

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

src/qt/bitcoingui.cpp Outdated
@@ -123,7 +123,8 @@ BitcoinGUI::BitcoinGUI(const PlatformStyle *_platformStyle, const NetworkStyle *
spinnerFrame(0),
platformStyle(_platformStyle)
{
GUIUtil::restoreWindowGeometry("nWindow", QSize(850, 550), this);
QSettings settings;
this->restoreGeometry(settings.value("MainWindowGeometry").toByteArray());

This comment has been minimized.

@promag

promag Sep 16, 2017

Member

Remove this :)

This comment has been minimized.

@MeshCollider

MeshCollider Sep 16, 2017

Author Member

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

@MeshCollider MeshCollider force-pushed the MeshCollider:201709_fix_offscreen_2 branch Sep 16, 2017

@KanoczTomas

This comment has been minimized.

Copy link

KanoczTomas commented Sep 16, 2017

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

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Sep 16, 2017

@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 MeshCollider force-pushed the MeshCollider:201709_fix_offscreen_2 branch Sep 17, 2017

@MeshCollider

This comment has been minimized.

Copy link
Member Author

MeshCollider commented Sep 17, 2017

@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

This comment has been minimized.

Copy link
Member

achow101 commented Sep 17, 2017

@MeshCollider e1166d5dad2a146cea57d0f74a886178fe33f7dd centers it for me.

@MeshCollider MeshCollider force-pushed the MeshCollider:201709_fix_offscreen_2 branch Sep 17, 2017

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 19, 2017

utACK after squash

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Sep 20, 2017

utACK d6e8c6687227f96f846210a3c0a8dc6118141528 after squash

@MeshCollider MeshCollider force-pushed the MeshCollider:201709_fix_offscreen_2 branch Sep 20, 2017

@MeshCollider

This comment has been minimized.

Copy link
Member Author

MeshCollider commented Sep 20, 2017

Squashed

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 20, 2017

I'll review in a hour or so.

@@ -123,7 +123,11 @@ BitcoinGUI::BitcoinGUI(const PlatformStyle *_platformStyle, const NetworkStyle *
spinnerFrame(0),
platformStyle(_platformStyle)
{
GUIUtil::restoreWindowGeometry("nWindow", QSize(850, 550), this);
QSettings settings;

This comment has been minimized.

@promag

promag Sep 21, 2017

Member

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

This comment has been minimized.

@MeshCollider

MeshCollider Sep 21, 2017

Author Member

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

src/qt/bitcoingui.cpp Outdated
QSettings settings;
if (!restoreGeometry(settings.value("MainWindowGeometry").toByteArray())) {
// Restore failed (perhaps missing setting), center the window
move(QApplication::desktop()->screen()->rect().center() - rect().center());

This comment has been minimized.

@promag

promag Sep 21, 2017

Member

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

This comment has been minimized.

@promag

promag Sep 21, 2017

Member

Should be something like?

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

This comment has been minimized.

@MeshCollider

MeshCollider Sep 21, 2017

Author Member

Fixed, thanks :-)

consoleFontSize = settings.value(fontSizeSettingsKey, QFontInfo(QFont()).pointSize()).toInt();
clear();
}

RPCConsole::~RPCConsole()
{
GUIUtil::saveWindowGeometry("nRPCConsoleWindow", this);
QSettings settings;
settings.setValue("RPCConsoleWindowGeometry", saveGeometry());

This comment has been minimized.

@promag

promag Sep 21, 2017

Member

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

This comment has been minimized.

@promag

promag Sep 21, 2017

Member

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

This comment has been minimized.

@MeshCollider

MeshCollider Sep 21, 2017

Author Member

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?

This comment has been minimized.

@laanwj

laanwj Sep 22, 2017

Member

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

@MeshCollider MeshCollider force-pushed the MeshCollider:201709_fix_offscreen_2 branch to 13baf72 Sep 21, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 22, 2017

utACK 13baf72
Will test on Windows soon.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 22, 2017

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

This comment has been minimized.

Copy link
Member

luke-jr commented Sep 22, 2017

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

This comment has been minimized.

Copy link
Member Author

MeshCollider commented Sep 22, 2017

@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

This comment has been minimized.

Copy link
Member

luke-jr commented Sep 22, 2017

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

@MeshCollider

This comment has been minimized.

Copy link
Member Author

MeshCollider commented Sep 23, 2017

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

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

1 check passed

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

laanwj added a commit that referenced this pull request Sep 25, 2017

Merge #11335: Replace save|restoreWindowGeometry with Qt functions
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

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Oct 3, 2017

@MeshCollider MeshCollider deleted the MeshCollider:201709_fix_offscreen_2 branch Oct 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.