/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixing offscreen GUI issue #11208

Closed
wants to merge 2 commits into
from
+3 −3

Conversation

Reviewers

@jonasschnelli jonasschnelli

@promag promag

@luke-jr luke-jr

Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
No milestone
10 participants
@MeshCollider @MarcoFalke @jtimon @achow101 @AllanDoensen @sipa @laanwj @jonasschnelli @luke-jr @promag
@MeshCollider
Member

MeshCollider commented Aug 31, 2017 •
Edited 1 time

Fixes #11207

This still obviously needs to be confirmed as the cause of the issues in question, this is the current assumption

Edit: c.f. #11335

@MarcoFalke MarcoFalke added the GUI label Aug 31, 2017

@MarcoFalke MarcoFalke added this to the 0.15.0 milestone Aug 31, 2017

@MarcoFalke
Member

MarcoFalke commented Aug 31, 2017

Tagged for backport in case there will be another rc

@MarcoFalke MarcoFalke added the Needs backport label Aug 31, 2017

@jtimon
Member

jtimon commented Aug 31, 2017

utACK 7765030
src/qt/guiutil.cpp
- {
- QRect screen = QApplication::desktop()->screenGeometry();
+
+ QRect screen = QApplication::desktop()->screenGeometry();
@promag

promag Aug 31, 2017

Contributor

Use availableGeometry().

@MeshCollider

MeshCollider Sep 1, 2017

Member

Yep sounds good. I'll fix it to availableGeometry(screen_number) too now that I think about it. Will push it as a separate commit for review before squashing.

src/qt/guiutil.cpp
- QRect screen = QApplication::desktop()->screenGeometry();
+
+ QRect screen = QApplication::desktop()->screenGeometry();
+ if ((!pos.x() && !pos.y()) || pos.x() > screen.width() || pos.y() > screen.height() || (QApplication::desktop()->screenNumber(parent) == -1)) {
@promag

promag Aug 31, 2017

Contributor

How about

if (!screen.contains(parent->geometry()) || QApplication::desktop()->screenNumber(parent) == -1) {

And I think the first condition is enough, not sure though.

@MeshCollider

MeshCollider Sep 1, 2017

Member

parent->geometry() does not include the window frame, so perhaps frameGeometry()? But yeah I guess a check for full containment makes sense rather than just the point pos()

@achow101 achow101 referenced this pull request Aug 31, 2017

Closed

[Windows] GUI window can still appear off screen. #11207

@achow101
Member

achow101 commented Aug 31, 2017

ACK 7765030

Tested on Windows 10, confirms that this fixes the issue I identified in #11207. However #11171 sounds like it is a different problem.
@AllanDoensen
Contributor

AllanDoensen commented Sep 1, 2017

I believe this was fixed here : #10156
src/qt/guiutil.cpp
- if ((!pos.x() && !pos.y()) || (QApplication::desktop()->screenNumber(parent) == -1))
- {
- QRect screen = QApplication::desktop()->screenGeometry();
+
@promag

promag Sep 1, 2017

Contributor

Remove whitespace.

src/qt/guiutil.cpp
- QRect screen = QApplication::desktop()->screenGeometry();
+
+ int screen_number = QApplication::desktop()->screenNumber(parent);
+ QRect screen = QApplication::desktop()->availableGeometry(screen_number);
@promag

promag Sep 1, 2017 •
Edited 1 time

Contributor

What if the result of availableGeometry() when screen_number = -1?

@MeshCollider

MeshCollider Sep 1, 2017

Member

Do you mean -1? If the parameter is -1, it returns the default screen

@promag

promag Sep 1, 2017

Contributor

Fixed above comment.

src/qt/guiutil.cpp
+
+ int screen_number = QApplication::desktop()->screenNumber(parent);
+ QRect screen = QApplication::desktop()->availableGeometry(screen_number);
+ if ((!pos.x() && !pos.y()) || screen_number == -1 || !screen.contains(parent->frameGeometry())) {
@promag

promag Sep 1, 2017

Contributor

If I understand correctly, !pos.x() && !pos.y() should be replaced with !settings.contains(strSettings + "Pos").

@MeshCollider

MeshCollider Sep 1, 2017

Member

Safer to check after converting it to a point, just to make sure conversion worked properly?

@promag

promag Sep 1, 2017

Contributor

But what if the window position is QPoint(0, 0)?

@luke-jr

luke-jr approved these changes Sep 2, 2017
View changes

utACK

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 2, 2017

@MeshCollider @luke-jr
Fixing offscreen GUI issue 
Github-Pull: #11208
Rebased-From: 7765030
80b0f37

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 2, 2017

@MeshCollider @luke-jr
Check screen contains window when restoring geometry 
Github-Pull: #11208
Rebased-From: 6067244
347a7ef
src/qt/guiutil.cpp
- QRect screen = QApplication::desktop()->screenGeometry();
+ int screen_number = QApplication::desktop()->screenNumber(parent);
+ QRect screen = QApplication::desktop()->availableGeometry(screen_number);
+ if ((!pos.x() && !pos.y()) || screen_number == -1 || !screen.contains(parent->frameGeometry())) {
@jonasschnelli

jonasschnelli Sep 3, 2017

Member

Does this allow a window that is halfway moved out of the screen (I think we should allow this)?

@MeshCollider

MeshCollider Sep 3, 2017

Member

No I don't think it does, but I thought about that and the expected behaviour of most software is that it launches within the screen even if you moved it partially out last time?

@sipa
Owner

sipa commented Sep 3, 2017

utACK
@laanwj
Owner

laanwj commented Sep 5, 2017

I believe this was fixed here : #10156

I was about to remark this - there have been a few PRs before in the last few years 'fixing' this issue, but it keeps returning (or wasn't fixed properly after all). So normally we would make sure of this by unit tests but I don't think that's an option here due to the GUI-ness. So I suggest creating a manual testplan before/after, so we can verify that this bug is squashed.

@MarcoFalke MarcoFalke modified the milestones: 0.15.1, 0.15.0 Sep 5, 2017

@jonasschnelli
Member

jonasschnelli commented Sep 5, 2017

Just tested on macOS and windows 8.1 (VM):
Current master OSX: if I move the window outside of the boundary, it will relocate the window to the maximum boundaries without overlapping the visible area.
With this PR OX: overlapping the boundaries will always lead to reopen it in the screen center.
(I think this is acceptable, but slightly different to the current state).

Could not reproduce the issue on Windows 8.1 (did run on a very large screen, closes Bitcoin-Qt while having the main window on the right bottom, reduce screen by times 4, re-started Qt and had the window visible)

@jonasschnelli jonasschnelli referenced this pull request Sep 5, 2017

Closed

[0.15] Mention offscreen issue in release notes #11245

@laanwj
Owner

laanwj commented Sep 6, 2017

So it's unclear whether this is actually needed. Removing the milestone.

@laanwj laanwj removed the Needs backport label Sep 6, 2017

@laanwj laanwj removed this from the 0.15.1 milestone Sep 6, 2017

@MeshCollider MeshCollider referenced this pull request Sep 15, 2017

Merged

Replace save|restoreWindowGeometry with Qt functions #11335

@laanwj
Owner

laanwj commented Sep 15, 2017

Closing in favor of #11335

@laanwj laanwj closed this Sep 15, 2017

laanwj added a commit to laanwj/bitcoin that referenced this pull request Sep 15, 2017

@laanwj
Merge #11335: Replace save|restoreWindowGeometry with Qt functions 
e70a784 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: ef68e19b7c79a919389b74c7e12bc633c882906981831d9f0b6461e33cd1ba966edf500ae5e906c896cee4e1fe150fdf7e36900d63e9895d9a0cf9c5146edd18
f2e0ff6

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

@laanwj
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
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
8cf88b4

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment