Skip to content

Conversation

@subdavis
Copy link
Contributor

@subdavis subdavis commented Mar 25, 2022

fixes #850

Screen Shot 2022-04-27 at 4 20 25 PM

Summary of changes

  • Implement group load, display, and filtering support in Dive.
  • Implement KPF group parser in TS (cli only for now)
  • Implement new schema, Dive Json V2, in desktop and web, with migrations on both platforms.
  • Primary key of annotation objects (trackId) is migrated to id for 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 Track and Group variants

  • track.ts becomes BaseAnnotation.
  • useTrackStore becomes BaseAnnotationStore.
  • useFilterControls becomes BaseFilterControls.
  • useStyleManager becomes StyleManager.
  • useTrackSelectionControls generally has moved into useModeManager where it seems to belong.
  • TypeList becomes FilterList and takes generics.
  • Lots of stuff that used to operate on Track now operates on BaseAnnotation
  • FrameDataTrack, 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 like confidencePairs into 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, but deep:true is 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 provide interface 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.

// Before -- Inject state directly
const allTypesRef = useAllTypes();
// After -- Inject the TrackFilter class and access its property.
const allTypesRef = useTrackFilters().allTypes;

// Before -- Inject trackMap and then fetch using helper.
const trackMap = useTrackMap();
const track = getTrack(trackMap, trackId);
// After -- call instance method on trackStore class
const track = useTrackStore().get(trackId);

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. handler still 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 how useModeManager wraps 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 moved useTrackSelectionControls to that file.

Web notes

This change adds a new mongo collection and a migration script in docker/server_setup.py to move the old annotationItem to trackItem, change the id, regenerate indexes, etc.

Test data

I probably forgot to do some stuff IDK.

@subdavis subdavis changed the title Server/activity group support Activity Group Support Mar 29, 2022
@subdavis subdavis force-pushed the server/activity-group-support branch from 751d9bd to 4b42e16 Compare April 11, 2022 19:20
@subdavis subdavis changed the base branch from main to 2.0 April 27, 2022 20:27
@subdavis subdavis marked this pull request as ready for review April 27, 2022 20:28
Copy link
Collaborator

@BryonLewis BryonLewis left a 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:

  1. 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.
  2. 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.
  3. 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.

Comment on lines +102 to +104
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) => {
Copy link
Collaborator

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.

Copy link
Collaborator

@BryonLewis BryonLewis left a 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

@subdavis subdavis force-pushed the server/activity-group-support branch from 0229418 to edc1b00 Compare May 2, 2022 18:07
@subdavis subdavis requested a review from BryonLewis May 2, 2022 18:07
@subdavis
Copy link
Contributor Author

subdavis commented May 2, 2022

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
Copy link
Collaborator

I'm good to go after the final client test is fixed. I just want to note that for the group UI dev I don't think we want tracks that aren't assigned to a group to default to the 'none' category. I think for assigning tracks to a group seeing a mix of unassigned tracks and the groups at the same time may be necessary.

New tracks and unassigned tracks are default 'none' right now with no track type indication.
image

Copy link
Collaborator

@BryonLewis BryonLewis left a 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.

@subdavis subdavis merged commit bd50dea into 1.9 May 4, 2022
@subdavis subdavis deleted the server/activity-group-support branch May 4, 2022 13:02
subdavis added a commit that referenced this pull request May 19, 2022
* 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>
subdavis added a commit that referenced this pull request May 25, 2022
* 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>
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.

[FEATURE] Ability to create activity groups.

2 participants