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

[jarolrod]

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

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

[promag]

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

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

You can ditch

gui/src/qt/optionsdialog.cpp

Lines 91 to 92 in efa227f

	ui->thirdPartyTxUrlsLabel->setVisible(false);
	ui->thirdPartyTxUrls->setVisible(false);

now that it's under wallet tab.

[shaavan]

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!

[hebasto]

Concept ACK.

[hebasto]

bfb3037

"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?

[hebasto]

Please rebase :)

[jonatack]

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

[stratospher]

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)

2. In optionsmodel.cpp - Since Third-party transaction URLs come into picture only if ENABLE_WALLET is true,

• Move L78 to L115
• Move L348 to L344
• Move L478 to L473

[luke-jr]

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

[jarolrod]

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.

[DrahtBot]

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.

[DrahtBot]

🐙 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".}

[hebasto]

Closing due to a long inactivity here. Feel free to reopen.