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

feat: adds Save Config & Restart button #726

Merged

Conversation

pedrolamas
Copy link
Member

@pedrolamas pedrolamas commented Jun 21, 2022

Adds a new "Save config & restart" button that will call SAVE_CONFIG if there are pending configuration changes.

Wide

image

Medium

image

Small

image

Preview popup

image

Also adds tooltips to the other app bar buttons:

image

image

Signed-off-by: Pedro Lamas [email protected]

@pedrolamas pedrolamas added the UI - QoL Improvements to the UI label Jun 21, 2022
@pedrolamas pedrolamas added this to the 1.19 milestone Jun 21, 2022
@pedrolamas pedrolamas requested a review from matmen June 21, 2022 15:52
@matmen
Copy link
Member

matmen commented Jun 21, 2022

I'm not sure if I actually like this; isn't there already a save button next to all the config changing actions (bed mesh leveling, z-offset)? Mesh leveling also writes a notice to the console, for example.

This also breaks an important aspect of the UI: the e-stop button gets moved conditionally which breaks muscle memory. Either the button would have to be placed to the left of the e-stop, or it would have to be always shown (and maybe disabled if no config change can be saved):
image

@pedrolamas
Copy link
Member Author

Agreed, the current UX is bad... switching the two buttons probably makes more sense than the current approach (personally, disabling the button just to maintain the position doesn't feel right), or showing it elsewhere....

As for the existing "Save" buttons for bed mesh leveling, z-offset, etc., these are based on state of those specific functions.

The state of this button is based on the value of the Klipper configfile.save_config_pending flag, meaning that even manual invocation of commands such as MANUAL_PROBE, Z_ENDSTOP_CALIBRATE, PROBE_CALIBRATE, DELTA_CALIBRATE (that are not available in Fluidd UI, but will show the SAVE_CONFIG on command line)

There is also a new configfile.save_config_pending_items that was recently introduced in Klipper and this has all the pending changes that will be committed on SAVE_CONFIG

@matmen
Copy link
Member

matmen commented Jun 23, 2022

As this is only for manually invoked commands (from what I understood), maybe it makes sense to put it in the header of the console card? It doesn't really only affect the console, of course, but it would be a subtle way to place it. It feels kind of out of place in the main header. Lmk what you think :)
Additionally, I think a tooltip or modal that shows which settings have been changed (/ will be saved) would be nice (we already have the data; though it of course isn't necessary).

@pedrolamas
Copy link
Member Author

As this is only for manually invoked commands (from what I understood), maybe it makes sense to put it in the header of the console card? It doesn't really only affect the console, of course, but it would be a subtle way to place it. It feels kind of out of place in the main header. Lmk what you think :)

Honestly don't know... my point here was not to be subtle and ensure we "remember" the user that he has pending changes that have not been saved yet!

Additionally, I think a tooltip or modal that shows which settings have been changed (/ will be saved) would be nice (we already have the data; though it of course isn't necessary).

Very good point, I will add this in! 😁

@pedrolamas
Copy link
Member Author

How about something like this?

image

Adding a button to Console doesn't feel right to me (these changes can be caused by macros!), but I was thinking that if the top is not an option, we could instead use the status card (2nd position on the screenshot)

@matmen
Copy link
Member

matmen commented Jun 23, 2022

I think having it at the top formatted like a button like in your screenshot looks good 👍

@pedrolamas
Copy link
Member Author

@matmen PR updated with the changes we discussed. For now it is using a textarea for the changes preview as I had some issues ensuring all nodes were open on popup!

@pedrolamas pedrolamas force-pushed the pedrolamas/save-config-button branch from 3f7ac07 to c0f90ce Compare June 28, 2022 16:08
Copy link
Member

@matmen matmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure about the text box tbh, but I've tried it and it's functional, so I'm fine with that :)

@pedrolamas pedrolamas merged commit 68809bf into fluidd-core:develop Jun 30, 2022
@pedrolamas pedrolamas deleted the pedrolamas/save-config-button branch June 30, 2022 18:28
@pedrolamas
Copy link
Member Author

I've merged as-is, but we can revisit and test other layouts before we release the next version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI - QoL Improvements to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants