Remove confusing "Dust" label from coincontrol / sendcoins dialog by theStack · Pull Request #719 · bitcoin-core/gui

theStack · 2023-03-13T22:47:51Z

In contrast to to all other labels on the coin selection dialog, the displayed dust information has nothing to do with the selected coins. All that this label shows is whether at least one of the outputs qualify as dust, but the outputs are set in a different dialog. (Even worse, the dust check is currently simply wrong because it only looks at an output's nValue and just assumes a P2PKH script size.)

As the label clearly doesn't help the user and is, quite the contrary, rather increasing confusion/misguidance, it seems sensible to remove it. The label from the sendcoins dialog is also removed with the same rationale. Additionally, the "bytes" and "change" labels are aligned to the left (second commit).

Closes #699.

DrahtBot · 2023-03-13T22:47:54Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, hebasto
Concept ACK	jarolrod, MarcoFalke, luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

furszy

What about the "Dust" label in the send screen? it is not being set anywhere.

theStack · 2023-03-19T01:37:59Z

@furszy: Thanks, good catch, completely missed that! Force-pushed to remove that as well. (FWIW, I do think having a dust label in the sendcoins dialog would be useful, but only if it is always displayed, independently of whether coin control is used).

furszy

While you are here, could also align the "bytes" and "change" labels to left. Feel free to squash this:
furszy/bitcoin-core@53165e3

theStack · 2023-03-22T01:59:55Z

While you are here, could also align the "bytes" and "change" labels to left. Feel free to squash this: furszy/bitcoin-core@53165e3

Could you post a screenshot on how dialogs look before/after this change? On my end this doesn't change anything and the alignment seems to be fine even on master.
// EDIT: Nevermind, seeing it now in your posted screenshot above. Guess that it's more a matter of taste if we want to align around the colon (:) or left though. Added your commit, I think removing an item and moving another one are different enough to having it in separate commits.

furszy

ACK 394300c

hebasto

ACK 394300c

The commit message and the PR description should mention changes in the SendCoinsDialog as well.

hebasto · 2023-03-27T13:19:06Z

             <item>
              <layout class="QFormLayout" name="formLayoutCoinControl1">
+               <property name="labelAlignment">
+                <set>Qt::AlignLeading|Qt::AlignLeft|Qt::AlignVCenter</set>


Isn't Qt::AlignLeading a synonym for Qt::AlignLeft?

I'm not too experienced with Qt, can anyone elaborate on that question (ping @furszy as author of the second commit)? It seems at least that we already have a couple of instances in master where both Qt::AlignLeading and Qt::AlignLeft are used together:

$ git grep AlignLeading src/qt/forms/debugwindow.ui: <set>Qt::AlignBottom|Qt::AlignLeading|Qt::AlignLeft</set> src/qt/forms/helpmessagedialog.ui: <set>Qt::AlignLeading|Qt::AlignLeft|Qt::AlignTop</set> src/qt/forms/helpmessagedialog.ui: <set>Qt::AlignLeading|Qt::AlignLeft|Qt::AlignTop</set> src/qt/forms/signverifymessagedialog.ui: <set>Qt::AlignLeading|Qt::AlignLeft|Qt::AlignTop</set>

It seems at least that we already have a couple of instances in master where both Qt::AlignLeading and Qt::AlignLeft are used together.

Quite sure that we have them because of QT creator setting the value automatically. Probably because there is some ancient qt version that doesn't have them as synonyms.

But for us, it should be fine to drop them, even qt 4.8 has them as synonyms (link).

@furszy: Thanks a lot for checking. I removed the redundant Qt::AlignLeading properties from the second commit, i.e. it's only Qt::AlignLeft remaining.

jarolrod

Concept ACK, the changes to the UI file aside from removing the dust label are changing the positioning of the Titles relevant to the value. The screenshot below shows this with the "Bytes" and "Change" field. The fully visible text is master, and the slightly transparent text is the pr.

maflcko · 2023-06-08T10:47:14Z

Concept ACK

luke-jr · 2023-07-01T21:49:33Z

Concept ACK, but I see no change from the second commit. Only the coincontrol dialog's "Dust" field disappears - nothing else differs in my screenshots.

In contrast to to all other labels on the coin selection dialog, the displayed dust information has nothing to do with the selected coins. All that this label shows is whether at least one of the _outputs_ qualify as dust, but the outputs are set in a different dialog. (Even worse, the dust check is currently simply wrong because it only looks at an output's nValue and just assumes a P2PKH script size.) As the label clearly doesn't help the user and is, quite the contrary, rather increasing confusion/misguidance, it seems sensible to remove it. Also, remove the label from the sendcoins dialog with the same rationale.

theStack · 2023-07-03T16:15:41Z

The commit message and the PR description should mention changes in the SendCoinsDialog as well.

Sorry for the late reply, missed that. Force-pushed with an adapted commit message (and changed PR title / description as well):

$ git range-diff 394300c18...ef4185d2b
1:  944263a93 ! 1:  210ef1e98 qt: remove confusing "Dust" label from coincontrol dialog
    @@ Metadata
     Author: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

      ## Commit message ##
    -    qt: remove confusing "Dust" label from coincontrol dialog
    +    qt: remove confusing "Dust" label from coincontrol / sendcoins dialog

         In contrast to to all other labels on the coin selection dialog, the
         displayed dust information has nothing to do with the selected coins.
    @@ Commit message
         As the label clearly doesn't help the user and is, quite the contrary,
         rather increasing confusion/misguidance, it seems sensible to remove it.

    +    Also, remove the label from the sendcoins dialog with the same rationale.
    +
      ## src/qt/coincontroldialog.cpp ##
     @@ src/qt/coincontroldialog.cpp: CoinControlDialog::CoinControlDialog(CCoinControl& coin_control, WalletModel* _m
          QAction *clipboardFeeAction = new QAction(tr("Copy fee"), this);
2:  394300c18 = 2:  ef4185d2b gui: send, left alignment for "bytes" and "change" label

furszy

ACK a582b41

hebasto

Looks good. ACK a582b41.

hebasto changed the title ~~qt: remove confusing "Dust" label from coincontrol dialog~~ Remove confusing "Dust" label from coincontrol dialog Mar 14, 2023

hebasto added the Wallet label Mar 14, 2023

furszy reviewed Mar 19, 2023

View reviewed changes

theStack force-pushed the gui-nuke_cc_dust_label branch from 8b7c589 to 944263a Compare March 19, 2023 01:35

furszy reviewed Mar 19, 2023

View reviewed changes

furszy reviewed Mar 22, 2023

View reviewed changes

hebasto reviewed Mar 27, 2023

View reviewed changes

jarolrod reviewed May 2, 2023

View reviewed changes

theStack changed the title ~~Remove confusing "Dust" label from coincontrol dialog~~ Remove confusing "Dust" label from coincontrol / sendcoins dialog Jul 3, 2023

theStack force-pushed the gui-nuke_cc_dust_label branch from 394300c to ef4185d Compare July 3, 2023 16:12

gui: send, left alignment for "bytes" and "change" label

a582b41

theStack force-pushed the gui-nuke_cc_dust_label branch from ef4185d to a582b41 Compare July 3, 2023 16:41

furszy approved these changes Jul 4, 2023

View reviewed changes

DrahtBot requested a review from hebasto July 4, 2023 13:27

hebasto approved these changes Jul 4, 2023

View reviewed changes

hebasto merged commit 7446cb1 into bitcoin-core:master Jul 4, 2023

theStack deleted the gui-nuke_cc_dust_label branch July 4, 2023 15:03

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 4, 2023

Merge bitcoin-core/gui#719: Remove confusing "Dust" label from coinco…

20d6750

bitcoin-core locked and limited conversation to collaborators Jul 3, 2024

Conversation

theStack commented Mar 13, 2023 • edited by hebasto Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Mar 19, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

theStack Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

furszy Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

theStack Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 8, 2023

Uh oh!

luke-jr commented Jul 1, 2023

Uh oh!

theStack commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

theStack commented Mar 13, 2023 •

edited by hebasto

Loading

DrahtBot commented Mar 13, 2023 •

edited

Loading

theStack commented Mar 22, 2023 •

edited

Loading

theStack commented Jul 3, 2023 •

edited

Loading