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

Improve UX for the Force Upload [one time force upload] #1035

Closed
borekb opened this issue Sep 5, 2019 · 10 comments
Closed

Improve UX for the Force Upload [one time force upload] #1035

borekb opened this issue Sep 5, 2019 · 10 comments

Comments

@borekb
Copy link

borekb commented Sep 5, 2019

I'm probably hitting an issue created by "Safe Upload" feature #923 (cc @arnohovhannisyan) as I just updated keybindings.json and got this:

Screenshot 2019-09-05 at 10 30 14

I currently use a single computer, the Gist is 14 days old and I have just updated keybindings.json, so I think it's a bug.

Even if it isn't a bug, I'd like to comment on the UX in general because I find it relatively confusing:

  • The dialog doesn't tell me which of the two cases I'm in. Same versions mean no risk in forcing an upload; newer Gist mean high risk of overwriting my data.
  • Even if I wanted to forcibly upload the settings now, I don't like that the way to do it is to permanently enable forceUpload in my settings. I'd much rather just invoke a one-time "Force Upload" command and be warned next time again (because, again, one of the scenarios is high-risk).

🌴 Visual Studio Code Version : 1.38.0
🌴 Code Settings Sync Version : 3.4.2

@borekb
Copy link
Author

borekb commented Sep 5, 2019

Oh, I clicked "Yes", it added "sync.forceUpload": true to my settings.json but the updated Gist does not contain this. So my local settings.json are immediately out of sync with the Gist 😕.

@borekb
Copy link
Author

borekb commented Sep 5, 2019

Hmm, not sure if this is about keybindings.json after all, see Extension always asks to enable Force Upload #1016.

I'm not sure if this is an additional issue beside #1016 or a duplicate of it...

@karl-lunarg
Copy link
Contributor

Yes, version 3.4.2 definitely had a regression. PR #1026 was just approved for 3.4.3 which fixes this particular refusal to upload, but the PR does not change the UI.

BTW, I've been working around the refusal to upload problem by going ahead and letting the dialog set Force Upload and then immediately resetting Force Upload to off in my local settings. I'm sort of glad that it didn't update the Force Upload setting in the Gist. In any case, this should be better in the next release.

As far as the UX goes, I agree with @borekb. I like how you get a status message when you try to Download but already have the same settings as the Gist. Perhaps the same should be done for Upload.

The Upload path in the extension compares the local settings with the Gist settings and therefore knows if they are different or not.

The Upload logic could be:

  • If the local and Gist are the same, display a 3-second status message saying so and don't do anything.

  • If the local and Gist are different and the local LastDownload is earlier than the Gist LastUpload, then display a popup dialog warning that there is a risk in overwriting changes in the Gist. Don't say in the dialog that they could be identical because they are not. Don't do anything, but perhaps offer the same option to force the upload without changing the local ForceUpload setting.

  • If the local and Gist are different and the local LastDownload is later than or same as the Gist LastUpload, then upload the settings with no popup dialog. Perhaps add a 3-second status message saying that the settings were uploaded. I think that the upload details are also shown in the console.

It would be nice if this sort of UX improvement could be made for the 3.4.3 release if @shanalikhan and @borekb are in agreement.

@shanalikhan
Copy link
Owner

I'd much rather just invoke a one-time "Force Upload" command and be warned next time again

Yes, i agree with @borekb we can add one more button in this popup.

  1. Yes
  2. Always Force Upload ( text can be improved )
  3. Don't Show This again.

@karl-lunarg

The Upload logic could be...

Yes, the first 2 of 3 you have mentioned were kept in mind when this feature was originally implemented.
These are 3 scenarios we can cover. I hope the PR that is merged recently should be covering these scenarios.

As v3.4.3 is not released yet, if you can send PR that contain the updated button for the updated UX and verifying the above 3 scenarios you mentioned, v3.4.3 would be concrete release :)

@shanalikhan shanalikhan changed the title Upload being refused after updating keybindings.json Improve UX for the Force Upload [one time force upload] Sep 5, 2019
@borekb
Copy link
Author

borekb commented Sep 5, 2019

If the local and Gist are different and the local LastDownload is earlier than the Gist LastUpload, then display a popup dialog warning that there is a risk in overwriting changes in the Gist. Don't say in the dialog that they could be identical because they are not. Don't do anything, but perhaps offer the same option to force the upload without changing the local ForceUpload setting.

This is the key 👍 . Well-articulated, @karl-lunarg! I fully agree with your description and suggestions.

@karl-lunarg
Copy link
Contributor

Yes, i agree with @borekb we can add one more button in this popup.

  1. Yes
  2. Always Force Upload ( text can be improved )
  3. Don't Show This again.

Since we're talking about changing the UX anyway, how about making it simpler instead of more complex and potentially confusing?

I'm thinking that the popup should just say: "The settings in the Gist were changed after you last downloaded them. Upload anyway?". And there would be two buttons: Yes and No. Closing the popup would be the same as No. No side effects.

The popup would only display when there is potential for data loss, as is the case here. As such, it should ALWAYS be displayed when there is this potential unless the user explicitly requests ForceUpload.

So:

  • There is no "Don't show this again" setting or UX button to set this setting. The dialog won't show if the user explicitly set ForceUpload in the settings because the prompt isn't needed to get the "Yes" response implied by ForceUpload being on.

  • There is also no button in the popup UX to set the ForceUpload setting. There is already a way to do this via the normal settings. If a person repeatedly hits the Yes button on the proposed UX, they can set ForceUpload to avoid the popup if they intend to force all the time.

I think that this approach has less ambiguity and fewer side-effects.

Does this sound good to everyone?

If we settle on something, I can try working on the implementation. But I am a Typescript and vs code internals noob. I think that there will be a lot of localization and documentation work, so it could take a while.

@shanalikhan
Copy link
Owner

@karl-lunarg

Does this sound good to everyone?

That's sound perfect. No problem, take your time. It would be great if we can release the improved UX in the upcoming release. :)

@karl-lunarg karl-lunarg mentioned this issue Sep 13, 2019
24 tasks
@karl-lunarg
Copy link
Contributor

I've started working on this.

Note that I think that there is a problem with the latest commit for #993. The gist file name separator is supposed to be "|". If you have snippets, the data sent during an upload to patch the gist is malformed. I've included a commit that will be in this PR to correct this problem.

@shanalikhan shanalikhan added this to the v3.4.3 milestone Sep 18, 2019
@rcdailey
Copy link

I'm glad this was discussed because I've lost my settings countless times because of this bug. I work between two machines, that both had "force upload" on, so they were each clobbering settings in my GIST.

I think as part of the UX discussion we need an intrinsic method of reverting to an earlier GIST revision in case we wipe out our settings.

@shanalikhan
Copy link
Owner

v3.4.3 Released. Let me know if there is any problem :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants