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

Warn that settings changed while node is running will not take into affect until the next startup #43

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

Comments

@jarolrod
Copy link
Collaborator

jarolrod commented Feb 6, 2023

When a user makes changes to a setting after onboarding and when the node is running, those settings will not take into affect until the next node restart. The UI should provide a clear warning about this to the user.

In the Qt Widgets GUI this is accomplished like so, notice the red warning at the bottom of the screenshot:
Screen Shot 2023-02-06 at 1 00 10 AM

I think we could do something like the following in the screenshots below. Whenever a setting is changed, the main node setting page will display a warning at the bottom. And the gear icon in the setting screen can show a warning icon.

gear warning node setting page warning
setting-warning-a Screen Shot 2023-02-06 at 1 05 29 AM
@GBKS
Copy link
Contributor

GBKS commented Feb 6, 2023

Here's a mock-up. I would not use red, seems too much like an error. I think it's also important that the copy makes it clear that it's about the changes the user has just made. It should not be passive or indirect ("...this change would require...", "Changes to settings...").

image

A slightly unrelated question here. Should there be 3 options for the mode: Light, Dark, and Automatic (= based on OS settings)?

@GBKS GBKS added this to the 1.1 Design milestone Jun 29, 2023
@GBKS GBKS added the design label Jun 29, 2023
@yashrajd
Copy link

yashrajd commented Jul 5, 2023

A few suggestions:

  • the warning should appear when & where the user changed the setting, not just on the main settings page.
  • the changed setting should be highlighted/greyed until the change the applied on restart.
  • additionally, the warning could also contain a restart button so the user can take the action immediately (is this possible

For mobile: what does the restart requirement mean? Most users rarely/never quit an app on their phone. Can we relaunch the app for the user?

Happy to do mockups as needed...

@GBKS
Copy link
Contributor

GBKS commented Jul 6, 2023

Good suggestions. Let's start with the logic.

  1. Which specific changes require a restart?
  2. Can we trigger a restart via a button press, both on desktop and mobile?

@rabbitholiness
Copy link
Contributor

In short: any changes that touch Bitcoin :-)

@yashrajd
Copy link

Yes, and it makes total sense. Wonder if there are any exceptions/additional settings...

@jarolrod /@johnny9 will also look into the restart query and get back.

I'd like to work on this issue...

@yashrajd
Copy link

If I recollect correctly, @jarolrod said today that restart is possible but that would require some reworking of the codebase. So for now, we design for & communicate manual restart. Please correct if this is inaccurate in any way.

@GBKS
Copy link
Contributor

GBKS commented Jul 20, 2023

While we're at it, let's design both versions, and keep the one we want eventually as a future issue to tackle. That way we don't have to come back to thinking through this specific issue months down the road.

@yashrajd
Copy link

yashrajd commented Aug 2, 2023

  • Uses the same pattern as values updated from elsewhere (CLI). See Settings that are overridden  #44.
  • For when user edits a setting that needs restart to apply, shown immediately when the user does it, on the same page as the setting.
  • With and without Restart buttons.
Screenshot 2023-08-01 at 21 59 16

@jarolrod
Copy link
Collaborator Author

@GBKS do we want to go with @yashrajd suggestion here? If so can this go in the main design file? I'm working on a rework of optionsmodel to be able to take into account:

  1. All configuration values, and where they come from -> so we can show where a value is overriden from
  2. If a setting is modifable or not - a setting is modifiable if it is not overridden by bitcoin.conf or a terminal parameter
  3. Knowledge of when a value has changes, but a restart is required for the value to be applied

Will look something like this:

image

@yashrajd
Copy link

yashrajd commented Dec 1, 2024

Hi @GBKS, it's great to see the great progress Jarol & co. made on this... would love to see it get done.

@GBKS
Copy link
Contributor

GBKS commented Jan 15, 2025

In conversation with @pablomartin4btc around the proxy screen here, I proposed a solution that I also prototyped here.

  • There's a greyed-out notification on the top of the screen, informing the user that changes require a restart
  • When the user makes changes, and now has unsaved changes, the notification lights up
  • The top bar has two buttons, Back and Done, matching the iOS mobile pattern shown here. Back cancels out, and Done saves
  • If possible, Done also shows a model that asks the user whether they want to restart now, and the application could restart itself. If that is technically possible, I'd slightly adjust the copy in the prototype

Generally, especially on mobile, it is pretty common in many applications that you don't need to have an explicit save button. But sometimes you do, like here.

