Feature/persistant annotations #85

BrianWhitneyAI · 2024-05-07T20:55:21Z

Description of Changes

This PR proposes prioritizing recently used Groups, Filters, and Sorts in the ordering of their respective selection dropdowns ( resolves #57 ). This PR is a continuation of #76 feature/recent-hierarchy-annotations , but is based on the new generalize-ui #79 branch.

Testing

Monitored state through various selections and deselections of Groups, Filters, and Sorts including those with repeated no-file-found options.

I did experience some issues with reloading the application where it would disconnect on the first reload but succeed on the second reload. I believe this is an inherent issue to the base branch or my development environment as is still present without these changes.

This PR also passes tests locally with the exception of

  9) @renderer FileDownloadServiceElectron
       promptForDownloadDirectory
         complains about non-writeable directory when given:
     Error: Trying to stub property 'invoke' of undefined

I believe that this is unique to my local environment since I had to use WSL to run the full test suite. In reviewing this PR it would be super helpful to run tests locally with your dev env just to make sure everything is Ok.

Possible Changes/Adjustments

Limiting the length of recent annotations to a smaller number, I foresee a situation where a user has a lot of interaction and the recent list becomes very long (and unalphabetized) and this feature becomes a hindrance
A adjustment to ‘On Start” state to not include undefined elements.

…atch Test

…llenInstitute/aics-fms-file-explorer-app into feature/persistant-annotations

aswallace

Ran tests locally and they worked fine for me! Left a few comments

Limiting the length of recent annotations to a smaller number, I foresee a situation where a user has a lot of interaction and the recent list becomes very long (and unalphabetized) and this feature becomes a hindrance

I'm not quite sure what you mean by this one. I did quite a bit of interaction locally and the recent list stayed limited, especially since the non-selected recents are hidden when other filters are selected (e.g., if 3 filters are selected then only 2 recents show because of the slicing)

aswallace · 2024-05-13T21:03:12Z

packages/core/components/AnnotationPicker/index.tsx

    const unavailableAnnotations = useSelector(
        selection.selectors.getUnavailableAnnotationsForHierarchy
    );
    const areAvailableAnnotationLoading = useSelector(
        selection.selectors.getAvailableAnnotationsForHierarchyLoading
    );
+    const recentAnnotatitonNames = useSelector(selection.selectors.getRecentAnnotations);


nit: Annotation here and on lines 41 and 70

aswallace · 2024-05-13T21:20:11Z

packages/core/state/selection/selectors.ts

@@ -76,3 +77,22 @@ export const getUnavailableAnnotationsForHierarchy = createSelector(
            allAnnotations.filter((annotation) => !availableAnnotations.includes(annotation.name))
        )
 );
+
+// COMPOSED SELECTORS


nit: This is a duplicate comment since it's already in the composed selectors section

aswallace · 2024-05-13T21:47:00Z

packages/core/state/selection/test/reducer.test.ts

