Remove confusing “Dust” label from coincontrol / sendcoins dialog #719

1. 
   theStack commented at 10:47 pm on March 13, 2023: contributor

   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.

2. 
   DrahtBot commented at 10:47 pm on March 13, 2023: contributor

   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.

3. hebasto renamed this:
   qt: remove confusing "Dust" label from coincontrol dialog
   Remove confusing "Dust" label from coincontrol dialog
   on Mar 14, 2023
4. hebasto added the label Wallet on Mar 14, 2023
5. 
   furszy commented at 0:47 am on March 19, 2023: member

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

6. theStack force-pushed on Mar 19, 2023
7. 
   theStack commented at 1:37 am on March 19, 2023: contributor

   @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).
8. 
   furszy commented at 11:04 pm on March 19, 2023: member

   While you are here, could also align the “bytes” and “change” labels to left. Feel free to squash this: https://github.com/furszy/bitcoin-core/commit/53165e3cc02cf4fdff2b274ad98e1868196e9644
9. 
   theStack commented at 1:59 am on March 22, 2023: contributor

   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.

10. 
    furszy commented at 3:12 am on March 22, 2023: member

    ACK 394300c1
11. in src/qt/forms/sendcoinsdialog.ui:205 in 394300c188 outdated

    200@@ -201,6 +201,9 @@
    201              </property>
    202              <item>
    203               <layout class="QFormLayout" name="formLayoutCoinControl1">
    204+               <property name="labelAlignment">
    205+                <set>Qt::AlignLeading|Qt::AlignLeft|Qt::AlignVCenter</set>
    

    ---

    ---

    

    hebasto commented at 1:19 pm on March 27, 2023:

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

    ---

    

    theStack commented at 4:19 pm on July 3, 2023:

    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:

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

    ---

    

    furszy commented at 4:34 pm on July 3, 2023:

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

    ---

    

    theStack commented at 4:42 pm on July 3, 2023:

    @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.
12. 
    hebasto commented at 1:20 pm on March 27, 2023: member

    ACK 394300c188621732bb43357ed359c1bfd9cbba6a

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

13. 
    jarolrod commented at 4:00 pm on May 2, 2023: member

    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.

14. 
    maflcko commented at 10:47 am on June 8, 2023: contributor

    Concept ACK
15. 
    luke-jr commented at 9:49 pm on July 1, 2023: member

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

    210ef1e980
17. theStack renamed this:
    Remove confusing "Dust" label from coincontrol dialog
    Remove confusing "Dust" label from coincontrol / sendcoins dialog
    on Jul 3, 2023
18. theStack force-pushed on Jul 3, 2023
19. 
    theStack commented at 4:15 pm on July 3, 2023: contributor

    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):

     0$ git range-diff 394300c18...ef4185d2b
     11:  944263a93 ! 1:  210ef1e98 qt: remove confusing "Dust" label from coincontrol dialog
     2    @@ Metadata
     3     Author: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
     4
     5      ## Commit message ##
     6    -    qt: remove confusing "Dust" label from coincontrol dialog
     7    +    qt: remove confusing "Dust" label from coincontrol / sendcoins dialog
     8
     9         In contrast to to all other labels on the coin selection dialog, the
    10         displayed dust information has nothing to do with the selected coins.
    11    @@ Commit message
    12         As the label clearly doesn't help the user and is, quite the contrary,
    13         rather increasing confusion/misguidance, it seems sensible to remove it.
    14
    15    +    Also, remove the label from the sendcoins dialog with the same rationale.
    16    +
    17      ## src/qt/coincontroldialog.cpp ##
    18     @@ src/qt/coincontroldialog.cpp: CoinControlDialog::CoinControlDialog(CCoinControl& coin_control, WalletModel* _m
    19          QAction *clipboardFeeAction = new QAction(tr("Copy fee"), this);
    202:  394300c18 = 2:  ef4185d2b gui: send, left alignment for "bytes" and "change" label
    

20. gui: send, left alignment for "bytes" and "change" label a582b4141f
21. theStack force-pushed on Jul 3, 2023
22. furszy approved
23. 
    furszy commented at 1:27 pm on July 4, 2023: member

    ACK a582b41
24. DrahtBot requested review from hebasto on Jul 4, 2023
25. hebasto approved
26. 
    hebasto commented at 2:50 pm on July 4, 2023: member

    Looks good. ACK a582b4141f0756faa3793fb1c772898a984c83e4.
27. hebasto merged this on Jul 4, 2023
28. hebasto closed this on Jul 4, 2023

    ---

29. theStack deleted the branch on Jul 4, 2023
30. sidhujag referenced this in commit 20d6750a06 on Jul 4, 2023
31. bitcoin-core locked this on Jul 3, 2024

Contributors
theStack DrahtBot furszy hebasto jarolrod maflcko luke-jr

Labels
Wallet