Conversation
subdavis
force-pushed
the
client/626-atuosave
branch
from
b2aceb5 to
9bed314
Compare
| </v-card-actions> | ||
| </v-card> | ||
| </v-dialog> | ||
| </template> |
There was a problem hiding this comment.
Might hate me for suggesting this. I really like the inline async-ness and simplicity of the $prompt component and the 'prompt-> set flag->do one of multiple things' cycle is something that is done a lot. Should we invest in making a compositionAPI tool for this?
There was a problem hiding this comment.
Yes. It's used in 7 components.
Can we make this a separate issue thought? I think it's worth doing, but not as part of this.
There was a problem hiding this comment.
Yeah, I think it would be a separate thing.
There was a problem hiding this comment.
We should get rid of $snackbar as well. It's only used in once place, which can be replaced with just a regular v-snackbar
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
I couple of questions and the permissions error when trying to save someone else's data.
client/platform/desktop/constants.ts
Outdated
| export const JsonMetaCurrentVersion = 1; | ||
| export const SettingsCurrentVersion = 1; | ||
|
|
||
| export const DefaultOverlayOpacity = 0.95; |
There was a problem hiding this comment.
was this meant to be used? I don't see any external references to this const.
Also would we want to match the default vuetify dialog/prompt opacity for now. This looks better but the default I think is 0.46.
| try { | ||
| save = useHandler().save; | ||
| pendingSaveCount = usePendingSaveCount(); | ||
| } catch (err) { | ||
| /** | ||
| * Error indicates that this export dialog was opened in a place | ||
| * outside the viewer where pending changes aren't possible. | ||
| */ | ||
| } |
There was a problem hiding this comment.
Still going to get a Vue Warn on this obviously if in Home.vue. It's not the best indicator but the prop.small is only true when in Home.Vue so it may be used as a check against trying to access use(Handler|PendingSaveCout). It doesn't feel clean but could be used as additional check as well as catching the error.
There was a problem hiding this comment.
I got lazy. I don't like having weird side-effects based on seemingly unrelated props, so I added a new more explicit one.
There was a problem hiding this comment.
My long winded response was mostly because I felt weird using small to indicate that. A more explicit prop is a better idea.
| return getExportUrls(this.datasetId, this.excludeFiltered); | ||
| async function doExport({ forceSave = false, url }: { url?: string; forceSave?: boolean }) { | ||
| if (pendingSaveCount.value > 0 && forceSave) { | ||
| await save(); |
There was a problem hiding this comment.
On an error the save() function with automatically show an error prompt and cancel the saving of the file. The user hits okay and then it moves along in the function. I think we need to modify the save() function to respond back if it is successful or not here with a boolean value or perhaps the error. When it is not successful you want to return and prevent the rest of this doExport function from executing.
Easy example is to log in as another user and attempt to edit public dataset of a different user. On export without saving it will pop open the export prompt, you click on save it will pop open the error prompt indicating you don't have permissions. Clicking OK on that will cause you to fall through the rest of this function where it attempts to change the URL so another prompt comes up asking if you want to leave the page. If you click yes it will download the detections without your save in there.
There was a problem hiding this comment.
More of an error handling issue, I think. I resolved this by re-throwing the error after the prompt. I think it's more useful to have await save() live in try/catch and expose the error object that way than to have to pass back true/false and obscure the error from downstream handlers.
This case does get you "stuck" in a situation where there are pending changes you can't save so you can't export until you back out (and discard) and come back again, but I think that problem is out of scope for this issue. Adding some discard-changes feature would resolve this. I don't really like the idea of adding a special "discard and export", but I could maybe be talked into it.
What do you think?
There was a problem hiding this comment.
Yeah I like this, the stuck-ness I think is more a result of "not informing the user that their changes can't be saved until they attempt to save them" which is a different issue.
2 participants
fixes #626