What do you think?

@pablomartin4btc
Copy link

pablomartin4btc commented Jan 15, 2025

@GBKS, I do agree with points, tested them all on your prototype (except ofc for the restart, we can discuss it later).

Tested invalid IP address format on prototype -> screenshot ("Done" button is disabled - it doesn't allow to save).

image

  • Perhaps the notification on the top of the screen (informing the user that changes require a restart) could be greyed out back again in this case?
Tested correct IP address format on prototype -> screenshot ("Done" button is enabled - it allows to save).

image

Many thanks!

(P.S.: small observation, the "back" button returns to the "Settings" page, not the one strictly before which is "Connection Settings" page.)

@GBKS
Copy link
Contributor

GBKS commented Jan 20, 2025

Perhaps the notification on the top of the screen (informing the user that changes require a restart) could be greyed out back again in this case?

Thanks for testing. I'd keep the notification lit up. You still have unsaved changes, even if they are not acceptable.

@yashrajd
Copy link

In conversation with @pablomartin4btc around the proxy screen here, I proposed a solution that I also prototyped here.

Love these prototypes <3 one update I'll suggest is that if (for eg) IP/port field is left empty after enabling the toggle, it should trigger an error message saying something like "Enter the proxy address in the IP:port format"

  • There's a greyed-out notification on the top of the screen, informing the user that changes require a restart

This is a new for me, where the message is shown even if the user hasn't made any change. Qt and our previous approach (see mockups above) is to only show the message once a change is made, think I might prefer that.

  • When the user makes changes, and now has unsaved changes, the notification lights up
  • The top bar has two buttons, Back and Done, matching the iOS mobile pattern shown here. Back cancels out, and Done saves

I've been burnt by this pattern in macOS electron apps where you must click on Save after editing something to apply changes and if you forget to do that you lose changes. That's not something we're used to nowadays (word processors, email clients, most mobile apps auto-save) as you mentioned too.

If we must use this pattern, how about showing a modal with "Save" and "Discard" buttons when the user has made changes but clicks on Back?

  • If possible, Done also shows a model that asks the user whether they want to restart now, and the application could restart itself. If that is technically possible, I'd slightly adjust the copy in the prototype

Perhaps "Done" can be changed to something like "Save & Restart"?

Generally, especially on mobile, it is pretty common in many applications that you don't need to have an explicit save button. But sometimes you do, like here.

The problem is the user might not know/understand which pattern is applicable when, or simply forget to click on Save/Done

What do you think?

@GBKS
Copy link
Contributor

GBKS commented Jan 22, 2025

if (for eg) IP/port field is left empty after enabling the toggle, it should trigger an error message saying something like "Enter the proxy address in the IP:port format"

This seems like it should be done when trying to save. Having error messages all the time for empty fields is a bit annoying.

This is a new for me, where the message is shown even if the user hasn't made any change. Qt and our previous approach (see mockups above) is to only show the message once a change is made, think I might prefer that.

Logic is that this screen has a unique interaction model that we want to be explicit about.

If we must use this pattern, how about showing a modal with "Save" and "Discard" buttons when the user has made changes but clicks on Back?

Good idea.

Perhaps "Done" can be changed to something like "Save & Restart"?

Too long on mobile. We already have a very clear and explicit message at the top of the screen.

The problem is the user might not know/understand which pattern is applicable when, or simply forget to click on Save/Done

Not a big deal if they do. This seems overly cautious. There are no funds at risk here. It's 1-2 input fields, so not much effort. We're very explicit with the message at the top.


Overall, considering this issue has been open for a year now, I'd really like us to roll with this and focus on getting the implementation right. If someone wants to propose a different interaction model, I'd ask you to take on this whole design task altogether and make it happen. Sorry if that sounds snappy, but this is interaction model works, and we're dealing with a nested settings screen that is not something 95% of users will have crucial interactions with.

@GBKS GBKS self-assigned this Jan 24, 2025
GBKS added a commit that referenced this issue Jan 24, 2025
Brings the mock-ups in line with the recent work in issues and PRs.

For max upload limits:
#8

For the proxy screen:
#43
bitcoin-core/gui-qml#438
GBKS added a commit that referenced this issue Jan 27, 2025
Brings the mock-ups in line with the recent work in issues and PRs.

For max upload limits:
#8

For the proxy screen:
#43
bitcoin-core/gui-qml#438
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

5 participants