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 volume mode in hybrid tracings #4467

Merged
merged 15 commits into from
Mar 16, 2020

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Mar 8, 2020

This PR defaults to orthogonal mode when the active tracing is hybrid and the URL says to go into volume mode.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Create a volume tracing and convert it into a hybrid tracing (Dataset info tab).
  • The opened tracing should be a hybrid tracing in orthogonal mode.

Issues:


@MichaelBuessemeyer MichaelBuessemeyer self-assigned this Mar 8, 2020
@MichaelBuessemeyer MichaelBuessemeyer changed the title [WIP] Avoid volume mode in hybrid tracings Avoid volume mode in hybrid tracings Mar 8, 2020
@MichaelBuessemeyer
Copy link
Contributor Author

@daniel-wer I would like to open up the login popover (top right corner) when the dataset cloud not be loaded. The problem is that the dataset is fetched in the model-initialization. Can you think of a slightly elegant way on how to get the information, that the dataset could not be loaded, to the navbar to show the user login popover?
Using a new field in the store for this seems not appropriate in my opinion.

@philippotto philippotto requested review from philippotto and removed request for daniel-wer March 9, 2020 08:36
@philippotto
Copy link
Member

@MichaelBuessemeyer I'm taking over for @daniel-wer, since he's currently on vacation :)

@philippotto
Copy link
Member

@daniel-wer I would like to open up the login popover (top right corner) when the dataset cloud not be loaded. The problem is that the dataset is fetched in the model-initialization. Can you think of a slightly elegant way on how to get the information, that the dataset could not be loaded, to the navbar to show the user login popover?
Using a new field in the store for this seems not appropriate in my opinion.

I think, using a store field is the easiest solution. Something like state.uiInformation.showLoginMask which the navbar would read (and write to) as well as the model-initialization. Alternatively, only overkill solutions come to my mind (e.g., having an event emitter and listener system).

@MichaelBuessemeyer
Copy link
Contributor Author

By just looking at the code I came across this code. This might be useful to trigger re-fetching the tracing again without reloading it 🤔

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto Do you know a way how to know whether currently a tracing is opened (but not fully loaded), thus when the user logs in the view should be reloaded?
I cannot find any difference between the states of the store during tracing and while the dashboard is open that seems to be sufficient.

@philippotto
Copy link
Member

philippotto commented Mar 10, 2020

@philippotto Do you know a way how to know whether currently a tracing is opened (but not fully loaded), thus when the user logs in the view should be reloaded?
I cannot find any difference between the states of the store during tracing and while the dashboard is open that seems to be sufficient.

Hm, I can think of multiple options:

  • in controller.js, there is a this.state.ready flag which could be moved into the store to check whether a dataset or tracing was loaded completely.
  • check whether the dataset in the store is the default dataset with which the store was initialized. I think, this could be helpful in other places, too (e.g., a sharing token request is send for the "dummy dataset" which always fails).
  • set a new store attribute for "loadingFailed" in the store if the model initialization crashes.

Ok, now that I'm thinking about it a bit, I suggest the following which might be the easiest solution:
Change the ready attribute in Controller to a state attribute which can have the following values: loading, loaded and loadingFailed. Setting the value to loadingFailed can probably be done by extending the catch-handler for Model.fetch in componentDidMount. Then, you simply use the state flag to either display the brain-spinner, the tracing view or the log-in form. Consequently, the log-in form would be displayed in the center of the page (instead of opening the popover in the navbar).

What do you think?

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto Do you think it is helpful to additionally add an error message below the loading brain when the user has logged in but the model initialization still fails or not?

with additional information
image

without additional text
image

The rest is working perfectly 🚀

@philippotto
Copy link
Member

I'd show the text with the brain logo, but without the loading bar (since loading has finished in that case).

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto I cleaned up this PR. So it is ready for your first review 😄

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 stuff! I'd just tune the "either the dataset is missing..." message a bit. (1) I'd center the text and (2) I would add a "Return to dashboard" link to that message so that the user isn't "lost" in this view (the navbar is still there, but that might be not obvious enough).

frontend/javascripts/oxalis/controller.js Outdated Show resolved Hide resolved
@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto What do you mean by

I'd center the text and

Is this what you meant with centering?
image
As you can see, I also added a button to navigate back to the dashboard.

With this, I finished all your requested changes 😄. Could you please review this PR again? 🧐

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! Looking forward to seeing this merged.

@MichaelBuessemeyer MichaelBuessemeyer merged commit 8c9606c into master Mar 16, 2020
@fm3 fm3 deleted the avoid-volume-mode-in-hybrid-tracings branch March 20, 2020 08:57
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.

"Convert to Hybrid" tracing leads to page with "volume" mode activated
2 participants