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

Draw "eye" sign at the beginning of watch-only addresses #365

Merged
merged 2 commits into from
Jul 5, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jun 14, 2021

This PR guaranties that the "eye" sign won't be hidden for very long addresses/labels.

No longer need to extend TransactionOverviewWidget widget width to make "eye" signs shown:

Screenshot from 2021-06-15 00-21-05

Fixes #373

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 18ff8eb

Tested on macOS 11.3 Qt 5.15.2, Arch Linux Qt 5.15.2 and cross-compile to Windows 10.

Notes on Commits:

This fixes issue #22246, see: bitcoin/bitcoin#22246 (comment)

The below screenshots show the behavior between master and this PR when presented with more than one address in a transaction on the overview page across the major platforms.

Note: The before and after screenshots are taken with gui settings wiped. If a user upgrades from v0.21.1 to v22, the window will open up as wide as it was on v0.21.1. They must then resize it to their preference, this change no longer prevents you from making the window smaller (as noted in #22246)

macOS 11.3

Master PR
master-issue test-365-resize

Arch Linux

Master PR
22246-master 22246-pr

Windows 10

Master PR
22246-master 22246-pr

This is a nice improvement UX wise for watch only wallets. The watch symbol is placed on the left hand, could not force any weird behavior from this change.

Below I attach screenshots of this PR applied on the major platforms for the purposes of documentation and clarity

macOS 11.3

Master PR
master pr

Arch Linux

Master PR
365-master 365-pr

Windows 10

Master PR
365-master 365-pr

@@ -107,9 +106,9 @@ class TxViewDelegate : public QAbstractItemDelegate

painter->setPen(option.palette.color(QPalette::Text));
QRect date_bounding_rect;
painter->drawText(amountRect, Qt::AlignLeft | Qt::AlignVCenter, GUIUtil::dateTimeStr(date), &date_bounding_rect);
painter->drawText(amountRect, Qt::AlignLeft | Qt::AlignVCenter, GUIUtil::dateTimeStr(date) + QLatin1String(" "), &date_bounding_rect);
Copy link
Member

Choose a reason for hiding this comment

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

Is this assuming spaces have a certain width?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you prefer non-breaking space?

Copy link
Member

Choose a reason for hiding this comment

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

Offset the bounding rect?

Spaces are never a well-defined width... except zero-width :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Offset in fixed number of pixels? Is it a good approach with potentially scalable fonts?

Copy link
Member

Choose a reason for hiding this comment

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

Measure the icon width.

Scalable fonts is exactly why spaces won't work reliably. ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this assuming spaces have a certain width?

Thanks! Updated.

@hebasto
Copy link
Member Author

hebasto commented Jun 28, 2021

Updated 18ff8eb -> cd46c11 (pr365.01 -> pr365.02, diff):

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 cd46c11

Tested only on macOS 11.3

Changes since last review:

addressed @luke-jr's comment

Conceptually it does make sense to not use text elements in drawing of visual elements.

Confirmed that this still fixes #373 (previously #22246)

Transactions Tab Issue

Master PR
master pr

Watch-Only Symbol

Master PR
master pr

@hebasto hebasto merged commit 484d4ee into bitcoin-core:master Jul 5, 2021
@hebasto hebasto deleted the 210614-tx branch July 5, 2021 20:50
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 5, 2021
…-only addresses

cd46c11 qt: Draw "eye" sign at the beginning of watch-only addresses (Hennadii Stepanov)
9ea1da6 qt: Do not extend recent transaction width to address/label string (Hennadii Stepanov)

Pull request description:

  This PR guaranties that the "eye" sign won't be hidden for very long addresses/labels.

  No longer need to extend `TransactionOverviewWidget` widget width to make "eye" signs shown:

  ![Screenshot from 2021-06-15 00-21-05](https://user-images.githubusercontent.com/32963518/121961807-9123b600-cd70-11eb-8cdd-8b2b0d1bf44f.png)

  Fixes bitcoin-core/gui#373

ACKs for top commit:
  jarolrod:
    ACK cd46c11

Tree-SHA512: 0602b5bb65d53c5b18e86260750006bba03adbae181917b5a2b7f89b17290bd1f57b4f80adaba32f42cc6fb468598a888b12c0b6b09005d2f2c07bd4d1ad334a
@hebasto hebasto modified the milestones: 22.0, 0.21.2 Jul 11, 2021
hebasto added a commit to hebasto/bitcoin that referenced this pull request Jul 11, 2021
hebasto added a commit to hebasto/bitcoin that referenced this pull request Jul 11, 2021
@hebasto
Copy link
Member Author

hebasto commented Jul 11, 2021

Backported in bitcoin/bitcoin#22427.

fanquake added a commit to bitcoin/bitcoin that referenced this pull request Jul 29, 2021
e3f1da4 qt: Draw "eye" sign at the beginning of watch-only addresses (Hennadii Stepanov)
6ca54ce qt: Do not extend recent transaction width to address/label string (Hennadii Stepanov)
f220368 qt: Do not use QClipboard::Selection on Windows and macOS. (Hennadii Stepanov)

Pull request description:

  Backports bitcoin-core/gui#277, bitcoin-core/gui#365.

ACKs for top commit:
  fanquake:
    ACK e3f1da4
  jarolrod:
    ACK e3f1da4

Tree-SHA512: 43cc2ac48f4e5014bfdbe86cc904bb36d2be9fcd257f0fc0800c384bd727bb98466723e450a8909b06708784ad91184be599c49cf60de2e4377202774cb878f6
ComputerCraftr pushed a commit to ComputerCraftr/XEP-Core that referenced this pull request Aug 18, 2021
e3f1da4bf3db120cc691a844d612fbc522f11fb9 qt: Draw "eye" sign at the beginning of watch-only addresses (Hennadii Stepanov)
6ca54ce2ae0808513172c4945e38165e766e1381 qt: Do not extend recent transaction width to address/label string (Hennadii Stepanov)
f220368220abb11040fa944a853cda3d4f1fe84d qt: Do not use QClipboard::Selection on Windows and macOS. (Hennadii Stepanov)

Pull request description:

  Backports bitcoin-core/gui#277, bitcoin-core/gui#365.

ACKs for top commit:
  fanquake:
    ACK e3f1da4bf3db120cc691a844d612fbc522f11fb9
  jarolrod:
    ACK e3f1da4bf3db120cc691a844d612fbc522f11fb9

Tree-SHA512: 43cc2ac48f4e5014bfdbe86cc904bb36d2be9fcd257f0fc0800c384bd727bb98466723e450a8909b06708784ad91184be599c49cf60de2e4377202774cb878f6
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@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
Bug Something isn't working UI All about "look and feel"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.21.1 Recent transactions tab expands entire window beyond screen resolution
3 participants