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

Fix for issues with startup and multiple monitors on windows. #10156

Closed
wants to merge 38 commits into
from

Conversation

Projects
None yet
8 participants
@AllanDoensen
Contributor

AllanDoensen commented Apr 5, 2017

I noticed a small bug in bitcoin-qt with multiple physical displays. This bug affects me because I use a Windows 10 laptop that sometimes uses a second display.

I can only reproduce this issue on windows 10. I could not reproduce this issue on Ubuntu 16.04 LTS with dual displays running in a VirtualBox VM.

This is a pull request for a fix for this issue.

To demonstrate the bug:

  1. Install Bitcoin-qt on a system with multiple monitors (i.e. a laptop attached to a second screen). I have only reproduced this issue on windows 10 with ‘extend these displays’ configured.
  2. Start bitcoin-qt and place it on the second screen.
  3. Close bitcoin-qt (it will cache it’s position on the second screen).
  4. Remove (physically unplug) the second screen.
  5. Start bitcoin-qt.
  6. You will find that bitcoin-qt starts in the cached second screen position and cannot be found on the only active screen. It just disappears. You need to realise that it is off screen and then do some fancy control keys to get it to come back to the primary display.

This is not a critical bug, but when it happens the user experience is not good, hence this fix.

As for the code changes….The position of bitcoin-qt is saved into QSettings on exit. When bitcoin-qt restarts it grabs this position and places itself at that location. However it never tests if this position actually still exists. So I have added a test that if the location does not exist on any of the current screens then it re-positions it to the default location.

I also use abs() when calculating the center position. This is because there is the potential for the size of the application to be larger then the size of the current screen, abs() should handle that corner case.

I have tested this change on Windows 10 & Ubuntu 16.04 LTS.

jonasschnelli and others added some commits Feb 2, 2017

Replace includes of boost/filesystem.h with fs.h
This is step one in abstracting the use of boost::filesystem.
Replace uses of boost::filesystem with fs
Step two in abstracting away boost::filesystem.

To repeat this, simply run:
```
git ls-files \*.cpp \*.h | xargs sed -i 's/boost::filesystem/fs/g'
```
Use fsbridge for fopen and freopen
Abstracts away how a path is opened to a `FILE*`.

Reduces the number of places where path is converted to a string
for anything else but printing.
Remove `namespace fs=fs`
Having these inside functions is silly and redundant now.
doc: Remove version numbers from READMEs
If we want to keep these numbers, could generate them using autoconf.
But this seems unnecessary.

@fanquake fanquake added the GUI label Apr 5, 2017

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 5, 2017

Member

Makes sense.
Code looks good (re-sets the Widget to main screen center if the larger portion of Widget is not on a screen).
utACK 1896818

Member

jonasschnelli commented Apr 5, 2017

Makes sense.
Code looks good (re-sets the Widget to main screen center if the larger portion of Widget is not on a screen).
utACK 1896818

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 5, 2017

Member

This doesn't just affect windows but any setup with variable number monitors. Thanks for the fix.
(I think there's an reported issue for this open somewhere but can't find it)

Member

laanwj commented Apr 5, 2017

This doesn't just affect windows but any setup with variable number monitors. Thanks for the fix.
(I think there's an reported issue for this open somewhere but can't find it)

laanwj added some commits Apr 5, 2017

Merge #10154: init: Remove redundant logging code
faafa80 init: Remove redundant logging code (MarcoFalke)

Tree-SHA512: 5ad0e9aba0e25a36025dd4ee5e5fddd2c0039f95bafd0f33300ea59e2f9bba807da6a1a8b4311d6aad5a360b99163edf4a4f161cb13f0f38580d8d6b504c94ad
Merge #10151: [logging] initialize flag variable to 0 (and continue i…
…f GetLogCategory() fails)

cd7f394 initialize flag variable to 0 (and continue if GetLogCategory() fails) (John Newbery)

Tree-SHA512: d0f2653bd0e71ed763220cb08d3a5335c5bdfe2f54ff7f9302d97f3265d7aa7f57606fe416a61aaac1535dbb046d0fb40a61f5a9d5cf234b042268e00ee7679d
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 5, 2017

Member

Does this apply to 0.14?

Member

sipa commented Apr 5, 2017

Does this apply to 0.14?

@AllanDoensen

This comment has been minimized.

Show comment
Hide comment
@AllanDoensen

AllanDoensen Apr 5, 2017

