Move third-party tx URL setting from Display to Wallet options tab #435

pull jarolrod wants to merge 1 commits into bitcoin-core:master from jarolrod:move-3partytx-setting changing 3 files +40 −40
  1. jarolrod commented at 7:13 am on September 27, 2021: member

    Out of all the settings in the Display tab, the Third-party transaction URLs is the odd one out. All other settings here affect the way the GUI is displayed. This action affects what context menu actions are available when wallet functionality is enabled, and we have a transaction. As such, it is a better fit for this to be in the Wallet tab.

    Not a part of #430 as it is not a direct improvement of the related code. Instead, it is a different opinion on the position of this setting.

    Screenshots of visual changes:

    master pr
    master-setting-placement pr-setting-placement

    Based on the last commit of #430 to not conflict.

  2. jarolrod added the label UI on Sep 27, 2021
  3. jarolrod added the label UX on Sep 27, 2021
  4. promag commented at 2:17 pm on September 28, 2021: contributor

    Concept ACK, agree wallet tab is a better place for this setting.

    Code review ACK 870393fd357f7fc2fedc3a3877b43f1daa736a01. Verified 2nd commit is move-only.

    You can ditch https://github.com/bitcoin-core/gui/blob/efa227f5df5f5a9669dec5f1d574cf22d3c0903f/src/qt/optionsdialog.cpp#L91-L92

    now that it’s under wallet tab.

  5. shaavan commented at 2:41 pm on September 28, 2021: contributor

    While I can understand the logic behind moving it under the wallet section, I don’t think the reason is strong enough to make sense of the change. Let me explain why I feel so:

    1. Logical Reason:
      • Though this option might be an odd one out under the Display tab. Still, the purpose of this option is to “Display” the transaction in third-party transaction explorer. To put it under the Wallet section doesn’t seem natural from a non-techie’s perspective.
    2. Pragmatic Reason:
      • This option has been under the Display tab for quite a while now (in fact, since it was introduced). To move it under the Wallet tab would introduce unnecessary confusion for a general user.

    However, I do have some suggestions to add upon for this PR, which might help form a solid point for this PR, assisting this change in being added.

    1. Rename the option’s name: To be honest, when I first read the options name, I was not immediately clear about its purpose, and I had to do some further research to understand it. This is a sign of a bad UX design. Rename it to something like “Third Party Transaction explorer” or something else (I am sure you can think of better names of the option than me, :D) and explain clearly in the tooltip what this option means. This will have two advantages:

      • First, it would better UX for this option.
      • Second, this would come as an upgraded version of the earlier setting rather than a mere transfer, which would make it seem logical to be moved under the Wallet Tab.

    2. Giving this option its own subsection: This setting is too different from other options under the Wallet Tab. To put it under the common subsection doesn’t seem to do justice with this setting. A subsection of its own, with a heading that clearly states this setting’s purpose and its relationship to Wallet in general, will significantly increase the understandability of this setting as well as make it seems obvious to be put under the Wallet section.

    These were my 2 cents on this PR. I hope these suggestions might help you make this PR better!

  6. hebasto commented at 9:08 am on September 29, 2021: member
    Concept ACK.
  7. hebasto commented at 9:14 am on September 29, 2021: member

    bfb3037acafc29827e0f2469a6f4c0ddaceab73a

    “Our usage of “third-party” should be hyphenated as it is being used as a modifier of both “URL” and “transaction URLs”.

    Sorry, but my English is poor to get it. Could you elaborate it?

  8. hebasto commented at 10:23 am on September 29, 2021: member
    Please rebase :)
  9. in src/qt/forms/optionsdialog.ui:224 in 870393fd35 outdated 
    215@@ -216,6 +216,33 @@
216          </property>
217         </widget>
218        </item>
219+       <item>
220+        <layout class="QHBoxLayout" name="horizontalLayout_3_Display">
221+         <item>
222+          <widget class="QLabel" name="thirdPartyTxUrlsLabel">
223+           <property name="toolTip">
224+            <string>Third-party URLs (e.g. a block explorer) that appear in the transactions tab as context menu items. %s in the URL is replaced by transaction hash. Multiple URLs are separated by vertical bar |.</string>
    jonatack commented at 10:35 am on September 29, 2021:

    (change in both the tooltips if taken)

    0            <string>Third-party URLs (e.g. a block explorer) that appear in the transactions tab as context menu items. %s in the URL is replaced by the transaction hash. Multiple URLs are separated by a vertical bar |.</string>

    (Feel free to ignore: I wonder if %s would be better as "%s" to help it not appear to be a bug like "Dear {$customer_name}")

    jonatack commented at 10:57 am on September 29, 2021:
    Looks like while doing this you could add a missing hyphen to “third party tool” in src/qt/forms/optionsdialog.ui:192.
    jarolrod commented at 3:22 am on October 29, 2021:
    Thanks for the review, took all the suggestions!
  10. jonatack commented at 10:45 am on September 29, 2021: contributor

    Tested ACK.

    “Third-party” is for an adjective that modifies a subject/noun and “third party” for a subject/noun. So this seems correct. Info: https://www.merriam-webster.com/dictionary/third-party.

    I wonder if it would be marginally better UI to group the checkboxes together and the user input boxes together (tx urls and script path).

  11. DrahtBot added the label Needs rebase on Sep 29, 2021
  12. stratospher commented at 8:29 pm on October 2, 2021: contributor

    Concept ACK. In agreement with Third-party transaction URLs being a better fit for the wallet tab.

    We’ll also need to make the following changes:

    1. In optionsdialog.cpp
    • Move L217(from display section) to L211(wallet section).
    • Move L270(from display section) to L239(wallet section)
    1. In optionsmodel.cpp - Since Third-party transaction URLs come into picture only if ENABLE_WALLET is true,

    unnamed

  13. luke-jr commented at 7:16 pm on October 3, 2021: member

    Concept NACK, I think the Display tab makes more sense since it’s about the GUI not the wallet itself.

    Additionally, it may be useful outside wallet stuff when (eg) #444 finally gets merged

  14. qt: move third-party tx URL setting from Display to Wallet options tab 
    Out of all the settings in the Display tab, the 'Third-party
transaction URLs' is the odd one out. All other settings here
affect the way the GUI is displayed. This action affects what
context menu actions are available when wallet functionality
is enabled, and we have a transaction. As such, it is a better
fit for this to be in the Wallet tab.
    08291806a0
  15. jarolrod force-pushed on Oct 29, 2021
  16. jarolrod commented at 3:24 am on October 29, 2021: member

    updated from 870393f -> 0829180

    Changes:

    • addressed @jonatack and @stratospher comments @luke-jr : for now we don’t have a usecase for this outside of the wallet. #444 is a long way from being ready. If you decide to revisit it and get it ready,then i’d be happy to drop this.
  17. DrahtBot removed the label Needs rebase on Oct 29, 2021
  18. DrahtBot commented at 10:54 am on December 30, 2021: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #556 (refactor: Make BitcoinUnits::Unit a scoped enum by jb55)
    • #440 (Do not require restart if overridden option is modified by hebasto)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  19. DrahtBot commented at 11:51 am on April 15, 2022: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  20. DrahtBot added the label Needs rebase on Apr 15, 2022
  21. hebasto commented at 4:19 pm on May 26, 2022: member
    Closing due to a long inactivity here. Feel free to reopen.
  22. hebasto closed this on May 26, 2022
  23. bitcoin-core locked this on May 26, 2023


jarolrod promag shaavan hebasto jonatack stratospher luke-jr DrahtBot

Labels
Needs rebase UX UI

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-23 01:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me