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

AllanDoensen · 2017-04-05T07:51:53Z

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:

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.
Start bitcoin-qt and place it on the second screen.
Close bitcoin-qt (it will cache it’s position on the second screen).
Remove (physically unplug) the second screen.
Start bitcoin-qt.
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.

…available

…ir from emplace_back

This is step one in abstracting the use of boost::filesystem.

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' ```

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.

Having these inside functions is silly and redundant now.

If we want to keep these numbers, could generate them using autoconf. But this seems unnecessary.

jonasschnelli · 2017-04-05T07:56:42Z

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 · 2017-04-05T07:59:14Z

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)

faafa80 init: Remove redundant logging code (MarcoFalke) Tree-SHA512: 5ad0e9aba0e25a36025dd4ee5e5fddd2c0039f95bafd0f33300ea59e2f9bba807da6a1a8b4311d6aad5a360b99163edf4a4f161cb13f0f38580d8d6b504c94ad

…f GetLogCategory() fails) cd7f394 initialize flag variable to 0 (and continue if GetLogCategory() fails) (John Newbery) Tree-SHA512: d0f2653bd0e71ed763220cb08d3a5335c5bdfe2f54ff7f9302d97f3265d7aa7f57606fe416a61aaac1535dbb046d0fb40a61f5a9d5cf234b042268e00ee7679d

sipa · 2017-04-05T12:37:04Z

Does this apply to 0.14?

AllanDoensen · 2017-04-05T13:37:45Z

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

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 · 2017-04-07T15:30:16Z

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 · 2017-04-07T16:08:24Z

@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 · 2017-04-07T16:12:56Z

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 · 2017-04-07T16:14:18Z

Yes, that is acceptable.

AllanDoensen · 2017-04-07T16:15:22Z

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

laanwj · 2017-04-07T16:20:30Z

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.

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.

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 · 2017-04-08T07:52:35Z

Looks good to me now, utACK after squash.

45ce471 Reduce spammy test logging (John Newbery) Tree-SHA512: 64b2ce29fb62a4e738840bbaf93563559451c2ef078ba66ecfc1dbe34adefea61ad2ad2d768444cb2e0b30cb3cbe47e38ed818d4c91f7723a3d1ba9fdd0043f9

luke-jr · 2017-04-08T13:56:48Z

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 · 2017-04-08T14:07:45Z

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.

@laanwj

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.

…itcoin into masterMultiMonFix Squashed.

jonasschnelli · 2017-04-10T08:43:47Z

@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 · 2017-04-10T08:55:19Z

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

…ndows. e9ff818 Fix for issues with startup and multiple monitors on windows. (Allan Doensen) Tree-SHA512: 8502042a9b5a2fd6f5e409163bee9bd7c85e34c158754f393065f8cc6cdd0f8505b9a1803069d01fc1fb2df04d1b2ed6291388851f2ed3608eb2dd53fc22e06e

laanwj · 2017-04-10T09:40:27Z

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

Github-Pull: bitcoin#10156 Rebased-From: 1896818

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: bitcoin#10156 Rebased-From: 3b58485

jonasschnelli and others added 20 commits April 2, 2017 09:48

Bumpfee move request parameter interaction to the top

d1a95e8

Refactor Bumpfee core functionality

0337a39

Restore invalid fee check (must be > 0)

2718db0

Directly abort execution in FeeBumper::commit if wallet or tx is not …

bcc72cc

…available

Use "return false" instead assert() in CWallet::SignTransaction

51ea44f

Restore CalculateMaximumSignedTxSize function signature

bb78c15

Use static calls for GetRequiredFee and GetMinimumFee, remove make_pa…

44cabe6

…ir from emplace_back

Cancel feebump is vErrors is not empty

0df22ed

Improve CFeeBumper interface, add comments, make use of std::move

5f59d3e

Add fs.cpp/h

19e36bb

Replace includes of boost/filesystem.h with fs.h

7d5172d

