-
Notifications
You must be signed in to change notification settings - Fork 24
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 tags for datasets #5832
Add tags for datasets #5832
Conversation
@MichaelBuessemeyer The backend part should be done. I could only test it in a limited way so far, as there is no frontend yet. If you encounter problems, feel free to ping me :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend mostly LGTM, please see my two comments @fm3:
@@ -98,6 +98,7 @@ CREATE TABLE webknossos.dataSets( | |||
logoUrl VARCHAR(2048), | |||
sortingKey TIMESTAMPTZ NOT NULL, | |||
details JSONB, | |||
tags VARCHAR(256)[] NOT NULL DEFAULT '{}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also use the json
or jsonb
type (seems like it's available from pg 9.6). It's also possible to create indices on jsonb columns and query them efficiently (e.g. when searching/selecting by tag):
https://www.postgresql.org/docs/9.4/datatype-json.html#JSON-INDEXING
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also totally fine as-is for now I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The front-end code looks great 👍 However, I think that I've found a potential race condition (see my comment).
Other than that, I have some UI suggestions:
- turn the cursor into a pointer when hovering the tags (via CSS)
- show a tooltip which explains the tag interactions (e.g., "Click to only show datasets with this tag", "Add a new tag", "Remove this tag from this dataset")
Would be great if you could generalize these UI improvements so that the tags for the explorative annotations also profit from these :)
if (isLoading) return; | ||
setIsLoading(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I change the tags for two datasets and don't wait for the first update to be finished, the second update will get lost then, right? I think, a clean solution would be to have some sort of "mutex promise" which is either null (then, one can execute the update) or it's a promise which needs to be awaited. Also, maybe not the entire function needs to block on that promise (but instead only the await updateDataset
part maybe?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this. I implemented a queueing like behaviour for all updates on the same dataset. This queuing ensures that each previous request is finished before the current request starts. Therefore it is ensured that the backend always gets the updates in the correct order. Thus no update call that (by network magic) was faster at the server gets lost because a slower update request that is much slower overwrites its changes afterwards.
Here is a screenshot of a few dataset changes made on a single dataset in a row by changing the tags of a single dataset. The connection is throttled very strongly to test this corner case. The graph on the right illustrates that one call only starts after the previous updates finished.
frontend/javascripts/dashboard/dataset/dataset_cache_provider.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Philipp Otto <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, looks merge-ready to me :)
Is it correct, that I had to adjust the version of the If yes, hopefully the CI works now 😅 |
frontend/javascripts/dashboard/dataset/dataset_cache_provider.js
Outdated
Show resolved
Hide resolved
* Add tags for datasets * update schema version * add tag column to dataset table * add filtering and persistence for dataset tags * unify tag handling for datasets and explorative annotations view * adjust version of migration * adjust revision of dataset migration * ensure qdataset updates to be in fixed order * Add tags for datasets * undo accidental changes due to merging * Update frontend/javascripts/dashboard/dataset/dataset_cache_provider.js Co-authored-by: Philipp Otto <[email protected]> * update version in schema.sql manually * fix flow Co-authored-by: Michael Büßemeyer <[email protected]> Co-authored-by: MichaelBuessemeyer <[email protected]> Co-authored-by: Michael Büßemeyer <[email protected]> Co-authored-by: Philipp Otto <[email protected]>
Adding a tag system to Datasets, analogous to what Annotations already have.
URL of deployed dev instance (used for testing):
Backend Changes
tags
(list of string)tags
as well (it being missing will be interpreted as empty list), changes tags in databaseSteps to test:
Issues: