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.

Sign up for GitHub

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

Jump to bottom

Fix duplicating annotations via toggling show archived annotations button and fix downsample modal rendering errors #6058

Merged
merged 7 commits into from Feb 28, 2022
Merged

Fix duplicating annotations via toggling show archived annotations button and fix downsample modal rendering errors #6058

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3e28707
fix duplicating annotations via toggling show archieved button
MichaelBuessemeyer Feb 17, 2022
282cde8
fix rendering error upon selecting down sample modal
MichaelBuessemeyer Feb 17, 2022
2f5efe7
add changelog entries
MichaelBuessemeyer Feb 17, 2022
240ab69
apply pr feedback && avoid another race condition in explorative anno…
MichaelBuessemeyer Feb 23, 2022
9cec3f5
Merge branch 'master' of github.com:scalableminds/webknossos into fix…
MichaelBuessemeyer Feb 23, 2022
29f9837
fix spelling
MichaelBuessemeyer Feb 23, 2022
02b7419
Merge branch 'master' of github.com:scalableminds/webknossos into fix…
MichaelBuessemeyer Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.unreleased.md
View file
Open in desktop
Expand Up @@ -34,6 +34,8 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
### Fixed
- Fixed volume-related bugs which could corrupt the volume data in certain scenarios. [#5955](https://github.com/scalableminds/webknossos/pull/5955)
- Fixed the placeholder resolution computation for anisotropic layers with missing base resolutions. [#5983](https://github.com/scalableminds/webknossos/pull/5983)
- Fixed a bug that lead to crashing the layer settings once the icon for the downsample modal was clicked. [#6058](https://github.com/scalableminds/webknossos/pull/6058)
MichaelBuessemeyer marked this conversation as resolved.
Show resolved Hide resolved
- Fixed a bug where toggling between not archived and archived annotations in the "My Annotation" of the dashboard lead to inconsistent states and duplicates of annotations. [#6058](https://github.com/scalableminds/webknossos/pull/6058)
MichaelBuessemeyer marked this conversation as resolved.
Show resolved Hide resolved
- Fixed a bug where ad-hoc meshes were computed for a mapping, although it was disabled. [#5982](https://github.com/scalableminds/webknossos/pull/5982)
- Fixed a bug where volume annotation downloads would sometimes contain truncated zips. [#6009](https://github.com/scalableminds/webknossos/pull/6009)
- Fixed a bug where downloaded multi-layer volume annotations would have the wrong data.zip filenames. [#6028](https://github.com/scalableminds/webknossos/pull/6028)
Expand Down
25 changes: 18 additions & 7 deletions frontend/javascripts/dashboard/explorative_annotations_view.js
View file
Open in desktop
Expand Up @@ -70,6 +70,7 @@ type State = {
searchQuery: string,
tags: Array<string>,
isLoading: boolean,
performedInitialFetch: boolean,
};

const persistence: Persistence<State> = new Persistence(
Expand All @@ -96,15 +97,21 @@ class ExplorativeAnnotationsView extends React.PureComponent<Props, State> {
searchQuery: "",
tags: [],
isLoading: false,
performedInitialFetch: false,
};

componentDidMount() {
this.setState(persistence.load(this.props.history));
this.fetchNextPage(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here was that the fetchNextPage relies on this.state.shouldShowArchivedTracings being up to date, but the setState Update of line 102 is not executed before the fetchNextPage is called. This lead to one of the errors. -> When wanting to initially show the archived annotations, the view instead showed the default (the active annotations).

Copy link
Member

Choose a reason for hiding this comment

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

Cool that you found the issue 👍 However, I think, that there is another race condition looming: fetchNextPage itself fetches annotations (depending on this.state.shouldShowArchivedTracings) and then calls setModeState which itself reads again from this.state.shouldShowArchivedTracings. If the user toggles the state after the annotations-fetch has started but before it finished, the annotations will be pushed into the wrong array. I think, it would make sense to adapt setModeState to accept the shouldShowArchivedTracings parameter directly (instead of reading it out again). Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you agree?

Jep 👍 , that sounds like a reasonable approach to fix this.

}

componentDidUpdate() {
componentDidUpdate(_prevProps, prevState) {
persistence.persist(this.props.history, this.state);
if (
this.state.shouldShowArchivedTracings !== prevState.shouldShowArchivedTracings ||
!this.state.performedInitialFetch
) {
this.fetchNextPage(0);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The performedInitialFetch is needed to enforce the initial fetch, as otherwise the initial fetch would not be executed when the active annotations should be shown initially. Reason: Because the default value for shouldShowArchivedTracings is false leading to the whole if to be true.
performedInitialFetch simply enforces the initial fetch. I did not take the option make shouldShowArchivedTracings initially null, because performedInitialFetch seemed easier to understand later on to me.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it's still a bit unusual, I guess, since componentDidUpdate is not called on the initial render (that's what componentDidMount is for). So, you are relying on the component updating, anyway, so that the fetch can be done. Probably this happens most of the time because of this.setState(persistence.load(this.props.history));, but it's not really safe. For example, if the localStorage is empty, the persistence module probably doesn't load anything so the state wouldn't change then?

I think, I would prefer to use this in componentDidMount:

componentDidMount() {
    this.setState(persistence.load(this.props.history), () => this.fetchNextPage(0));
}

The callback is triggered after setState is done. Then, you also don't need performedInitialFetch if I'm not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably this happens most of the time because of this.setState(persistence.load(this.props.history));, but it's not really safe.

Oh yeah, you are totally right here 👍. That's a nice suggestion. Thanks 🚀.

The callback is triggered after setState is done. Then, you also don't need performedInitialFetch if I'm not mistaken.

👍

}

getCurrentModeState = () =>
Expand Down Expand Up @@ -146,7 +153,9 @@ class ExplorativeAnnotationsView extends React.PureComponent<Props, State> {
// this refers not to the pagination of antd but to the pagination of querying data from SQL
const showArchivedTracings = this.state.shouldShowArchivedTracings;
const previousTracings = this.getCurrentModeState().tracings;

if (this.getCurrentModeState().loadedAllTracings) {
return;
}
try {
this.setState({ isLoading: true });
const tracings =
Expand All @@ -162,7 +171,7 @@ class ExplorativeAnnotationsView extends React.PureComponent<Props, State> {
} catch (error) {
handleGenericError(error);
} finally {
this.setState({ isLoading: false });
this.setState({ isLoading: false, performedInitialFetch: true });
}
};

Expand Down Expand Up @@ -210,8 +219,8 @@ class ExplorativeAnnotationsView extends React.PureComponent<Props, State> {
}

const hasVolumeTracing = getVolumeDescriptors(tracing).length > 0;
const { typ, id } = tracing;
if (!this.state.shouldShowArchivedTracings) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem was that this method did not depend on the state of the annotation but on the "show archived buttons" state. This way it is saver, that the correct actions are shown

const { typ, id, state } = tracing;
if (state === "Active") {
return (
<div>
<Link to={`/annotations/${typ}/${id}`}>
Expand All @@ -237,7 +246,7 @@ class ExplorativeAnnotationsView extends React.PureComponent<Props, State> {
<br />
</div>
);
} else {
} else if (state === "Finished") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philippotto Do you know of any other states an annotation can be in?

Copy link
Member

Choose a reason for hiding this comment

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

If I'm interpreting

webknossos/tools/postgres/schema.sql

Line 29 in 6c6a23b

CREATE TYPE webknossos.ANNOTATION_STATE AS ENUM ('Active', 'Finished', 'Cancelled', 'Initializing');
correctly, there can also be 'Cancelled' and 'Initializing'. However, I'm not sure if this is also possible for exploratives (and I've never seen "initializing"; maybe it's not exposed to the front-end). @fm3 Could you clarify this? Afterwards, @MichaelBuessemeyer can you change the string type in api_flow_types.js to "ACTIVE" | "Finished" | ... (depending on what @fm3 replied).

Copy link
Member

@fm3 fm3 Feb 18, 2022 •
edited

Choose a reason for hiding this comment

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

Explorative annotations exposed to the front-end can currently only be Active or Finished

return (
<div>
<AsyncLink
Expand All @@ -250,6 +259,8 @@ class ExplorativeAnnotationsView extends React.PureComponent<Props, State> {
<br />
</div>
);
} else {
return null;
}
};

Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.js
View file
Open in desktop
Expand Up @@ -714,7 +714,7 @@ class DatasetSettings extends React.PureComponent<DatasetSettingsProps, State> {

return (
<Tooltip title="Open Dialog to Downsample Volume Data">
<LinkButton onClick={this.showDownsampleVolumeModal}>
<LinkButton onClick={() => this.showDownsampleVolumeModal(volumeTracing)}>
<img
src="/assets/images/icon-downsampling.svg"
style={{
Expand Down
1 change: 1 addition & 0 deletions frontend/javascripts/oxalis/view/left-border-tabs/modals/downsample_volume_modal.js
View file
Open in desktop
Expand Up @@ -32,6 +32,7 @@ export default function DownsampleVolumeModal({
footer={null}
width={800}
maskClosable={false}
visible
>
<p>
This annotation does not have volume annotation data in all resolutions. Consequently,
Expand Down