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 cloud save #7836

Merged
merged 22 commits into from
Feb 3, 2021
Merged

Improve UX for cloud save #7836

merged 22 commits into from
Feb 3, 2021

Conversation

darzu
Copy link
Contributor

@darzu darzu commented Jan 26, 2021

  • in the editor, shows only temporary "saving..." and "saved!" messages while/after a cloud save happened.
  • Remove the double page reload after signing in
  • Projects list in the Home Screen and "All Projects" view sync correctly and without a reload
  • Fix several bugs around the virtual APIs
  • Fix bugs around project delete not working right with the cloud

Fixes: microsoft/pxt-arcade#2988
Fixes: microsoft/pxt-arcade#3017

In editor "saving..." and "saved!" message:
2021-02-01 08 57 22

In project view dynamic syncing and deleting without a reload!
2021-02-01 09 00 37

@darzu
Copy link
Contributor Author

darzu commented Jan 26, 2021

Pausing this as we re-evaluate priorities.

@darzu
Copy link
Contributor Author

darzu commented Jan 26, 2021

Fixes: microsoft/pxt-arcade#2988

@darzu darzu requested a review from eanders-ms February 1, 2021 17:07
@darzu darzu changed the title [DRAFT] Improve UX for cloud save Improve UX for cloud save Feb 1, 2021
@darzu darzu marked this pull request as ready for review February 1, 2021 17:31
Copy link
Collaborator

@eanders-ms eanders-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this work is still in progress, but this looks like a good merge point if it is stable enough.

@@ -1332,7 +1332,8 @@ export class ProjectView
// We are too late; the editor has already been loaded.
// Call the onChanges handler to update the editor.
pxt.tickEvent(`identity.syncOnProjectOpen.timedout`, { 'elapsedSec': elapsed})
cloud.onChangesSynced(changes)
if (changes.length)
cloud.forceReloadForCloudSync()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this call trigger a reload while in the editor? I've seen the toast "Cloud synchronization complete. Reloading" happen in the editor, which was jarring. Logged it here: microsoft/pxt-arcade#3005

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it can. Unfortunately I don't know a good way to avoid this.

The problem is that if the user opens up a project at version A but after that we sync with the cloud and determine they should be fast-forwarded to version B, then we need to reload the editor from underneath them. Currently, there isn't a way to reload the editor's content without doing a page reload. Likely we could fix this using virtual APIs but it's another chunk of work..

if (remote) {
local.cloudLastSyncTime = remote.cloudLastSyncTime
// Note that we use modification time to detect differences. If we had full (or partial) history, we could
// use version numbers. However we cannot currently use etags since the Cosmos list operations
// don't return etags per-version. And because of how etags work, the record itself can never
// have the latest etag version.
if (local.modificationTime !== remote.modificationTime) {
if (local.modificationTime !== remote.modificationTime || local.isDeleted !== remote.isDeleted) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to switch to cosmos etag here. We're will see sync bugs with modificationTime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm pro switching to etags. It'll take a cloud deploy.

@darzu
Copy link
Contributor Author

darzu commented Feb 2, 2021

I know this work is still in progress, but this looks like a good merge point if it is stable enough.

Yes, this is meant to be a complete PR. Future work will be in a future PR.

@darzu darzu merged commit ead7ef4 into master Feb 3, 2021
@darzu darzu deleted the dazuniga/cloudsync_ux branch February 3, 2021 00:25
darzu added a commit that referenced this pull request Feb 8, 2021
* Showing more cloud state on the project cards

* showing last cloud edit time and state in UX

* Better date appearance; better alignment; todo lst

* dynamic homescreen update works

* Dim the cloud state message in the editor

* Notify virtual api of header change at the start of cloud save (so we can show "saving...")

* Keep cloudInProgressSyncStartTime in memory only

* cloud sync update without reload is working!

* update home page project list when num headers changes

* Fix how delete propegates

* Update virtual APIs & DataSubscriber to propegate specific path that changed from invalidate() call

* Revert virtual API changes. one-to-many subscriptions need more work before they'll funciton.

* Remove cloud sync message on home screen

* Don't show cloud state unless saving

* Keep a "saved!" indicator around right after we cloud save.

* Move CloudStateSummary out of pxt localtypings

* fix render bug

* remove debug logging

* Don't download deleted projects
darzu added a commit that referenced this pull request Feb 8, 2021
* Showing more cloud state on the project cards

* showing last cloud edit time and state in UX

* Better date appearance; better alignment; todo lst

* dynamic homescreen update works

* Dim the cloud state message in the editor

* Notify virtual api of header change at the start of cloud save (so we can show "saving...")

* Keep cloudInProgressSyncStartTime in memory only

* cloud sync update without reload is working!

* update home page project list when num headers changes

* Fix how delete propegates

* Update virtual APIs & DataSubscriber to propegate specific path that changed from invalidate() call

* Revert virtual API changes. one-to-many subscriptions need more work before they'll funciton.

* Remove cloud sync message on home screen

* Don't show cloud state unless saving

* Keep a "saved!" indicator around right after we cloud save.

* Move CloudStateSummary out of pxt localtypings

* fix render bug

* remove debug logging

* Don't download deleted projects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants