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

Avoid proofreading actions while the last one is not finished #6325

Merged
merged 14 commits into from
Jul 26, 2022

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Jul 7, 2022

Set wk busy during proofreading and also modify the input catchers so that they not only show a loading spinner if wk is busy, but actually block all inputs.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open an annotation with an agglomerate view, activate it and switch to proofreading mode
  • Click a couple of agglomerates to load their agglomerate skeletons
  • Open the dev tools, switch to the "Network" tab and select "Slow 3G" throttling so that the proofreading action takes longer
  • Do a proofreading action by adding or deleting an edge and try to perform another proofreading action in rapid succession
    • The second action should no longer be possible since all inputs to the viewports are blocked and a loading spinner is shown

Issues:


(Please delete unneeded items, merge only when none are left open)

@daniel-wer daniel-wer self-assigned this Jul 7, 2022
@@ -234,7 +234,7 @@ class SynapseTree extends React.Component<Props, State> {
<Dropdown // Lazily create the dropdown menu and destroy it again, afterwards
overlay={() => this.createSegmentDropdownMenu(data.id)}
autoDestroy
placement="bottomCenter"
placement="bottom"
Copy link
Member Author

Choose a reason for hiding this comment

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

Saw this deprecation warning: Warning: [antd: Dropdown] You are using 'bottomCenter' placement in Dropdown, which is deprecated. Try to use 'bottom' instead.

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Looks good to me 🎉.

I only got one point, where I was unsure whether the deletion of a line was intentional. I'll approve anyway so you can directly merge when this was intentional. Otherwise might need to wait a bit again until I can approve :)

I only managed to sometimes load a proofreading annotation where the hdf5 mapping was already loaded directly after opening the annotation. But the proofreading tool was not visible in the toolbar. Only after I switched the mapping, the proofreading tool was visible. I think this is unrelated but still a bug, right?

Comment on lines -287 to -288
// Undo the last splitting/merging action since the proofreading action did not succeed
yield* put(undoAction());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we no longer undo the action when the call failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was an oversight while merging the original proofreading PR. Undo/Redo is actually not allowed if the proofreading tool is active and a warning in that regard is shown to the user if it is triggered. This was added kind of last-minute, because the actual proofreading actions cannot be undone yet. Unfortunately, that's why this line had to be removed for now.

@daniel-wer
Copy link
Member Author

Thanks a lot for the review!

I only managed to sometimes load a proofreading annotation where the hdf5 mapping was already loaded directly after opening the annotation. But the proofreading tool was not visible in the toolbar. Only after I switched the mapping, the proofreading tool was visible. I think this is unrelated but still a bug, right?

This sounds like a bug, I'll have a look :)

@daniel-wer
Copy link
Member Author

daniel-wer commented Jul 20, 2022

@MichaelBuessemeyer Thanks again for indicating the buggy behavior. I've pushed a fix :)

To reproduce, make sure the an agglomerate view mapping is activated and the mapping selection tab is not visible when reloading the page - that's what reliably triggered the bug before.

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Thanks again for indicating the buggy behavior. I've pushed a fix :)

I just tested the fix locally and it worked 🎉. Thanks for fixing this bug 🐛🧋.

The pr should be ready to merge. (I do not think a changelog entry for the fix is necessary.)

@bulldozer-boy bulldozer-boy bot merged commit 0139cbd into master Jul 26, 2022
@bulldozer-boy bulldozer-boy bot deleted the busy-proofreading branch July 26, 2022 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating multiple edges quickly in proofreading mode crashes
2 participants