Contributor

@sipa This applies to 0.14. This bug has existed for many years and is in all bitcoin & alt-coins that I am aware of.

Contributor

AllanDoensen commented Apr 5, 2017

@sipa This applies to 0.14. This bug has existed for many years and is in all bitcoin & alt-coins that I am aware of.

@laanwj laanwj changed the title from Fix for issues with startup and mutiple monitors on windows. to Fix for issues with startup and multiple monitors on windows. Apr 6, 2017

laanwj and others added some commits Apr 5, 2017

build: Remove duplicate version information from src/clientversion.h
Fail when the version information is not defined otherwise when
HAVE_CONFIG_H is not set.
Merge #9902: Lightweight abstraction of boost::filesystem
f110272 Remove `namespace fs=fs` (Wladimir J. van der Laan)
75594bd torcontrol: Use fs::path instead of std::string for private key path (Wladimir J. van der Laan)
2a5f574 Use fsbridge for fopen and freopen (Wladimir J. van der Laan)
bac5c9c Replace uses of boost::filesystem with fs (Wladimir J. van der Laan)
7d5172d Replace includes of boost/filesystem.h with fs.h (Wladimir J. van der Laan)
19e36bb Add fs.cpp/h (Wladimir J. van der Laan)

Tree-SHA512: 2c34f059dfa6850b9323f3389e9090a6b5f839a457a2960d182c2ecfafd9883c956f5928bb796613402d3aad68ebc78259796a7a313f4a6cfa98aaf507a66842

laanwj and others added some commits Apr 7, 2017

Merge #10162: [trivial] Log calls to getblocktemplate
1352092 Log calls to getblocktemplate (John Newbery)

Tree-SHA512: de2c6faac8baea2f63eb499dbcd0669062a71759050cf8bcada9b454fe82f056c23635d41c755badf55158ffc40a380f82bea1f40c8a2cc51604d981515b71eb
Merge #10155: build: Deduplicate version numbers
9ff7818 doc: Update release process for simplified version bumping (Wladimir J. van der Laan)
08d9aee build: Remove duplicate version information from src/clientversion.h (Wladimir J. van der Laan)
168a703 doc: Make build system insert version in Doxyfile (Wladimir J. van der Laan)
b67eb8d doc: Remove version numbers from READMEs (Wladimir J. van der Laan)

Tree-SHA512: 531e44282b1c1383a382847a5605177917dfbf78acfaa754d1cbadd2e165c7e34ddbd01790f87615083fac359571708c2551ad24b712aab1f84a2068360c3a17
Merge #9681: Refactor Bumpfee, move core functionality to CWallet
5f59d3e Improve CFeeBumper interface, add comments, make use of std::move (Jonas Schnelli)
0df22ed Cancel feebump is vErrors is not empty (Jonas Schnelli)
44cabe6 Use static calls for GetRequiredFee and GetMinimumFee, remove make_pair from emplace_back (Jonas Schnelli)
bb78c15 Restore CalculateMaximumSignedTxSize function signature (Jonas Schnelli)
51ea44f Use "return false" instead assert() in CWallet::SignTransaction (Jonas Schnelli)
bcc72cc Directly abort execution in FeeBumper::commit if wallet or tx is not available (Jonas Schnelli)
2718db0 Restore invalid fee check (must be > 0) (Jonas Schnelli)
0337a39 Refactor Bumpfee core functionality (Jonas Schnelli)
d1a95e8 Bumpfee move request parameter interaction to the top (Jonas Schnelli)

Tree-SHA512: 0e6d1f3322ed671fa2291e59ac9556ce4646bc78267edc6eedc46b0014b7b08aa83c30315358b911d82898847d4845634a18b67e253a7b699dcc852eb2652c07
Merge #10159: [tests] color test results and sort alphabetically
d80baaa fixup - align summary row correctly and make colors/glyphs globals (John Newbery)
bb92d83 [tests] Add unicode symbols for tests passing/failing/skipping (John Newbery)
63062bd [tests] color test results and sort alphabetically (John Newbery)

Tree-SHA512: a5b85c05480722abd6e483d1817b7527ca487b8bb8292bc81efba158df5a619b8103ed43b790396071ab0710f39457895a79460480044324798c81331bbade5a
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 7, 2017

Member

This is because there is the potential for the size of the application to be larger then the size of the current screen, abs() should handle that corner case.

