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

Iterate on version history view #3365

Merged
merged 22 commits into from
Oct 23, 2018
Merged

Iterate on version history view #3365

merged 22 commits into from
Oct 23, 2018

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Oct 16, 2018

This PR refines the version view as described in #3309. Versions are grouped by day and collapsed into 5-minute buckets. If a 5-min bucket would only contain a single version, it will be displayed directly.
A version 0 (createTracing) is added to the version view, so it is possible to revert to the very first state of the tracing.
This PR also minimizes (to some extent) the number of update actions that are being sent to the server. We're no longer unconditionally sending updateTracing update actions, if nothing changed. Additionally, subsequent update action batches that only contain an updateTree update action for the same treeId (happens for every keystroke when editing a comment, for example) will be "compressed", so that only the last updateTree update action is sent to the server.

Open issues:

Ideally, I would like to jump to the respective volume tracing's editPosition when previewing previous volume tracing versions in a hybrid tracing, so the user can see the changed buckets, but I can only think of hacky ways to do that (by default only the skeleton tracing editPosition is used, and the volume one is discarded). Maybe you have a good idea for that?

When previewing volume versions (very) fast, bucket request errors are printed to the console (caused by the pending requests), however, I could not see any display artifacts. I also made sure that previewing volume versions doesn't cause a memory leak.

Screenshots:

refined-versions

create-tracing

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open a hybrid tracing with some versions, or create one and skeleton/volume trace a bit. Then return 10 minutes later and trace some more. Then, opening the Version View should show the created versions in the previously described manner.
  • Open an older skeleton tracing and open the version view. You should see the versions grouped by day.
  • Edit a comment and then open the version view, only one version should have been created.

Issues:


@daniel-wer daniel-wer self-assigned this Oct 16, 2018
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome, looks really good 🎉 I made one comment about potentially refactoring the chunking logic a bit, but other than that I'm sold :)

@@ -83,28 +102,71 @@ class VersionList extends React.Component<Props, State> {
Store.dispatch(setAnnotationAllowUpdateAction(true));
};

previewVersion = (version: number) => previewVersion({ [this.props.tracingType]: version });
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, is there a better way to name the method? Having the same names for a method and a function is a bit confusing. Maybe we don't need the method and can inline its content?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to inline it to avoid unnecessary re-renders and renamed it to handlePreviewVersion.

return _.mapValues(groupedVersions, versionsOfOneDay => {
let chunkIndex = 0;
let chunkTime = 0;
return _.reduce(
Copy link
Member

Choose a reason for hiding this comment

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

Do you think this can be refactored into a helper function, which can be used like:

chunkIntoTimeWindows(versionsOfOneDay, batch => _.max(batch....), CHUNK_BY_X_MINUTES)

First parameter would be the data array, second parameter the function which maps an entry to a time and the third the window size? Then, we could also write a unit test for the chunking code :)

Copy link
Member Author

Choose a reason for hiding this comment

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

What a marvelous idea 😀 I did both of that :)

@@ -360,7 +360,7 @@ declare module "lodash" {
defer(func: Function, ...args?: Array<any>): number;
delay(func: Function, wait: number, ...args?: Array<any>): number;
flip(func: Function): Function;
memoize(func: Function, resolver?: Function): Function;
memoize<T>(func: T, resolver?: Function): T;
Copy link
Member

Choose a reason for hiding this comment

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

😍

@daniel-wer daniel-wer merged commit 2b651cd into master Oct 23, 2018
@daniel-wer daniel-wer deleted the refine-version-view branch October 23, 2018 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Iterate on version history
3 participants