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

add button to reload data buckets for each layer #4383

Merged
merged 8 commits into from
Jan 13, 2020

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Dec 17, 2019

This PR adds a button to reload all cached buckets of a data layer in the frontend.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open a tracing and the dataset settings.
  • Click on the reload icon above the histogram of any layer.
  • A toast should indicate the success of dropping the cached buckets.
  • Now move a bit around to trigger the reloading of the bucket.
  • If there are multiple layers, only the data buckets of the selected layer should be reloaded.

Issues:


@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Dec 17, 2019

ToDo:

  • Check support for segmentation layer

@fm3
Copy link
Member

fm3 commented Dec 17, 2019

I think the backend should also be adapted (change the clear bucket cache api so it supports selecting the layer), right?

@philippotto
Copy link
Member

I think the backend should also be adapted (change the clear bucket cache api so it supports selecting the layer), right?

If it's quick to implement, then, yes. Otherwise, it's not that big of a deal to clear the entire dataset, I think. As long as the local buckets are not discarded for all layers, the UX will be fine, at least.

await clearCache(this.props.dataset);
api.data.reloadBuckets(layerName);
Toast.success(
`Successfully deleted cached data of layer ${layerName}. Now move a bit around to reload the data.`,
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't force the user to move around to see the new data. Setting window.needsRerender = true; above should do the trick here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not work for me. Do you have any other suggestions or know why it might not be working?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, let's have a look together at this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe this helps app.vent.trigger("rerender"); 🤷‍♂️ . Just saw it in our code base.

Copy link
Member

Choose a reason for hiding this comment

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

I had a look and fixed it now :) The layer rendering manager had to be refreshed.

@youri-k
Copy link
Contributor

youri-k commented Dec 20, 2019

@MichaelBuessemeyer I just pushed the necessary changes, so the backend can clear the cache of one layer only. I also added the new layerName attribute to the routes in the admin_rest_api. Hope this helps 🙂

@youri-k youri-k requested a review from fm3 December 20, 2019 13:04
@youri-k
Copy link
Contributor

youri-k commented Dec 20, 2019

@fm3 could you review the backend part? Thanks 😄

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Backend LGTM

@MichaelBuessemeyer
Copy link
Contributor Author

ToDo:

  • Check support for segmentation layer

works perfectly.

@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review January 7, 2020 13:53
@MichaelBuessemeyer
Copy link
Contributor Author

@MichaelBuessemeyer I just pushed the necessary changes, so the backend can clear the cache of one layer only. I also added the new layerName attribute to the routes in the admin_rest_api. Hope this helps 🙂

Thanks a lot @youri-k

@philippotto
Copy link
Member

@MichaelBuessemeyer Awesome stuff! Only a minor request from my side:

Could you move the reload/find-data buttons so that they align with the right side of the histogram? Right now, it looks a bit weird in my opinion 🙈

image

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto Could you please review my changes? I arranged the buttons with the right edge of the histogram.

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! I'm very looking forward to this feature :)

@MichaelBuessemeyer MichaelBuessemeyer merged commit 242cd0c into master Jan 13, 2020
@fm3 fm3 deleted the reload-layer-data-button branch March 20, 2020 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "reload" button per layer
4 participants