Thinking of it, I don't think abs fixes this problem.
E.g. say the screen is 10x10, and the window 20x20. With the code without abs, the position of the window will be ((10-20)/2, (10-20)/2) or (-5,-5): screen will be centered in the center of the window instead of the other way around.

+-----------------|
| window          |
| +------------+  |
| |            |  |
| |  screen    |  |
| |            |  |
| +------------+  |
|                 |
+-----------------|

This is not useful, as agreed. However abs would cause the window to be at (5,5), which reflects the coordinates over both axes and causes something like

+------------+
| screen     |
|   +-----------------+
|   |  window|        |
+---|--------+        | 
    |                 |
    |                 |
    +-----------------+

I'm not sure that makes sense. What would be the reasoning to put it there? A part of the window will still be outside the screen.

If the size of the application is larger than the size of the screen, wouldn't it be better to resize the application to fullscreen? Or alternatively, just ignore the hint, and leave the application at the default size in the center.

Member

laanwj commented Apr 7, 2017

This is because there is the potential for the size of the application to be larger then the size of the current screen, abs() should handle that corner case.

Thinking of it, I don't think abs fixes this problem.
E.g. say the screen is 10x10, and the window 20x20. With the code without abs, the position of the window will be ((10-20)/2, (10-20)/2) or (-5,-5): screen will be centered in the center of the window instead of the other way around.

+-----------------|
| window          |
| +------------+  |
| |            |  |
| |  screen    |  |
| |            |  |
| +------------+  |
|                 |
+-----------------|

This is not useful, as agreed. However abs would cause the window to be at (5,5), which reflects the coordinates over both axes and causes something like

+------------+
| screen     |
|   +-----------------+
|   |  window|        |
+---|--------+        | 
    |                 |
    |                 |
    +-----------------+

I'm not sure that makes sense. What would be the reasoning to put it there? A part of the window will still be outside the screen.

If the size of the application is larger than the size of the screen, wouldn't it be better to resize the application to fullscreen? Or alternatively, just ignore the hint, and leave the application at the default size in the center.

@AllanDoensen

This comment has been minimized.

Show comment
Hide comment
@AllanDoensen

AllanDoensen Apr 7, 2017

Contributor

@laanwj The screen is placed in a location that the user can easily grab the top & edges. This allows the user to choose a new size. If you do not do the abs() then the user does not have access to a single edge. The user cannot cannot easily grab the application and re-size/move it...hence the move and the abs() .

While we could also resize the application to fit, I think if you do that there are many other considerations and the code starts to get messy. I did contemplate it, but this is really an extreme corner case we are talking about here.

The important point is that the user has a means of easily interacting with the application when it starts. By placing the application top right comer on the screen guarantees that the user can move & resize that application. Without the abs() that becomes much harder because the user has no edge to select and resize.

Contributor

AllanDoensen commented Apr 7, 2017

@laanwj The screen is placed in a location that the user can easily grab the top & edges. This allows the user to choose a new size. If you do not do the abs() then the user does not have access to a single edge. The user cannot cannot easily grab the application and re-size/move it...hence the move and the abs() .

While we could also resize the application to fit, I think if you do that there are many other considerations and the code starts to get messy. I did contemplate it, but this is really an extreme corner case we are talking about here.

The important point is that the user has a means of easily interacting with the application when it starts. By placing the application top right comer on the screen guarantees that the user can move & resize that application. Without the abs() that becomes much harder because the user has no edge to select and resize.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 7, 2017

Member

The screen is placed in a location that the user can easily grab the top & edges.

That's not guaranteed to be the case. The window could be twice the size of the screen, for example.

While we could also resize the application to fit, I think if you do that there are many other considerations and the code starts to get messy. I did contemplate it, but this is really an extreme corner case we are talking about here.

What about my proposal to just go to default settings in that case? I think that is the best fallback if the settings in the QSettings don't make sense (sometimes they get corrupted too), just ignore them.

Member

laanwj commented Apr 7, 2017

The screen is placed in a location that the user can easily grab the top & edges.

That's not guaranteed to be the case. The window could be twice the size of the screen, for example.

While we could also resize the application to fit, I think if you do that there are many other considerations and the code starts to get messy. I did contemplate it, but this is really an extreme corner case we are talking about here.

What about my proposal to just go to default settings in that case? I think that is the best fallback if the settings in the QSettings don't make sense (sometimes they get corrupted too), just ignore them.

@AllanDoensen

