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

Clean up merge modal and allow "live-merge" of explorative and project #2935

Merged
merged 30 commits into from Jul 30, 2018

Conversation

@philippotto
Copy link
Member

commented Jul 20, 2018

URL of deployed dev instance (used for testing):

Steps to test:

  • merge an explorative directly in the tracing view (new functionality) via merge-modal
  • merge an explorative directly old-school (creates a new explorative)
  • do the same for a project

To discuss:

  • for big projects (or exploratives), the merge will probably work, but saving to the server might crash. we probably need to think about how to tackle this (e.g., chunk the update actions so the server is not overwhelmed; show better progress report so that saving for a few minutes is an ok UX; show a warning that this may take some time etc...)

Issues:


@philippotto philippotto added this to the Sprint 25a milestone Jul 20, 2018

@philippotto philippotto self-assigned this Jul 20, 2018

@philippotto philippotto requested a review from daniel-wer Jul 20, 2018

@daniel-wer
Copy link
Contributor

left a comment

Nice - two things I noticed:

  • I would expect the merge modal to close after a successful merge.
  • It's also possible to merge projects/explorative annotations from other datasets. I'm not sure whether this was allowed before (probably?), but I don't think this is wanted behavior and we specifically disallow this when uploading NMLs.
this.setState({ isUploading: false });
Toast.success(messages["tracing.merged"]);
} else {
Toast.error("Merging is not supported for volume tracings.");

This comment has been minimized.

Copy link
@daniel-wer

daniel-wer Jul 23, 2018

Contributor

Should this be moved to the messages file as well? Actually, I'm not sure what our current practice regarding the messages.js is anyways? Maybe we should discuss this, not as part of this PR :)

@daniel-wer

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

Also, does the backend still do some kind of tree renaming when merging a project @fm3? If so, does this happen in this "frontend-merge" case as well (by retrieving the compound project from the server)? I just want to make sure that we don't lose any functionality :)

@fm3

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

no, there is no tree renaming, only the node id mapping and the combining of the tracings’ bounding boxes

@daniel-wer

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

Sorry for spamming, but I just noticed another thing. When un-checking the checkbox (to create a new tracing when merging), didn't we redirect to that new tracing before? I didn't use the merge functionality for quite a while now, so maybe that wasn't the case, but it feels weird that the user just gets the "Merging successful" message and then nothing happens (apart from the new tracing appearing in the explorative tracings list, which the user cannot see at that point).

@philippotto

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2018

Thank you for your feedback, @daniel-wer.

I would expect the merge modal to close after a successful merge.

I guess this has never been like that for the case that the user wants to do several merges consecutively. I wouldn't change it now unless some requests it.

It's also possible to merge projects/explorative annotations from other datasets. I'm not sure whether this was allowed before (probably?), but I don't think this is wanted behavior and we specifically disallow this when uploading NMLs.

Good point, I added a guarding check for that.

When un-checking the checkbox (to create a new tracing when merging), didn't we redirect to that new tracing before?

Good point! In fact, the code redirects to the new tracing via the router's history.push function, but this doesn't work (I think, since the oxalis controller does not listen to prop changes..). I changed the redirect to use location.href so that we get a fresh page.

@philippotto

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2018

@daniel-wer @fm3 What do you think about the bounding boxes? When doing the merge via the back-end, the bboxes are combined. When done locally, it's only importing the trees. I'm not sure whether this is an issue at all. The trees will be visible, anyway..

Also, do we want to add a "warning" that the local change can take some time for saving. Or do we want to land this feature together with the chunk saving PR which adds progress indicators.

@daniel-wer

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

I'd say it's fine to ignore the bounding boxes when merging in the frontend for now. As you say, the trees are visible anyways. We can create an issue and add it later.
Let's land it together with the progress indicator as that PR is also almost ready.

philippotto added 6 commits Jul 26, 2018
Merge pull request #2947 from scalableminds/chunk-save-actions
Send save actions in chunks to server

@philippotto philippotto merged commit 3a29f15 into master Jul 30, 2018

2 checks passed

ci/circleci: build_test_deploy Your tests passed on CircleCI!
Details
coverage/coveralls Coverage increased (+0.08%) to 49.97%
Details

@philippotto philippotto deleted the clean-up-merge-modal branch Aug 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.