-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add system settings manager #1685
base: master
Are you sure you want to change the base?
Add system settings manager #1685
Conversation
2194b7d
to
6617c9f
Compare
src/assets/defaults.ts
Outdated
@@ -901,3 +907,90 @@ export const defaultSensorDataloggerProfile: OverlayGrid = { | |||
export const defaultDisplayUnitPreferences = { | |||
distance: DistanceDisplayUnit.Meters, | |||
} | |||
|
|||
export const defaultCockpitSettings = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked the idea of having all the defaults centralized in one place!
One thing thought is that we need to use those in the places where they are required (e.g.: calls to useStorage
and useBlueOsStorage
), otherwise if someone changes the value there, the value here will be deprecated and the code will not work.
src/assets/defaults.ts
Outdated
{ level: 'warning', enabled: true }, | ||
{ level: 'critical', enabled: true }, | ||
], | ||
'cockpit-mission-start-time': '2025-02-10T21:34:50.113Z', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's crucial that we don't change the default values from what they are today. This one should be new Date()
, and not that random date.
The same happens for several others. I believe you copied your own settings instead of the real defaults.
// Trying to sync automatically | ||
watch( | ||
() => userSettings.value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tip here: it looks like you're creating an extra localStorage entry to store a duplication of all variables just to be alerted by the useStorage
composable about changes in the value. You can instead just listen to the "storage" event. This way you don't duplicate everything (which can cause a lot of confusion for someone reading the stored values, as there would be two possible two sources of truth) and you can monitor for changes directly in the values of the keys.
addEventListener("storage", (event) => {});
Actually, from this code, I assume you're not receiving changes made to the original settings, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, I'm not receiving the changes.
Actually what I was trying to do there was to check for changes on the local copy of the settings. and if there was any change, save it to the storage.
I'll implement the event listener and check the results
6617c9f
to
8119e18
Compare
@@ -519,7 +533,6 @@ const tryToPrettifyRtcConfig = (): void => { | |||
const openTutorial = (): void => { | |||
interfaceStore.isMainMenuVisible = false | |||
interfaceStore.configComponent = -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To better understand, why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Was a leftover indeed!
Yes, makes sense to be on a separate PR.
Sad that our linter is partially broken and not catching all errors. I should revisit the PR that fixes it at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant commit and not PR 😅
8119e18
to
cb7c4d5
Compare
Show tutorial | ||
</v-btn> | ||
<v-btn | ||
v-if="isElectron()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if should not be removed. It guarantees we don't show the button to open the folder while in a browser, which doesn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a mistake I made. Nice that you saw. I was checking the menu with all buttons and forgot to put this back.
Returning now. Thanks
text: 'Reset settings', | ||
action: () => { | ||
localStorage.clear() | ||
location.reload() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will reload the page immediately, so the snackbar won't show or will do for a quick moment.
There's a utils method that deals with warning about the reload and setting a timeout so the user can read any message left. It can help here.
a8efdd6
to
9385ddd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good now.
Will just test it before approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have an edit button in the right to enable editing the value? Or maybe enabling it with a single click? Having to double-click requires the user to guess what to do. It's also not working on touch/mobile.
9385ddd
to
6039ec3
Compare
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got somewhat confused with the behavior of the edit button. When I click it, it says "Done" and puts a green checkmark. It seems like something was completed. Strange and unexpected for an edit button. Is the checkmark like a "save" button?
Kapture.2025-02-19.at.09.39.22.mp4
It's also unexpected to have different behaviors in those terms between the JSON variables and the primitive ones. Could we instead have the same for both?
A suggestion would be:
- Instead of the "Edit json" button, put a "{ ... }" there, as this was supposed to be the value, and we are not showing just because it doesn't fit there
- Use a pencil icon to start editing in all cases (json and not-json)
- Have the "Close" and "Save" buttons as the way to undo and save for all cases (json or primitive)
- Add a visual hint for the string and number values, as currently they don't seem to have entered an edit mode (different from the boolean and json ones, where this is clear)
With those implemented I think it would be much clear how to use the tool, as the behavior would be consistent.
Nice, I agree with your suggestions. |
Signed-off-by: Arturo Manzoli <[email protected]>
6039ec3
to
254cba4
Compare
On last patch:
Screenshare.-.2025-02-19.2_52_51.PM.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason it is showing this old "main-menu-style" variable (which was replaced by "cockpit-main-menu-style"), even with it not being in my localStorage
anymore.
Also, if i change any other variable, this old "main-menu-style" variable is being recreated.

.
Could we also add a clear button in the search input?
Screenshare.-.2025-02-14.9_18_13.AM.mp4
Closes #1486