Add UI to restore previous versions of a tracing #3194

daniel-wer · 2018-09-11T09:11:25Z

This PR adds UI to restore older versions of a skeleton tracing. Restoring an older version simply creates a "new version" in the version history of the tracing - so it is possible to revert the reversion :)
The version preview and restore reuses functionality that we implemented for the task hot swap.

ToDo:

@philippotto In the version restore view, when scrolling, I would like to keep the headline on top and only scroll the version entries. However, I'm not css/html-savy enough to accomplish this. Any ideas?
@philippotto I'm having trouble with the flow types for the updateActions that are sent by the server. The server sends the same updateActions that we are sending, BUT there's an additional actionTimestamp (and maybe version). I tried to use flows $Call syntax to adapt our types, but the resulting flow type looks wrong to me (when looking at it through the type-at-pos functionality). I also can't get rid of the remaining flow errors.
@fm3 The versions are acquired through the datastore updateActionLog route, which returns batches of update actions. However, those batches are missing a version number. Currently the frontend is computing those versions, assuming the last entry is version 1, and counting up. Ideally, the backend would send the version number for each batch.
We need to decide whether we want to group subsequent createNode actions by time interval or so, to avoid having too many of those createNode actions spamming the version restore view.

URL of deployed dev instance (used for testing):

https://restoreoldversions.webknossos.xyz

Steps to test:

Test the version restore mode
- Create a skeleton tracing and trace a bit, split/merge a tree, delete a tree, whatever.
- Then open the version restore view by clicking Restore older version in the dropdown next to the save button. The tracing should be saved and in read only mode now.
- Preview any version by selecting it. You should not be able to modify the tracing.
- Now there are two options:
  - Either close the version restore view. The newest version should be loaded again and you should be able to modify it and save.
  - Or select Restore this version for an older version. The old version should be loaded and you should be able to modify it and save. Opening the version restore view again should show that you reverted to a previous version.
Create a volume tracing - the option to open the version restore view should not be visible.

Issues:

fixes [OX-475] Versioned tracings / Reset button #51 😮

Updated changelog
~~Updated migration guide if applicable~~
Updated documentation if applicable
Needs datastore update after deployment
Ready for review

… description and icon

…tore-old-versions

daniel-wer · 2018-09-11T12:28:00Z

app/assets/javascripts/oxalis/model/sagas/update_actions.js

+};
+type AsServerAction<A> = $Call<AddServerValuesFn, A>;
+
+// Since flow does not provide ways to perform type transformations on the


@philippotto Thanks!!! 🙏

philippotto · 2018-09-11T11:02:52Z

app/assets/javascripts/oxalis/model/actions/ui_actions.js

@@ -6,11 +6,21 @@ type SetDropzoneModalVisibilityActionType = {
  visible: boolean,
 };

-export type UiActionType = SetDropzoneModalVisibilityActionType;
+type SetVersionRestoreModeActionType = {
+  type: "SET_VERSION_RESTORE_MODE_ACTION_TYPE",


Oh, I see that I used the "_TYPE" suffix in the action constant for SET_DROPZONE_MODAL_VISIBILITY_ACTION_TYPE. However, we usually don't use this suffix in action strings. Can you remove that suffix for both actions? :)

philippotto · 2018-09-11T11:03:53Z

app/assets/javascripts/oxalis/model/actions/ui_actions.js

@@ -6,11 +6,21 @@ type SetDropzoneModalVisibilityActionType = {
  visible: boolean,
 };

-export type UiActionType = SetDropzoneModalVisibilityActionType;
+type SetVersionRestoreModeActionType = {


Why Mode? Isn't it only visible/invisible? I'd use visibility then, since mode sounds like there would be different "restore modes".

philippotto · 2018-09-11T11:07:48Z

app/assets/javascripts/oxalis/model/sagas/update_actions.js

@@ -300,3 +307,11 @@ export function updateTreeGroups(treeGroups: Array<TreeGroupType>): UpdateTreeGr
    },
  };
 }
+export function revertToVersion(version: number) {


Is it okay to omit the return type here?

I copy-pasted from the only other update action that omitted the return type ^^ Added one to both of them :)

philippotto · 2018-09-11T11:21:07Z

app/assets/javascripts/oxalis/view/version_view.js

