Upload UX Improvements

[karl-lunarg]

Task checklist (roughly):

• Add status bar message for "already uploaded" case
• If local and gist is different and lastLocalDownload < lastGistUpload, display popup dialog to ask for one-time force update
  • Remove "Don't show again" UX and code
  • Change dialog box to "The gist settings changed since you last downloaded them. Do you want to upload your current local settings to the gist anyway?" Single yes/no button.
• If local and gist is different and lastLocalDownload >= lastGistUpload, upload with only a status bar message
• NLS for new dialogs and messages
• Update README doc (reverted to original)
• Update wiki

Short description of what this resolves:

• Improve the upload user experience
  • Detect if gist has changes that are not on the user's machine and prompt for overwrite
  • Remove popup option to turn on forceUpload and "Don't show again"

Changes proposed in this pull request:

• Streamline Upload UX
  • Remove "Don't show this dialog again" feature and setting
  • Change "Gist is newer than what you downloaded" dialog to a simple one-time force upload button

Fixes: #1035

How Has This Been Tested?

Since only the Upload logic was changed, it was necessary to test only upload features

1. Test the simple case of the user starting with synced settings and uploading without a change.

• Should see status bar message saying settings already uploaded

2. Test the simple case of the user starting with synced settings and uploading a change.

• No dialog popup and status bar indicates the successful upload

3. Modify the gist with another client and try to upload changes from the client under test. Expect to see a prompt asking if the user wants a forced upload.

• Verify that selecting Yes proceeds with the upload
• Verify that selecting No or closing the dialog does nothing and show canceled status msg

4. Verify that automatic upload on setting change still works.

• Start with synced settings and make a change - should complete normally
• Push a change from another host and make a change - should get prompt to overwrite

5. Upload after reset

• Reset, repeat test 1
• Reset, repeat test 2
• Reset, repeat test 3

6. Upload with forceUpload=true, All should do an upload

• Reset, repeat test 1
• Reset, repeat test 2
• Reset, repeat test 3 (expect no dialog)

7. New Gist: Delete gist, reset settings, and perform an upload

• New gist should be created and upload completes

Screenshots (if appropriate):

n/a

Checklist:

• I have read the contribution guidelines.
• My change requires a change to the documentation and GitHub Wiki.
• I have updated the documentation and Wiki accordingly.

[shanalikhan]

@karl-lunarg

When are you expecting to complete this PR.

[karl-lunarg]

When are you expecting to complete this PR.

I think I am finished. It would be good to get a few people to test.

[YunChaoTsai]

cmd.updateSettings.info.gotLatestVersion in package.nls.zh-tw.json is simplified chinese. It should be added in package.nls.zh-cn.json

[karl-lunarg]

cmd.updateSettings.info.gotLatestVersion in package.nls.zh-tw.json is simplified chinese. It should be added in package.nls.zh-cn.json

Sorry, but it looks like Google Translate gave me the same translation for simplified and traditional. Could you please supply the correct strings for both of these?

[shanalikhan]

@karl-lunarg
Before running manual tests.

I have pulled PR, run the following scenario:

1. Reset The extension settings
2. Reload Code
3. Upload and Get Token and Gist from GitHub
4. Hit upload

Upload isnt working, instead its showing the dialog "Sync: The settings in the gist have changed since ..."

[karl-lunarg]

Upload isnt working, instead its showing the dialog "Sync: The settings in the gist have changed since ..."

This was a bug in the call to the IsGistNewer github service function.

After a reset, customSettings.lastDownload is null. The call to IsGistNewer converts the customSettings.lastDownload null value to a Date, which results in Jan 1 1970. Later, inside of IsGistNewer, the code checks for a null localLastDownload by using !localLastDownload which evaluates to false in this case because Jan 1 1970 is a valid date. Then the gistLastUpload > localLastDownload check always is true when the localLastDownload is Jan 1 1970.

This caused the upload code to think that the gist is newer and so it showed the dialog.

customSettings.lastDownload is already a Date, so there is no need to convert it to a Date when calling IsGistNewer. Further, converting it is harmful because it breaks the null check in IsGistNewer.

Fix is to not do the conversion. I'll push a fix for this.

[shanalikhan]

@karl-lunarg

Exactly! Thank you for a detailed explanation. Fantastic Work

How Has This Been Tested?

I hope you have tested all the possible scenarios you have mentioned. (Ref : #1035 (comment) , #1035 (comment) and above scenarios)

I have tested it and it looks fine but as Settings Sync dont have proper test cases defined since start and i always miss that. I just want to know are you available to fix if somethings comes up after release regarding this feature ? So i can release hitfix for it if somethings happens.

[karl-lunarg]

In these recent commits, I refactored the two commits that contained the related changes for moving the download date conversion to inside ofisGistNewer() into one commit since it makes better sense and is more bisectable.

I also added a commit to remove trailing whitespace so that tslint runs clean.

[karl-lunarg]

I hope you have tested all the possible scenarios you have mentioned

I did pretty thorough testing after the main changes, but I want to do them all again and more after these more recent minor changes. I'll do that in the next day or so. This project is difficult to test because of all the possible cases and the difficulty in setting up some of them.

I just want to know are you available to fix if somethings comes up after release regarding this feature ?

I use vscode and this extension a lot (thanks for it, btw), so I have some interest in keeping it in good shape, so I'll try to pay attention to the repo. I have improved the code a bit by making it simpler, fixing a few bugs, and adding some comments to help make the logic clearer. So I'm hoping that you and/or the other contributors to this project who know a lot more than me about all this can take the first crack at any problems. I'll help out as needed.

---

I use vscode and this extension on several dev machines. On one of them, I can't seem to test the extension using the Extension Development Host, the second instance of vscode that pops up when you debug the extension from the primary instance. The extension under test fails to load in the EDH and I get messages in the Debug Console saying "ReferenceError: navigator is not defined". I've searched all over and can't find a solution. One of my other dev machines is set up with the same OS, etc and does not have this problem. It feels like I'm missing an npm component or something. I do have everything working on the other machine, so I don't think it is a problem with the extension. But it would be nice to solve the problem on the broken machine so I can work on this more easily.

Any ideas?

[shanalikhan]

I did pretty thorough testing after the main changes, but I want to do them all again and more after these more recent minor changes. I'll do that in the next day or so.

Perfect. Just let me know once your testing is completed and yes testing this extension is hard specially when you are already have dev. env. and using this extension for your env. for this i would suggest you to use vscode portable and setup that code for this extension only so you dont have to switch to extension debugging env. and your personal env. for work.
Yes there are actually alot of use cases that needs to be tested. I often sometimes miss but eventually complete the testing before releasing.

The extension under test fails to load in the EDH and I get messages in the Debug Console

Have you tried running vscode in Admin Mode, just make sure you are installing the npm components as an admin mode. If you want to install and validate the components you can try running these commands.

npm i --no-optional
npm dedupe
npm up

If still you can't test on other computers, you can create a package using vsce package command and install the extension from the package in those computers.

[karl-lunarg]

The extension under test fails to load in the EDH and I get messages in the Debug Console

I got my eyeballs on the two machines at the same time and found that I had just installed npm/nodejs differently and incorrectly on one of them. I've reinstalled and I think I have it sorted.

I should have mentioned that this was on Linux. I found this guide helpful.

[karl-lunarg]

Testing:

I've finished testing and updated the test plan I used up in the top comment. Things look pretty good.

But there is one corner-case problem:

• Start with local machine with settings synced to the gist
• A different machine uploads a settings change to the gist
• On local machine, reset settings
• On local machine, upload settings

The settings will upload to the gist, overwriting the change made by the other machine, without prompting.

This is caused by the lack of a localLastDownload date after the reset. Without this, I can't tell if the gist has changed since the last download.

One way out of this is to throw a new dialog message if the files are different and there is no localLastDownload date. Something like "Settings Sync is unable to reconcile the differences between your local settings and your gist. Consider downloading the settings or doing a forced upload now. Do you want to upload your current local settings to the gist now?" And there would be the same Yes/No buttons as the other dialog.

Another option would be to try to merge the above idea in the existing dialog, saying something like "The settings in the Gist have changed since you last downloaded them or you have reset your Sync Settings. Do you want to upload your current local settings to the gist now?".

I sort of prefer the new dialog, since giving the user two reasons for the popup error is not very useful.

@shanalikhan Should we fix this for this release? The exposure to these conditions might be pretty small, but there is unannounced data loss. (Although, it can be recovered from the git gist).

[shanalikhan]

one corner-case problem

Yes you are right, @karl-lunarg Settings Sync doesnt check for that case. Thank you for the detailed testing of each scenario.

I sort of prefer the new dialog, since giving the user two reasons for the popup error is not very useful.

If you can fix that dialog, i would recommend to add that new dialog too in this PR so UX will be perfect handling all of the situations.

Instead of

"Settings Sync is unable to reconcile the differences between your local settings and your gist. Consider downloading the settings or doing a forced upload now. Do you want to upload your current local settings to the gist now?"

I would suggest:

"Upload will replace the older version of Settings in GitHub Gist. Consider downloading the settings or doing a force upload. Do you still want to upload forcefully ?"

Phrase can be improve furthermore.

[karl-lunarg]

@shanalikhan : I agree that it is good to fix this now.

I wasn't completely sure if you wanted the new dialog or not. We have a lot more information when there is a valid last download date, so I felt a new dialog was better since the existing gistNewer message can be more definite about what changed. So I added a new dialog with a message pretty close to your suggestion.

This adds more NLS entries. @YunChaoTsai, I likely got the Chinese translations wrong again, so if you could please look at those in the last commit and use the GitHub suggestion mechanism to fix them, that would be great, just like you did last time.

I did the testing for this latest change and things look good.

I think I am done. This seems to be a reasonable improvement over the current version.

[borekb]

Thanks @karl-lunarg for all the effort you put into this. Really appreciated!

[shanalikhan]

@karl-lunarg

Thank you for you detailed effort and testing all of the possible scenarios. I will release the next version in few days.