Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settings that are overridden #44

Open
jarolrod opened this issue Feb 6, 2023 · 14 comments
Open

Settings that are overridden #44

jarolrod opened this issue Feb 6, 2023 · 14 comments
Assignees
Labels

Comments

@jarolrod
Copy link
Collaborator

jarolrod commented Feb 6, 2023

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

@GBKS
Copy link
Contributor

GBKS commented Feb 6, 2023

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

image

@yashrajd
Copy link

yashrajd commented Jul 5, 2023

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
Copy link

yashrajd commented Jul 5, 2023

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

@GBKS
Copy link
Contributor

GBKS commented Jul 6, 2023

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

@yashrajd
Copy link

yashrajd commented Jul 7, 2023

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

@yashrajd
Copy link

yashrajd commented Jul 14, 2023

@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
Copy link

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

@GBKS
Copy link
Contributor

GBKS commented Jul 19, 2023

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

@ryanofsky
Copy link

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.

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.

@yashrajd
Copy link

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 .

@ryanofsky
Copy link

re: #44 (comment), 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."

@yashrajd
Copy link

yashrajd commented Aug 2, 2023

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 Warn that settings changed while node is running will not take into affect until the next startup #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

@GBKS
Copy link
Contributor

GBKS commented Aug 2, 2023

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
Copy link
Contributor

GBKS commented Jan 22, 2025

Alright, let's bring this issue to the finish line. First, I'd like to refocus on the original issue, when a setting is overridden by a command-line parameter. This is not related to changed settings requiring a restart (covered in #43), and also does not longer apply to bitcoin.conf per Ryan's comment.

Per this comment, the use case is testing and debugging by more experienced users.

The simplest solution I can think of is this:

Image

Each field tells you whether it's overridden or not. You can't edit it. That's it.

What do you think?

@GBKS GBKS self-assigned this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

4 participants