+
+  render() {
+    const VersionEntry = ({ batch, version, isNewest }) => {
+      const lastTimestamp = Math.max(...batch.map(entry => entry.value.actionTimestamp));


I'm not sure how likely it is to get a stack overflow with this (see https://stackoverflow.com/questions/42623071/maximum-call-stack-size-exceeded-with-math-min-and-math-max), but for the sake of safety I'd replace this with _.max to avoid the arg spread.

philippotto · 2018-09-11T12:33:25Z

app/assets/javascripts/oxalis/view/version_view.js

+  T,
+) => {
+  name: $PropertyType<T, "name">,
+  value: { ...$PropertyType<T, "value">, actionTimestamp: number, version: number },


In general, this already looks very nice. Unfortunately, flow cannot understand that $PropertyType<T, "name"> and $PropertyType<T, "value"> should always refer to the same Union-part. That's why it thinks that name can be "updateTracing or deleteNode or ..." while simultaneously thinking "value can be X or Y or Z". It doesn't derive the constraint "if name is updateTracing, value is X".

I didn't find a nice way to express what we want, which is why I ended up defining a ServerUpdateAction which is a union of AsServerType<UpdateTracing-style parts. Additionally, it was necessary to add extra type-checks, since the typings of _.groupBy are not expressive enough (not sure whether they can be improved or if flow is too limited here, too).

philippotto

The code looks really nice! However, I broke it immediately by opening a new tracing, creating five nodes (hitting save after each creation) and opening the view :(

philippotto · 2018-09-11T12:41:30Z

However, I broke it immediately by opening a new tracing, creating five nodes (hitting save after each creation) and opening the view :(

My bad, that's due to the changes I committed. Fixed it now :)

daniel-wer · 2018-09-11T12:41:51Z

~~What 😂 I reject all blame 😁 In all seriousness, I think you broke it with your flow fix commit.~~ nvm :)

daniel-wer · 2018-09-11T12:42:19Z

Thanks for fixing! :)

philippotto

Ok, I also implemented the scrolling now :) Additionally, I tweaked the layout/styling a bit (I removed the tooltip and put its content under the headline). Not too sure overall, though. So feel free to change it back.

In general, this feature looks and feels really, really cool :) I'm also very happy that it works so smoothly with the hot-swap functionality!

I have a few remarks/questions:

If the version-view is open, the tracing becomes read-only. This is not super obvious in my opinion. I added a hint to the text below the version-history-headline, but I'm not sure whether this enough. Do you think we can/should we make it more prominent?
Does the "copy to my account" button work when being in read-only mode?
When reverting to a specific version, the version entry says "Reverted to version X". However, the version numbers are mentioned nowhere else in the view. Ideally, each entry should have the version number shown somewhere. But maybe you wanted to add this after the back-end provides this data?
If I create a node, the corresponding version says "createNode and two other entries". What are the two other entries? update tracing due to moving? Also, we might want to map "createNode" to a non-camel-case variant.

…tore-old-versions

daniel-wer · 2018-09-12T13:28:47Z

@philippotto Thanks a lot for your help with the flow types and flex boxes :)

If the version-view is open, the tracing becomes read-only. This is not super obvious in my opinion. I added a hint to the text below the version-history-headline, but I'm not sure whether this enough. Do you think we can/should we make it more prominent?

Does the "copy to my account" button work when being in read-only mode?

It did not work as expected, same for the Download and Merge functionality, which is why I hide all of the tracing actions now. I added an Alert in its place and made the description in the version restore view an Alert as well. Not too intrusive but noticeable in my opinion :)

When reverting to a specific version, the version entry says "Reverted to version X". However, the version numbers are mentioned nowhere else in the view. Ideally, each entry should have the version number shown somewhere. But maybe you wanted to add this after the back-end provides this data?

I added the version number :)

If I create a node, the corresponding version says "createNode and two other entries". What are the two other entries? update tracing due to moving? Also, we might want to map "createNode" to a non-camel-case variant.

I added descriptions for the other cases as well (createNode, updateTreeGroups, ...). The only thing that's "problematic" is updateTracing, as that could mean the user switched positions, or updated a comment, or updated a branchpoint, or ... I'd say a generic description is fine for now and we can always improve later.

I also added a scroll-into-view for the version restore view as I noticed that it was hidden to the right side if the screen was not big enough (because the tracing views were scaled too big).

…tore-old-versions

philippotto · 2018-09-12T15:27:33Z

Awesome! These changes really top it off :)

We need to decide whether we want to group subsequent createNode actions by time interval or so, to avoid having too many of those createNode actions spamming the version restore view.

Do you want to do this in this PR? I'd suggest to leave it the current state and iterate in another PR. Ideally, I would also like some grouping by date, so that there are collapsibles for each day. Then we could also remove the date formatting from each version so that there are only the times of the day. So, like that:

