-
Notifications
You must be signed in to change notification settings - Fork 23
Activity Group Support #1205
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
Activity Group Support #1205
Conversation
751d9bd to
4b42e16
Compare
BryonLewis
left a comment
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 doing all of this work. I haven't looked over the testing in detail yet but I'll do that next.
Overall I had 3 higher level questions:
- The question about group confidence levels and if they should play any role in the visualization/editing. I know this is probably a Roddy question. I only bring it up because there are some assumptions that the confidence level will always be 1.0 in the interactions.
- A better indication of the one-way relationship between groups and tracks. The groups seem to be a forefront and control which tracks are visible. The tracks don't seem to have an opposite effect on the groups. Deleting tracks or hiding them doesn't change the count of the groups in the display. It also doesn't indicate a group is empty if you happen to delete all tracks of that group. This is more complicated stuff that we may not want to handle right now.
- While it is easier for implementation I don't know if GroupStyles should just be a direct copy of TypeStyles and override all of the settings. I'm wondering if there should be a visible way to determine the type and group of annotation instead of relying on the text data alone. Maybe having the group only effect the group text color. It just seems like the 100% overlap between typeStyles and groupStyles could make it a bit confusing when displaying the two or switching between them.
| const text = ['This will remove the type from any visible track or delete the track if it is the only type.', | ||
| 'Do you want to delete all tracks of following types:']; | ||
| checkedTypesRef.value.forEach((item) => { |
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.
This text may need some work to better distinguish between what it does in groups vs what it does with Types.
In types it will actually delete the track.
In groups it will delete the 'group' instead.
BryonLewis
left a comment
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.
Sorry forgot to add this comment in there
0229418 to
edc1b00
Compare
|
I think I've fixed the things that need fixing on client and server. Integration tests are now passing. Client unit tests are passing except for that one test. |
BryonLewis
left a comment
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 the only remaining things for the next PR are resolving the Group-Style editor text and making sure that tracks that aren't assigned to groups to something logical when it group 'viewing' mode.
* Activity Groups * Move Image Enhancement * wip * GroupItem * Refactor Filter Controls * Complete backend support for activity groups with migrations * Refactor of GroupStore, Filter Controls, Filter List * Group filtering support * test fixes * sidebar * sidebar update * Respond to comments * Fix tests * fixing final test (#1234) Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com>
* Activity Group Support (#1205) * Activity Groups * Move Image Enhancement * wip * GroupItem * Refactor Filter Controls * Complete backend support for activity groups with migrations * Refactor of GroupStore, Filter Controls, Filter List * Group filtering support * test fixes * sidebar * sidebar update * Respond to comments * Fix tests * fixing final test (#1234) Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com> * Release 1.9.0-beta.1 * Group Creation (#1237) * WIP Group Creation * Edit type and select. Refactor TypePicker * Progress toward creation and editing of activity groups * Group membership add and remove * Update package.json * Complete Activity Group Support (#1238) * Progress on activity group editing * Progress on correct editing behavior * Finalize editing * Documentation updates * Update documentation * Add shortcut docs * Wip * KPF import support desktop * icon * Fix docs add tests * whatdo * Remove multi-file inputs * Upgrade node for electron-builder * Respond to comments * Backend fixes * Add readOnlyMode to TypePicker in GroupList * Update package.json * Include buildResources * Server/kpf support v2 (#1235) * Work in progress * Finalize KPF support * Fully move to new annotation schema in all parsers * Liint fix * Add extra meta in zip extraction * Fix * Fix merge conflict Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com>

fixes #850
Summary of changes
trackId) is migrated toidfor consistency between track and group.Summary of Approach
The general goal in this PR is to refactor a significant chunk of the core data and logic that dealt exclusively with tracks to make generic, abstract base classes that could be extended as
TrackandGroupvariantstrack.tsbecomesBaseAnnotation.useTrackStorebecomesBaseAnnotationStore.useFilterControlsbecomesBaseFilterControls.useStyleManagerbecomesStyleManager.useTrackSelectionControlsgenerally has moved intouseModeManagerwhere it seems to belong.TypeListbecomesFilterListand takes generics.BaseAnnotationFrameDataTrack, the interface to an on-screen annotation, gets expanded to include references to groups. I also back out the unnecessary "performance enhancement" of destructuring properties likeconfidencePairsinto that interface and just pass the track reference wholesale. This performance enhancement was useless and based on an incomplete understanding of computed properties, which are always shallow refs. (They could still be watched deeply, which would be very bad for performance, butdeep:trueis such a red flag to me at this point that I don't see much risk in making this change -- it would be caught in review)Instead of taking the return values of each composition function and breaking them down into reactive state and handlers, the
provideinterface is simplified by taking instances of the classes above (2 each, for tracks and groups).In general, this change moves away from abusing composition functions and toward more standard OOP. Here's an example.
This does have one glaring disadvantage: the ability to wrap functions (like
removeTrack, for example) in contextual behavior (like showing a dialog) is now less obvious when and where it should be done.handlerstill exists, and its functions should still be used to trigger mutations. It would be great to have some way to register a chain of side-effects on certain kinds of state mutations or come up with a more sane theory of howuseModeManagerwraps or does not wrap mutations in other modules. Resolving this design difficulty will make a huge improvement on the quality of the code.For now, the rule I've tried to follow is if it can be called directly (in an AnnotationStore, FilterControls, etc) do that. Otherwise, make a wrapper function in
useModeManager. This is partly why I moveduseTrackSelectionControlsto that file.Web notes
This change adds a new mongo collection and a migration script in
docker/server_setup.pyto move the oldannotationItemtotrackItem, change the id, regenerate indexes, etc.Test data
I probably forgot to do some stuff IDK.