This is step one in abstracting the use of boost::filesystem.

Replace uses of boost::filesystem with fs

bac5c9c

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

2a5f574

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.

torcontrol: Use fs::path instead of std::string for private key path

75594bd

Remove namespace fs=fs

f110272

Having these inside functions is silly and redundant now.

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

cd7f394

init: Remove redundant logging code

faafa80

doc: Remove version numbers from READMEs

b67eb8d

If we want to keep these numbers, could generate them using autoconf. But this seems unnecessary.

doc: Make build system insert version in Doxyfile

168a703

Fix for issues with startup and mutiple monitors on windows.

1896818

fanquake added the GUI label Apr 5, 2017

laanwj added 2 commits April 5, 2017 11:28

Merge #10154: init: Remove redundant logging code

3c95bd4

faafa80 init: Remove redundant logging code (MarcoFalke) Tree-SHA512: 5ad0e9aba0e25a36025dd4ee5e5fddd2c0039f95bafd0f33300ea59e2f9bba807da6a1a8b4311d6aad5a360b99163edf4a4f161cb13f0f38580d8d6b504c94ad

[tests] color test results and sort alphabetically

63062bd

fanquake added the Needs backport label Apr 5, 2017

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

jnewbery and others added 2 commits April 7, 2017 22:16

Reduce spammy test logging

45ce471

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.

3b58485

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.

Merge #10124: [test] Suppress test logging spam

88799ea

45ce471 Reduce spammy test logging (John Newbery) Tree-SHA512: 64b2ce29fb62a4e738840bbaf93563559451c2ef078ba66ecfc1dbe34adefea61ad2ad2d768444cb2e0b30cb3cbe47e38ed818d4c91f7723a3d1ba9fdd0043f9

AllanDoensen added 2 commits April 9, 2017 15:43

Fix for issues with startup and mutiple monitors on windows.

36aeab4

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.

Merge branch 'masterMultiMonFix' of https://github.com/AllanDoensen/b…

b0c302b

…itcoin into masterMultiMonFix Squashed.

laanwj closed this Apr 10, 2017

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

Fix for issues with startup and mutiple monitors on windows.

316c001

Github-Pull: bitcoin#10156 Rebased-From: 1896818

This was referenced Apr 22, 2017

Fix for issues with startup and multiple monitors on windows. bitcoinclassic/bitcoinclassic#257

Closed

Multiple monitor bugfix bitcoinxt/bitcoinxt#196

Merged

AllanDoensen mentioned this pull request May 3, 2017

Fix for dash-qt issue with startup and multiple monitors. dashpay/dash#1461

Merged

This was referenced Sep 1, 2017

Fixing offscreen GUI issue #11208

Closed

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

Closed

laanwj added this to the 0.14.3 milestone Sep 5, 2017

fanquake removed the Needs backport label Mar 7, 2018

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

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

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

Uh oh!

Conversation

AllanDoensen commented Apr 5, 2017

Uh oh!

jonasschnelli commented Apr 5, 2017

Uh oh!

laanwj commented Apr 5, 2017

Uh oh!

sipa commented Apr 5, 2017

Uh oh!

AllanDoensen commented Apr 5, 2017

Uh oh!

laanwj commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AllanDoensen commented Apr 7, 2017

Uh oh!

laanwj commented Apr 7, 2017

Uh oh!

AllanDoensen commented Apr 7, 2017

Uh oh!

AllanDoensen commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Apr 7, 2017

Uh oh!

laanwj commented Apr 8, 2017

Uh oh!

luke-jr commented Apr 8, 2017

Uh oh!

laanwj commented Apr 8, 2017

Uh oh!

jonasschnelli commented Apr 10, 2017

Uh oh!

laanwj commented Apr 10, 2017

Uh oh!

laanwj commented Apr 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

laanwj commented Apr 7, 2017 •

edited

Loading

AllanDoensen commented Apr 7, 2017 •

edited

Loading