-                ...initialState,
-                selection: nextSelectionState,
-            });
+describe("Selection reducer", () => {


Minor: can remove lines 43-45 since this should be just be a separate it in the same describe, not a new describe

Alternately, can keep the previous test format and reduce duplicate code by doing something like

Suggested change

describe("Selection reducer", () => {

describe("Selection reducer", () => {

[

{

actionConstant: selection.actions.SET_ANNOTATION_HIERARCHY,

expectedAction: selection.actions.setAnnotationHierarchy([])

},

{

actionConstant: interaction.actions.SET_FILE_EXPLORER_SERVICE_BASE_URL,

expectedAction: interaction.actions.setFileExplorerServiceBaseUrl("base")

}

].forEach(({actionConstant, expectedAction}) =>

it(`clears selected file state when ${actionConstant} is fired`, () => {

// arrange

const prevSelection = new FileSelection().select({

fileSet: new FileSet(),

index: new NumericRange(1, 3),

sortOrder: 0,

});

const initialSelectionState = {

...selection.initialState,

fileSelection: prevSelection,

};

// act

const nextSelectionState = selection.reducer(

initialSelectionState,

expectedAction

)

const nextSelection = selection.selectors.getFileSelection({

...initialState,

selection: nextSelectionState,

});

// assert

expect(prevSelection.count()).to.equal(3); // consistency check

expect(nextSelection.count()).to.equal(0);

}));

aswallace · 2024-05-13T23:29:22Z

packages/core/components/ListPicker/ListRow.tsx

@@ -37,6 +39,7 @@ export default function ListRow(props: Props) {
            className={classNames(styles.itemContainer, {
                [styles.selected]: item.selected,
                [styles.disabled]: item.disabled,
+                [styles.isBuffer]: item.isBuffer,
            })}
            menuIconProps={{
                iconName: props.subMenuRenderer ? "ChevronRight" : undefined,


Small visual bug where the ChevronRight icon tries to render on the buffer line for the Filter submenu since it still has a subMenuRenderer prop

packages/core/components/ListPicker/ListRow.tsx

aswallace · 2024-05-13T23:59:37Z

packages/core/components/AnnotationPicker/index.tsx

    const unavailableAnnotations = useSelector(
        selection.selectors.getUnavailableAnnotationsForHierarchy
    );
    const areAvailableAnnotationLoading = useSelector(
        selection.selectors.getAvailableAnnotationsForHierarchyLoading
    );
+    const recentAnnotatitonNames = useSelector(selection.selectors.getRecentAnnotations);
+    const recentAnnotations = recentAnnotatitonNames.flatMap((name) =>
+        annotations.filter((annotation) => annotation.name === name)


I'm curious if there was a reason we're making recentAnnotations (the one defined in state/store, not the const defined here) an array of strings instead of Annotation[] since the only time the getter is accessed so far is here where it immediately gets used to filter back to type Annotation[]

Edit: I see that it's likely because we're also needing to do comparisons to other types in selections/reducer, so I take back my question 😅 . I'll leave it here for posterity though

aswallace · 2024-05-14T00:11:42Z

packages/core/components/AnnotationPicker/index.tsx

-import { metadata, selection } from "../../state";
+import Annotation, { AnnotationName } from "../../entity/Annotation";
+import { selection } from "../../state";
+import { uniqBy } from "lodash";

 interface Props {
    disabledTopLevelAnnotations?: boolean;


Line 14: outside the scope of this PR, but I'm unsure if the disableUnavailableAnnotations prop still does anything?

kmitcham · 2024-05-15T16:49:19Z

packages/core/components/ListPicker/index.tsx

@@ -160,7 +160,8 @@ export default function ListPicker(props: ListPickerProps) {
            </div>
            <div className={styles.footer}>
                <h6>
-                    Displaying {filteredItems.length} of {items.length} Options
+                    {/* (item.lenght -1) to account for buffer in item list. */}


nit: length

SeanLeRoy

I think resolving Anya's comments and getting File Name moved would make this good to go to me

BrianWhitneyAI · 2024-05-21T23:52:14Z

closing in favor of #110

BrianWhitneyAI and others added 12 commits May 2, 2024 16:07

feature/persistant-annotations

fe33d2e

maintain recent ordering

289829a

update tests to match feature

0f3d65b

Test

f37e12e

Merge pull request #84 from AllenInstitute/feature/test-workflow-disp…

fac16ff

…atch Test

Test

73122d0

Test

03d47e8

Test

4c392a0

feature/persistant-annotations

f5f61e7

maintain recent ordering

5c21ddc

update tests to match feature

ccdca1d

Merge branch 'feature/persistant-annotations' of https://github.com/A…

9e27469

…llenInstitute/aics-fms-file-explorer-app into feature/persistant-annotations

SeanLeRoy force-pushed the master branch from 0f5c656 to 5df48e3 Compare May 7, 2024 21:02

BrianWhitneyAI added 6 commits May 7, 2024 14:31

simplify reducer

7cc9af7

limit recent annotations to 5

30669d8

bugfix: undefined elemements

2eae870

simplify map reduce

836add9

remove typing of implied types

14add36

add divider to annotation list

f549e2d

BrianWhitneyAI requested review from SeanLeRoy, aswallace, pgarrison and kmitcham May 9, 2024 20:18

aswallace approved these changes May 14, 2024

View reviewed changes

kmitcham reviewed May 15, 2024

View reviewed changes

SeanLeRoy approved these changes May 20, 2024

View reviewed changes

BrianWhitneyAI closed this May 21, 2024

BrianWhitneyAI deleted the feature/persistant-annotations branch May 21, 2024 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/persistant annotations #85

Feature/persistant annotations #85

BrianWhitneyAI commented May 7, 2024

aswallace left a comment

aswallace May 13, 2024

aswallace May 13, 2024

aswallace May 13, 2024

aswallace May 13, 2024

aswallace May 13, 2024

aswallace May 13, 2024

aswallace May 14, 2024

kmitcham May 15, 2024

SeanLeRoy left a comment

BrianWhitneyAI commented May 21, 2024

-describe("Selection reducer", () => {
+describe("Selection reducer", () => {
+    [
+        {
+            actionConstant: selection.actions.SET_ANNOTATION_HIERARCHY,
+            expectedAction: selection.actions.setAnnotationHierarchy([])
+        },
+        {
+            actionConstant: interaction.actions.SET_FILE_EXPLORER_SERVICE_BASE_URL,
+            expectedAction: interaction.actions.setFileExplorerServiceBaseUrl("base")
+        }
+    ].forEach(({actionConstant, expectedAction}) =>
+        it(`clears selected file state when ${actionConstant} is fired`, () => {
+            // arrange
+            const prevSelection = new FileSelection().select({
+                fileSet: new FileSet(),
+                index: new NumericRange(1, 3),
+                sortOrder: 0,
+            });
+            const initialSelectionState = {
+                ...selection.initialState,
+                fileSelection: prevSelection,
+            };
+            // act
+            const nextSelectionState = selection.reducer(
+                initialSelectionState,
+                expectedAction
+            )
+            const nextSelection = selection.selectors.getFileSelection({
+                ...initialState,
+                selection: nextSelectionState,
+            });
+            // assert
+            expect(prevSelection.count()).to.equal(3); // consistency check
+            expect(nextSelection.count()).to.equal(0);
+        }));

Feature/persistant annotations #85

Feature/persistant annotations #85

Conversation

BrianWhitneyAI commented May 7, 2024

Description of Changes

Testing

Possible Changes/Adjustments

aswallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanLeRoy left a comment

Choose a reason for hiding this comment

BrianWhitneyAI commented May 21, 2024