This comment has been minimized.

Show comment
Hide comment
@AllanDoensen

AllanDoensen Apr 7, 2017

Contributor

Yes, that is acceptable.

Contributor

AllanDoensen commented Apr 7, 2017

Yes, that is acceptable.

@AllanDoensen

This comment has been minimized.

Show comment
Hide comment
@AllanDoensen

AllanDoensen Apr 7, 2017

Contributor

Should I update it or do you/others want the say first?

Contributor

AllanDoensen commented Apr 7, 2017

Should I update it or do you/others want the say first?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 7, 2017

Member

That's up to you. To be clear I'm fine with the fix as it is now, as at least it improves things compared to how it is now, but I just think it could be even more robust.

Member

laanwj commented Apr 7, 2017

That's up to you. To be clear I'm fine with the fix as it is now, as at least it improves things compared to how it is now, but I just think it could be even more robust.

jnewbery and others added some commits Mar 31, 2017

Reduce spammy test logging
This commit reduces spammy logging by the test framework. It truncates
logging send/receive message in mininode to 500 characters.  mininode
was previously logging the entire message sent received, which can be up
to 1MB for a full block.
Changes after comments from @laanwj.
If the application is detected to be in an invalid position then the default size and position are now used.
Use of use of abs() has been removed.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 8, 2017

Member

Looks good to me now, utACK after squash.

Member

laanwj commented Apr 8, 2017

Looks good to me now, utACK after squash.

Merge #10124: [test] Suppress test logging spam
45ce471 Reduce spammy test logging (John Newbery)

Tree-SHA512: 64b2ce29fb62a4e738840bbaf93563559451c2ef078ba66ecfc1dbe34adefea61ad2ad2d768444cb2e0b30cb3cbe47e38ed818d4c91f7723a3d1ba9fdd0043f9
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Apr 8, 2017

Member

Seems the right behaviour here is to let the WM give it the default position. (Arguably we shouldn't be saving it at all, but that's another issue.)

Member

luke-jr commented Apr 8, 2017

Seems the right behaviour here is to let the WM give it the default position. (Arguably we shouldn't be saving it at all, but that's another issue.)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 8, 2017

Member

Yeah yeah, let's just agree that this is an improvement over not being able to find the window.
Whether to store the window position at all is a completely different discussion that doesn't belong here.

Member

laanwj commented Apr 8, 2017

Yeah yeah, let's just agree that this is an improvement over not being able to find the window.
Whether to store the window position at all is a completely different discussion that doesn't belong here.

AllanDoensen added some commits Apr 4, 2017

Fix for issues with startup and mutiple monitors on windows.
Changes after comments from @laanwj.

If the application is detected to be in an invalid position then the default size and position are now used.
Use of use of abs() has been removed.
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 10, 2017

Member

@AllanDoensen: commit history is messed up. Maybe do a git fetch origin; git reset --hard origin/master and cherry-pick the relevant commit(s).

Member

jonasschnelli commented Apr 10, 2017

@AllanDoensen: commit history is messed up. Maybe do a git fetch origin; git reset --hard origin/master and cherry-pick the relevant commit(s).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 10, 2017

Member

Oops. I think I have the previous version of this saved somewhere.

Member

laanwj commented Apr 10, 2017

Oops. I think I have the previous version of this saved somewhere.

laanwj added a commit that referenced this pull request Apr 10, 2017

Merge #10156: Fix for issues with startup and multiple monitors on wi…
…ndows.

e9ff818 Fix for issues with startup and multiple monitors on windows. (Allan Doensen)

Tree-SHA512: 8502042a9b5a2fd6f5e409163bee9bd7c85e34c158754f393065f8cc6cdd0f8505b9a1803069d01fc1fb2df04d1b2ed6291388851f2ed3608eb2dd53fc22e06e
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 10, 2017

Member

Merged via 51833a1 (retrieved the previous good version and squashed)

Member

laanwj commented Apr 10, 2017

Merged via 51833a1 (retrieved the previous good version and squashed)

@laanwj laanwj closed this Apr 10, 2017

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

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

Changes after comments from @laanwj.
If the application is detected to be in an invalid position then the default size and position are now used.
Use of use of abs() has been removed.

Github-Pull: #10156
Rebased-From: 3b58485

@laanwj laanwj added this to the 0.14.3 milestone Sep 5, 2017

@fanquake fanquake removed the Needs backport label Mar 7, 2018

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