Bitcoin-Core-App icon indicating copy to clipboard operation
Bitcoin-Core-App copied to clipboard

Settings that are overridden

Open jarolrod opened this issue 2 years ago • 14 comments
trafficstars

Settings can be overridden either by passing command line options or by being defined in a bitcoin.conf. If it is overridden, then the value for the setting that is in settings.json will be ignored.

This is the warning that the qt widgets gui gives in such a case: Screen Shot 2023-02-06 at 1 23 50 AM

How should we handle such a case? I think we could just disable the icon and include a description within the setting row that this setting is overridden

jarolrod avatar Feb 06 '23 06:02 jarolrod

Something like this? These are two options, with the right option being more explicit.

image

GBKS avatar Feb 06 '23 10:02 GBKS

If a setting is overridden from CLI or conf file, can we just update the UI to display the right information?

Greying out the setting & icon etc. makes sense, add the help text inside the box saying "This setting has been set from the command line/.conf file"

yashrajd avatar Jul 05 '23 18:07 yashrajd

The options that do not have a UI item can be listed in the way Qt app does it currently, possibly in the Developer Options section...

yashrajd avatar Jul 05 '23 18:07 yashrajd

The greying out works. Very subtle, but should work. I'd only grey out the actual value itself, not the item text on left.

GBKS avatar Jul 06 '23 09:07 GBKS

I'd like to work on this, please assign it to me.

yashrajd avatar Jul 07 '23 02:07 yashrajd

@jarolrod is it possible to identify and show where the setting is getting overridden from? It'd be great to not just show that a setting is being overridden but let users potentially action it.

edit: also need to confirm that we can display the actual overriding value in the UI?

yashrajd avatar Jul 14 '23 03:07 yashrajd

Screenshot 2023-07-17 at 22 33 21 Screenshot 2023-07-17 at 22 34 49

yashrajd avatar Jul 18 '23 02:07 yashrajd

Thanks for creating those mock-ups. I find the language too technical. Let's try to avoid terms like CLI and .conf. It's also not clear what they mean. Do the tags indicate that you can override the setting that's in the conf file? Or is it the other way around?

Maybe just disable those options, add a small info circle, and repeat that the bottom of the screen with a message like "These options are defined in your settings can file and cannot be changed here."?

GBKS avatar Jul 19 '23 11:07 GBKS

As of https://github.com/bitcoin-core/gui/pull/602 in bitcoin-qt, GUI settings that have been changed override settings in the bitcoin.conf file. The bitcoin.conf file just provides default settings values that can be customized in the GUI.

CLI settings still override GUI settings so they can be used for debugging and testing. CLI settings should be temporary and probably non-technical users should never encounter them. It seems fine to disable settings in the GUI that are overridden by the command line. Also fine to allow them to be set but note that they won't have an effect until bitcoin is restarted.

ryanofsky avatar Jul 19 '23 13:07 ryanofsky

Thanks for the correction @ryanofsky. Let's see if I understood that currently, and what it implies:

As of bitcoin-core/gui#602 in bitcoin-qt, GUI settings that have been changed override settings in the bitcoin.conf file. The bitcoin.conf file just provides default settings values that can be customized in the GUI.

  • settings changed from GUI override .conf file values. This means the bitcoin.conf override label is not required, neither is disabling control (like toggle or text-field).

CLI settings still override GUI settings so they can be used for debugging and testing. CLI settings should be temporary and probably non-technical users should never encounter them. It seems fine to disable settings in the GUI that are overridden by the command line. Also fine to allow them to be set but note that they won't have an effect until bitcoin is restarted.

If a setting is changed from CLI, we can simply show the updated value in the GUI and inform the user of the fact (retain the CLI override label, with better language). We do not disable the control and let users try and change it. If they to attempt to change the value, we update it in the GUI along with the note that restart is needed, per #43 .

yashrajd avatar Jul 25 '23 20:07 yashrajd

re: https://github.com/BitcoinDesign/Bitcoin-Core-App/issues/44#issuecomment-1650541231, yes that's all correct.

Personally, I don't think the "CLI override" label by itself really explains very much, and it is potentially confusing because it isn't clear about whether the displayed value is the saved value which will take effect next restart, or the current effective value which comes from the command line.

IMO, the ideal interface would not have a "CLI override" tag, but instead would have a "Pending" tag. The pending tag would be shown next to any value which was saved to settings, but will not take effect until the next restart. To be even more explicit, below each pending setting there could be a little note like: "The value above is saved and will take effect the next time this application is started. The current effective value is XXX." If desired, in the case of a CLI override, the note could be even more specific and say "The current effective value is XXX, which was specified on the command line."

ryanofsky avatar Jul 31 '23 15:07 ryanofsky

Implemented both your & @GBKS 's suggestions in a way that (hopefully) reflect the following learnings:

  • GUI values now intuitively override bitcoin.conf values, so they don't need any comment
  • text labels don't work for overridden values from cli (use coloured status icons instead, explain in help-text)
  • these status icons also work for showing restart requirement ( see #43 )
  • we don't have to disable overridden settings, we can simply show updated value and allow user to attempt to change it. (don't disable overridden field, just use a status icon instead)
Screenshot 2023-08-01 at 21 27 40

yashrajd avatar Aug 02 '23 01:08 yashrajd

Nice! I like the simplicity of this. Small tweak could be to remove "Bitcoin Core" from the label on the right, so it would just be "Restart to apply this change".

There is as balance of how explicit we want/need to be. Yashraj's solution is more minimal, while Ryan's is more expressive with full sentences that describe the change. Personally, I lean more towards the minimal one.

Another thing to consider is whether we want to give an option to revert changes. Not 100% sure this is necessary. We can take our approach again where we start with a simple solution and only add more complexity if people ask for it (or run into problems). That way we don't overcomplicate right from the start due to defensive thinking.

Yashraj, have you looked for established patterns on this from other applications or operating systems? I can't think of any examples spontaneously, but there is probably some material to borrow from out there.

GBKS avatar Aug 02 '23 11:08 GBKS