Monday, 19-07-18
   Version X (15:23)
   Version X (8:53)
Friday, 16-07-18
   Version X (18:23)
   Version X (12:53)

…tore-old-versions

…rsionRestore url parameter that opens the version restore view without fetching the newest tracing

daniel-wer added 5 commits September 6, 2018 15:21

first working version of tracing version restore

5091391

added types and categorized some update action batches to show better…

95f38ec

… description and icon

Merge branch 'master' of github.com:scalableminds/webknossos into res…

2bc2dcd

…tore-old-versions

Merge branch 'master' of github.com:scalableminds/webknossos into res…

f0277ed

…tore-old-versions

improve batch description, save before opening version restore view

bc317a4

daniel-wer added backend frontend labels Sep 11, 2018

daniel-wer self-assigned this Sep 11, 2018

daniel-wer requested a review from philippotto September 11, 2018 09:11

rework flow types for ServerUpdateActions

f8a54f1

daniel-wer commented Sep 11, 2018

View reviewed changes

philippotto reviewed Sep 11, 2018

View reviewed changes

fix missing null checks

a5fbd4f

only scroll version list in version history view

4505729

philippotto approved these changes Sep 11, 2018

View reviewed changes

philippotto and others added 4 commits September 11, 2018 17:17

remove obsolete import

c440da3

Merge branch 'master' of github.com:scalableminds/webknossos into res…

2bb3f0c

…tore-old-versions

PR feedback #1, show info alert instead of tracing actions

cbd75dc

in updateActionLog, annotate batches with version number (#3203)

b5179a9

daniel-wer added the datastore-changes label Sep 12, 2018

PR feedback #2, scroll into view, descriptions for other update actions

5536431

daniel-wer added 3 commits September 12, 2018 15:29

Merge branch 'master' of github.com:scalableminds/webknossos into res…

e64b3b7

…tore-old-versions

adapt changelog and docs

f39f289

[docs] point to 'restore older version' from redo/undo, fix typo

58e57b1

daniel-wer added 5 commits September 17, 2018 13:56

Merge branch 'master' of github.com:scalableminds/webknossos into res…

5a064e8

…tore-old-versions

make version restore view's position fixed, style fixes, allow showVe…

3e9c150

…rsionRestore url parameter that opens the version restore view without fetching the newest tracing

Merge branch 'master' into restore-old-versions

4f67ffa

Merge branch 'master' into restore-old-versions

d2aad3f

Merge branch 'master' into restore-old-versions

c7187c1

daniel-wer merged commit 9628dde into master Sep 17, 2018

daniel-wer deleted the restore-old-versions branch September 17, 2018 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI to restore previous versions of a tracing #3194

Add UI to restore previous versions of a tracing #3194

daniel-wer commented Sep 11, 2018 •

edited

Loading

daniel-wer Sep 11, 2018

philippotto Sep 11, 2018

philippotto Sep 11, 2018

philippotto Sep 11, 2018

daniel-wer Sep 12, 2018

philippotto Sep 11, 2018

philippotto Sep 11, 2018

philippotto left a comment

philippotto commented Sep 11, 2018

daniel-wer commented Sep 11, 2018 •

edited

Loading

daniel-wer commented Sep 11, 2018

philippotto left a comment

daniel-wer commented Sep 12, 2018

philippotto commented Sep 12, 2018

Add UI to restore previous versions of a tracing #3194

Add UI to restore previous versions of a tracing #3194

Conversation

daniel-wer commented Sep 11, 2018 • edited Loading

ToDo:

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

daniel-wer Sep 11, 2018

Choose a reason for hiding this comment

philippotto Sep 11, 2018

Choose a reason for hiding this comment

philippotto Sep 11, 2018

Choose a reason for hiding this comment

philippotto Sep 11, 2018

Choose a reason for hiding this comment

daniel-wer Sep 12, 2018

Choose a reason for hiding this comment

philippotto Sep 11, 2018

Choose a reason for hiding this comment

philippotto Sep 11, 2018

Choose a reason for hiding this comment

philippotto left a comment

Choose a reason for hiding this comment

philippotto commented Sep 11, 2018

daniel-wer commented Sep 11, 2018 • edited Loading

daniel-wer commented Sep 11, 2018

philippotto left a comment

Choose a reason for hiding this comment

daniel-wer commented Sep 12, 2018

philippotto commented Sep 12, 2018

daniel-wer commented Sep 11, 2018 •

edited

Loading

daniel-wer commented Sep 11, 2018 •

edited

Loading