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

Fix TxViewDelegate layout #176

Merged
merged 3 commits into from
Jan 21, 2021
Merged

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jan 3, 2021

This change:

  • prevents overlapping date and amount strings
  • guaranties that "eye" sign at the end of the watch-only address/label is always visible

Fix bitcoin/bitcoin#20826

Here are some screenshots with this PR with the minimum available width of the transaction list widget:

Screenshot from 2021-01-03 20-23-56
Screenshot from 2021-01-03 20-24-47
Screenshot from 2021-01-03 20-25-27
Screenshot from 2021-01-03 20-33-20

@dooglus
Copy link
Contributor

dooglus commented Jan 4, 2021

Looks good to me. The window is a lot bigger than it was before, and perhaps bigger than absolutely necessary but this is as small as I can make it:

Screenshot_2021-01-04_07-58-45

For comparison, this is the 'before' shot:

Screenshot_2021-01-01_14-19-16

Edit: the client window is made wider by this fix even for wallets that have no watchonly addresses. Is that intentional?

@maflcko maflcko added this to the 0.21.1 milestone Jan 4, 2021
@maflcko
Copy link
Contributor

maflcko commented Jan 4, 2021

Is this for 0.21.1?

@hebasto
Copy link
Member Author

hebasto commented Jan 4, 2021

Is this for 0.21.1?

Yes, I think so, as it is a bugfix.

@hebasto
Copy link
Member Author

hebasto commented Jan 4, 2021

@dooglus

The window is ... bigger than absolutely necessary

On fresh Debian 10.7 with Xfce 4.12 cannot reproduce such behavior:
Screenshot from 2021-01-04 22-31-19

Did you tweak fonts or themes?

@dooglus
Copy link
Contributor

dooglus commented Jan 5, 2021

Yes I did, and I've included a screenshot of my settings below, but I ought to describe the issue more fully now that I've played around with it more.

I can reproduce the issue as follows: I have 2 wallets, one with watchonly addresses and one without. I have the one without show up by default when the client first loads, and it makes the client window 1109x842 pixels. If I then use the menu in the top right to switch to the wallet with watchonly addresses then the window resizes itself to 1329x842 to make room for the watchonly address balances as expected.

The problem is when I switch back to the first wallet. The window stays at its increased size and I can't shrink it back. Some screenshots:

  1. no watchonly - small window:

new0

  1. watchonly - bigger window:

new1

  1. back to no watchonly, but the window stays big:

new2


I have the following font settings:

Screenshot_2021-01-04_16-49-04

This is the font I'm using.

@hebasto
Copy link
Member Author

hebasto commented Jan 5, 2021

@dooglus

Thanks for testing!

The problem is when I switch back to the first wallet. The window stays at its increased size and I can't shrink it back.

Confirming this regression. Looking for a fix.

This change (1) prevents overlapping date and amount strings,
and (2) guaranties that "eye" sign at the end of the watch-only
address/label is always visible.
@hebasto
Copy link
Member Author

hebasto commented Jan 5, 2021

Updated 5cf67f2 -> 1e31eca (pr176.01 -> pr176.02, diff):

The problem is when I switch back to the first wallet. The window stays at its increased size and I can't shrink it back.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1e31eca

Tested on macOS 11.1 and Arch Linux both with Qt 5.15.2.

I was able to replicate the bug described by @dooglus on 5cf67f2. I can confirm that 1e31eca now fixes this for me.

@dooglus
Copy link
Contributor

dooglus commented Jan 8, 2021

I cherry-picked your commits back to the v0.21.0rc4 tag and built. I saw a warning:

