-
Notifications
You must be signed in to change notification settings - Fork 1
Adds feature for filtering annotations by shapes and user in Largo #193
Conversation
…e, and integrate with the component. The component is still not working as expected, however API routes are working.
This feature should be improved by making it load shapes from the DB on load rather than on click. Temporary checkpoint because of change of pc.
This commit implements the filtering functionality for the tab, which now fetches only the images required. For now the only filter implemented is the `shape` filter, but this implementation should allow for an easy expansion with any other filter.
This reverts commit fd29dd1. This commit inadvertently introduces an autoformatting that makes the diffs unreadable.
Readds filtering functionality for shapes, that allows for expanding with other filters.
This commit implements the possibility of filtering images by users. Implements the select button as well as the js components required.
…unctions In this commit, code that was previously imcomplete (e.g. API calls, filtering tab loading user names incorrectly) gets fixed by refining functions and missing API calls.
Wow. Cool PR. Thanks Davide |
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.
Awesome, thanks! High-level comments first (in no particular order). I'll look at the code in a later review.
-
The BIIGLE modules track their compiled assets (JS/CSS) as well. So once you have something ready to review/publish, please run
npm run prod
once and commit the minified assets. Otherwise you end up with a 4k lines JS file like now. -
When a filter is active, the sidebar tab must be highlighted. Compare this with the sorting tab when a sorting is active:
This is important so users don't forget about the filters once the tab is closed.
-
There is no need to make an API call to fetch the available annotation shapes. You can hard-code them in the template. Either directly as HTML or you populate a JS variable (with
biigle.$declare()
andbiigle.$require()
). Also, if Largo is opened for a single volume and it is not a video volume, the WholeFrame shape should not be shown. -
To make the objective speed faster, you can perform the API call to fetch available users when the filter tab is open (instead of when the select input is focussed). Then the response will hopefully be already there when the user selects the input.
-
Ideally, I want the filter tab here to work similar to the filter tab of the volume overview (except the flag/filter switch and the show filenames button):
If you want you can implement it this way here. Otherwise we can keep your UI with the select inputs as first version and then update the UI later.
There were two things that didn't work for me:
-
When I open Largo for a whole project, I get an empty response from the "users-with-annotations" API endpoint.
-
When I click on a label (with annotations) and thenn immediately click on another label (without annotations), it shows the message "no annotations" for both labels. I even see a flash of annotation patches of the first label while it loads. But somehow the caching is messed up afterwards. I don't see this behavior in the version on biigle.de so I assume that it has something to do with your changes. Here is an example:
Screencast.from.04.11.2024.14.42.57.webm
This commit introduces highlighting to the filters tab by using the activeFilters property
…alues of these filters This commit changes fundamentally the way the filters are loaded; in particular, they are loaded not on click, rather on page load. To do so, the filters are loaded and then injected inside the template view, removing the need of further API calls.
This commit introduces a system that allows the selection of multiple values in the filter tab, so that it is now possible to combine multiple filters. This comes with logical operators (is, is not, or, and). Changes the functioning of the API endpoints for filters of users and shapes as well. In a further commit, tests will be introduced.
I am not able to run
This should be done now!
Done for the most part, need to remove the WholeFrame shape. EDIT: I fixed this part
I managed to add this as the page loads, lmk if it seems good to you!
This took almost the whole week, but it seems to work now. Not the prettiest, but I'd like to now if the way it functions now is good for you. It was fun, and I hope the code is not too messy.
Since there is no API anymore, it should not appear now!
I was not able to reproduce this, but then again with all the new changes a lot has been modified and it could be that I inadvertently fixed it while changing something else. Thanks for the feedback! |
… with filters This commit makes the loading of filtered annotations into a different promise. A bug of pinned images showing up during filtering is also handled.
…ifferent than expected This commit solves a problem where, if adding a `is` and `is not` filter for the same user/shape caused the union not to work correctly. As an example, when adding in `union` mode filter `is Point` and `is not Point`, then we expect the filter to always return the entire annotation catalog. If new filters are added in this case, this should not change anything, as they should act as inclusive filters. New tests are added for this case.
… resetting filters
This commit removes a bug that caused an image to appear when no annotations where loaded through filters. It also removes the now useless reference to the filteringTab from the largoContainer
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.
I think I found another case with flickering thumbnails but it is quite hard to reproduce. I'm quite sure I repeated the steps that caused the flickering but the second time it didn't happen...
Maybe you can have another look at possible race conditions?
src/Http/Controllers/Api/Projects/GetUsersWithAnnotationsProject.php
Outdated
Show resolved
Hide resolved
src/Http/Controllers/Api/Volumes/GetUsersWithAnnotationsVolume.php
Outdated
Show resolved
Hide resolved
src/Http/Controllers/Api/Projects/GetUsersWithAnnotationsProject.php
Outdated
Show resolved
Hide resolved
src/Http/Controllers/Api/Volumes/GetUsersWithAnnotationsVolume.php
Outdated
Show resolved
Hide resolved
This commit refactors the filtering tab and the largoContainer so that activeFilters and union values are not managed by the tab, rather they are now a prop passed by the tab to the tab. The filtering tab now passes on the events by the annotationFilter to the largoContainer, which now manages it. This should clarify the hierarchy of elements, reduce the number of repeated lines and allow the largoContainer the capacity of handling the filters directly in case of events.
@mzur I have not been able to reproduce the flickering... While the page is loading, it should not display anything, so I don't think race conditions play a role here. |
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.
I found the flickering issue! Steps:
- Add a filter rule that matches annotations of two different labels.
- Select first one label, then the other to cache the filtered annotations.
- Add another filter rule that still matches annotations of both labels.
- Select the label that is currently not selected. Some patches flicker.
Screencast.from.2025-04-24.15-32-55.webm
Co-authored-by: Martin Zurowietz <[email protected]>
Co-authored-by: Martin Zurowietz <[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.
Looks good. Thanks for all the work 👍
This PR (partially) addresses issue biigle/core#1166 by introducing a filtering tab in Largo for users and shapes.
I also introduced tests for the new filtering functionalities on the backend side.
What I hoped to achieve, was a system that could be expanded upon. Please let me know if this is satisfactory.
Something that I am not 100% convinced is the fact that the filters get loaded on click - I was not able to pass the information from the API in any other way. Is it okay to you?
To test this, try to use filters to change the displayed largo annotations and check that no unexpected behaviour is present (e.g. annotations that should not be there that appear, other largo features stop working etc.)