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

AllanDoensen commented at 7:51 AM on April 5, 2017: contributor

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.

Bumpfee move request parameter interaction to the top d1a95e8d3d

Refactor Bumpfee core functionality 0337a39d31

Restore invalid fee check (must be > 0) 2718db0705

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

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

Restore CalculateMaximumSignedTxSize function signature bb78c1599e

Use static calls for GetRequiredFee and GetMinimumFee, remove make_pair from emplace_back 44cabe6380

Cancel feebump is vErrors is not empty 0df22ed6fd

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

Add fs.cpp/h 19e36bbef6

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

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

7d5172d354

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

bac5c9cf64

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.

2a5f574762

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

Remove `namespace fs=fs`

Having these inside functions is silly and redundant now.

f110272dc9

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

init: Remove redundant logging code faafa801e8

doc: Remove version numbers from READMEs

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

b67eb8dde8

doc: Make build system insert version in Doxyfile 168a7034f5

Fix for issues with startup and mutiple monitors on windows. 18968187da

fanquake added the label GUI on Apr 5, 2017

jonasschnelli commented at 7:56 AM on April 5, 2017: contributor

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 18968187da375b1a71af102c13b0a9e5d7ea65ef

laanwj commented at 7:59 AM on April 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)

Merge #10154: init: Remove redundant logging code

faafa80 init: Remove redundant logging code (MarcoFalke)

Tree-SHA512: 5ad0e9aba0e25a36025dd4ee5e5fddd2c0039f95bafd0f33300ea59e2f9bba807da6a1a8b4311d6aad5a360b99163edf4a4f161cb13f0f38580d8d6b504c94ad

3c95bd43d8

Merge #10151: [logging] initialize flag variable to 0 (and continue if GetLogCategory() fails)

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

Tree-SHA512: d0f2653bd0e71ed763220cb08d3a5335c5bdfe2f54ff7f9302d97f3265d7aa7f57606fe416a61aaac1535dbb046d0fb40a61f5a9d5cf234b042268e00ee7679d

c7e73eafa1

sipa commented at 12:37 PM on April 5, 2017: member

Does this apply to 0.14?

AllanDoensen commented at 1:37 PM on April 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.

[tests] color test results and sort alphabetically 63062bda1a

fanquake added the label Needs backport on Apr 5, 2017

laanwj renamed this:
~~Fix for issues with startup and mutiple monitors on windows.~~
Fix for issues with startup and multiple monitors on windows.
on Apr 6, 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.

08d9aee3eb

doc: Update release process for simplified version bumping 9ff781884a

Log calls to getblocktemplate 1352092dbd

[tests] Add unicode symbols for tests passing/failing/skipping bb92d839d5

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

8c28670e92

Merge #10162: [trivial] Log calls to getblocktemplate

1352092 Log calls to getblocktemplate (John Newbery)

Tree-SHA512: de2c6faac8baea2f63eb499dbcd0669062a71759050cf8bcada9b454fe82f056c23635d41c755badf55158ffc40a380f82bea1f40c8a2cc51604d981515b71eb

a3a2160b66

fixup - align summary row correctly and make colors/glyphs globals d80baaa514

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

928695bee6

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

a5fd746674

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

df1ca9e93a

laanwj commented at 3:30 PM on April 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.

AllanDoensen commented at 4:08 PM on April 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.

laanwj commented at 4:12 PM on April 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.

AllanDoensen commented at 4:14 PM on April 7, 2017: contributor

Yes, that is acceptable.

AllanDoensen commented at 4:15 PM on April 7, 2017: contributor

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

laanwj commented at 4:20 PM on April 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.

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.

45ce471ab0

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.

3b584859fc

laanwj commented at 7:52 AM on April 8, 2017: member

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

88799ea1b1

luke-jr commented at 1:56 PM on April 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.)

laanwj commented at 2:07 PM on April 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.

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.

36aeab4b99

Merge branch 'masterMultiMonFix' of https://github.com/AllanDoensen/bitcoin into masterMultiMonFix

Squashed.

b0c302bdec

jonasschnelli commented at 8:43 AM on April 10, 2017: contributor

@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 commented at 8:55 AM on April 10, 2017: member

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

laanwj referenced this in commit 51833a1734 on Apr 10, 2017

laanwj commented at 9:40 AM on April 10, 2017: member

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

laanwj closed this on Apr 10, 2017

luke-jr referenced this in commit 316c00164c on Apr 21, 2017

luke-jr referenced this in commit 3d5604a244 on Apr 21, 2017

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

fanquake removed the label Needs backport on Mar 7, 2018

DrahtBot locked this on Sep 8, 2021