-
Notifications
You must be signed in to change notification settings - Fork 30
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(ui): Allow specifying whether local or all snapshots are shown by default #186
Conversation
ffdf7d7
to
1439dc8
Compare
I put this comment in the kopia PR, but I'll add it here too as any changes are probably more relevant here: Solutions: keep the code as is |
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.
Hi @PhracturedBlue, @jkowalski,
i've tested this pr. Basically its working.
I've just got a question. Is it possible to store the selectedOwner instead of a boolean? As of now, we need to override the selectedOwner if it is null - based on the boolean that has been set by the user or using the default (true-> all snapshots, false-> local snapshots).
Saving the specific selection seems more intuitive, as the old selection will be set every time we open the snapshot tab.
What do you think?
Cheers,
src/PreferencesView.jsx
Outdated
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 looks good to me.
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.
Hi @PhracturedBlue,
there is a lint error in line 222:3. Can you fix it? Additionally, there is console log within the code that should be removed.
Right now we need to override the selectedOwner in the rendering function, which basically throws the above mentioned lint error. For the first rendering, the state is not changed. The second rendering cycle then uses the set selectedOwner.
Can we fix this?
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.
LGTM
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.
We should extend the interface SerializedUIPreferences to reflect the new attribute defaultSnapshotViewAll.
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.
LGTM
I considered allowing selection of any user instead of just the all or local. It didn't make sense to me to save this setting, since normally I'm just using it as a filter, and not as a permanent setting. Whereas the 'Local' setting is of no use to me as I don't have any local snapshots. It is also a little weird to me to use what is effectively a filter as a preference and store that on the server. It would perhaps be better to store in in a local-store, but I don't see any enabling for that. If we do go this way, it eliminates the need for altering the preferences screen, so maybe it is overall more inuitive. Anyhow, I can store the value as a username. we'll need to validate that the user exists each time, so I'm not sure it will actually simplify the code any though. Please confirm you prefer preserving the last-selection, and I'll make the changes. Looks like @jkowalski already merged the boolean parameter, so we'll need a new pull-requrst into kopia for this change. |
Hi @PhracturedBlue, then we should just fix the code and implement it as it is - with the current logic. What you you think @jkowalski? Cheers, |
One further option I thought of: That has the benefit of not requiring a server-only configuration section in the preferences. |
1439dc8
to
caa0d66
Compare
I ended up removing the configuration option, as the more I thought about it the less it seemed helpful. |
caa0d66
to
9c9e9b2
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.
The changes look good to me. The selector now saves the last state - all or local snaphots.
Cheers
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.
LGTM
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.
LGTM
When I connect to the server http page, I prefer to see all snapshots shown by default. every time I switch away and come back I need to reselect 'All' instead of 'Local'. This just makes the default a preference option
Requires kopia/kopia#3289