qt/walletframe.cpp: In member function ‘void WalletFrame::setCurrentWallet(WalletModel*)’:
qt/walletframe.cpp:110:9: warning: init-statement in selection statements only available with -std=c++17 or -std=gnu++17
     if (WalletView* view_about_to_hide = currentWalletView(); view_about_to_hide != nullptr) {
         ^~~~~~~~~~

I don't know if I'm supposed to be using a newer compiler but it wasn't enforced when I ran the configure script. If this is intended to be backported to 0.21.1 maybe it shouldn't use c++17 features.

Despite the warning the code works and I can now shrink the window back after switching to and back from the watchonly wallet.

Layouts of the hidden widgets, those are children of QStackedWidget,
could prevent to adjust the size of the parent widget in the
WalletFrame widget.
@hebasto
Copy link
Member Author

hebasto commented Jan 8, 2021

Updated 1e31eca -> af58f5b (pr176.02 -> pr176.03, diff):

If this is intended to be backported to 0.21.1 maybe it shouldn't use c++17 features.

@dooglus
Copy link
Contributor

dooglus commented Jan 10, 2021

ACK af58f5b.

Now it resizes the window to make room for the watchonly addresses, allows me to shrink the window back when viewing a wallet without any such addresses, and builds without warnings when I backport it to v0.21.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK af58f5b

Saw the same warning as @dooglus when patching 1e31eca on 0.21rc4. Confirming that no warnings come up when patching af58f5b on top of 0.21rc5. Tested on macOS 11.1 and Arch Linux both with Qt 5.15.2

@hebasto
Copy link
Member Author

hebasto commented Jan 12, 2021

@dooglus

ACK 1e31eca.

Now it resizes the window to make room for the watchonly addresses, allows me to shrink the window back when viewing a wallet without any such addresses, and builds without warnings when I backport it to v0.21.

Thanks for reviewing! Is it intentional the previous commit (not the current top) mentioned? 😄

@dooglus
Copy link
Contributor

dooglus commented Jan 14, 2021

No, sorry. I meant af58f5b. Have edited it now.

@maflcko maflcko merged commit 7f653c3 into bitcoin-core:master Jan 21, 2021
@hebasto hebasto deleted the 210103-delegate branch January 21, 2021 17:59
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jan 21, 2021
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jan 21, 2021
This change (1) prevents overlapping date and amount strings,
and (2) guaranties that "eye" sign at the end of the watch-only
address/label is always visible.

Github-Pull: bitcoin-core/gui#176
Rebased-From: f0d0479
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jan 21, 2021
Layouts of the hidden widgets, those are children of QStackedWidget,
could prevent to adjust the size of the parent widget in the
WalletFrame widget.

Github-Pull: bitcoin-core/gui#176
Rebased-From: af58f5b
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 21, 2021
af58f5b qt: Stop the effect of hidden widgets on the size of QStackedWidget (Hennadii Stepanov)
f0d0479 qt: Fix TxViewDelegate layout (Hennadii Stepanov)
d439921 qt: Add TransactionOverviewWidget class (Hennadii Stepanov)

Pull request description:

  This change:
  - prevents overlapping date and amount strings
  - guaranties that "eye" sign at the end of the watch-only address/label is always visible

  Fix bitcoin#20826

  Here are some screenshots with this PR with the _minimum available width_ of the transaction list widget:

  ![Screenshot from 2021-01-03 20-23-56](https://user-images.githubusercontent.com/32963518/103486411-6408ca00-4e06-11eb-9c21-627a65e532c1.png)
  ![Screenshot from 2021-01-03 20-24-47](https://user-images.githubusercontent.com/32963518/103486413-6834e780-4e06-11eb-8221-478d98bbdf69.png)
  ![Screenshot from 2021-01-03 20-25-27](https://user-images.githubusercontent.com/32963518/103486418-6d923200-4e06-11eb-8625-a4ed3089b6ab.png)
  ![Screenshot from 2021-01-03 20-33-20](https://user-images.githubusercontent.com/32963518/103486420-708d2280-4e06-11eb-90c2-f2463fb3c4b3.png)

ACKs for top commit:
  dooglus:
    ACK af58f5b.
  jarolrod:
    re-ACK af58f5b

Tree-SHA512: 6dae682490ec50fa0335d220bc2d153fa3e6ed578f07c6353a3b180f8f6cf1c2f9e52ebd7b3076f51d7004d86bf5cca14e6b5db9cdf786e85a57a81eacbb4988
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 21, 2021
Github-Pull: bitcoin-core/gui#176
Rebased-From: d439921
(cherry picked from commit b7086e6)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 21, 2021
This change (1) prevents overlapping date and amount strings,
and (2) guaranties that "eye" sign at the end of the watch-only
address/label is always visible.

Github-Pull: bitcoin-core/gui#176
Rebased-From: f0d0479
(cherry picked from commit 7bc4498)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 21, 2021
Layouts of the hidden widgets, those are children of QStackedWidget,
could prevent to adjust the size of the parent widget in the
WalletFrame widget.

Github-Pull: bitcoin-core/gui#176
Rebased-From: af58f5b
(cherry picked from commit bdc64c9)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 21, 2021
Github-Pull: bitcoin-core/gui#176
Rebased-From: d439921
(cherry picked from commit b7086e6)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 21, 2021
This change (1) prevents overlapping date and amount strings,
and (2) guaranties that "eye" sign at the end of the watch-only
address/label is always visible.

Github-Pull: bitcoin-core/gui#176
Rebased-From: f0d0479
(cherry picked from commit 7bc4498)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 21, 2021
Layouts of the hidden widgets, those are children of QStackedWidget,
could prevent to adjust the size of the parent widget in the
WalletFrame widget.

Github-Pull: bitcoin-core/gui#176
Rebased-From: af58f5b
(cherry picked from commit bdc64c9)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 22, 2021
Github-Pull: bitcoin-core/gui#176
Rebased-From: d439921
(cherry picked from commit b7086e6)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 22, 2021
This change (1) prevents overlapping date and amount strings,
and (2) guaranties that "eye" sign at the end of the watch-only
address/label is always visible.

Github-Pull: bitcoin-core/gui#176
Rebased-From: f0d0479
(cherry picked from commit 7bc4498)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 22, 2021
Layouts of the hidden widgets, those are children of QStackedWidget,
could prevent to adjust the size of the parent widget in the
WalletFrame widget.

Github-Pull: bitcoin-core/gui#176
Rebased-From: af58f5b
(cherry picked from commit bdc64c9)
hebasto added a commit that referenced this pull request Jun 15, 2022
a50e0b1 qt, refactor: Add `transactionoverviewwidget.cpp` source file (Hennadii Stepanov)

Pull request description:

  The `TransactionOverviewWidget` class was added in #176 as a header-only one.

  Apparently, in upcoming [CMake project](hebasto/bitcoin#3), CMake [AUTOMOC](https://cmake.org/cmake/help/latest/prop_tgt/AUTOMOC.html) could be integrated better/simpler, if `QObject`-derived class implementation been placed into a source file.

  From our [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization):
  > Implementation code should go into the `.cpp` file and not the `.h`, unless necessary due to template usage or when performance due to inlining is critical.

ACKs for top commit:
  Sjors:
    tACK a50e0b1
  shaavan:
    ACK a50e0b1

Tree-SHA512: 4707b6be1c5e794c4014475f826ac45ec833e472db11f12d29995f9c5a599ee98622ad54f0af72734b192144b626411c69acdafa0e6d1a390bdebfd7e570f377
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 15, 2022
…cpp` source file

a50e0b1 qt, refactor: Add `transactionoverviewwidget.cpp` source file (Hennadii Stepanov)

Pull request description:

  The `TransactionOverviewWidget` class was added in bitcoin-core/gui#176 as a header-only one.

  Apparently, in upcoming [CMake project](hebasto#3), CMake [AUTOMOC](https://cmake.org/cmake/help/latest/prop_tgt/AUTOMOC.html) could be integrated better/simpler, if `QObject`-derived class implementation been placed into a source file.

  From our [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization):
  > Implementation code should go into the `.cpp` file and not the `.h`, unless necessary due to template usage or when performance due to inlining is critical.

ACKs for top commit:
  Sjors:
    tACK a50e0b1
  shaavan:
    ACK a50e0b1

Tree-SHA512: 4707b6be1c5e794c4014475f826ac45ec833e472db11f12d29995f9c5a599ee98622ad54f0af72734b192144b626411c69acdafa0e6d1a390bdebfd7e570f377
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GUI: recent transaction timestamps overlap amounts when watchonly addresses are in wallet
5 participants