Skip to content
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

Feature/persistant columns #75

Merged
merged 5 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/core/services/PersistentConfigService/index.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { AnnotationResponse } from "../AnnotationService";

/**
* Keys for the data saved by this service
*/
export enum PersistedConfigKeys {
AllenMountPoint = "ALLEN_MOUNT_POINT",
CsvColumns = "CSV_COLUMNS",
DisplayAnnotations = "DISPLAY_ANNOTATIONS",
ImageJExecutable = "IMAGE_J_EXECUTABLE", // Deprecated
HasUsedApplicationBefore = "HAS_USED_APPLICATION_BEFORE",
UserSelectedApplications = "USER_SELECTED_APPLICATIONS",
Expand All @@ -16,6 +19,7 @@ export interface UserSelectedApplication {

export interface PersistedConfig {
[PersistedConfigKeys.CsvColumns]?: string[];
[PersistedConfigKeys.DisplayAnnotations]?: AnnotationResponse[];
[PersistedConfigKeys.ImageJExecutable]?: string; // Deprecated
[PersistedConfigKeys.HasUsedApplicationBefore]?: boolean;
[PersistedConfigKeys.UserSelectedApplications]?: UserSelectedApplication[];
Expand Down
8 changes: 8 additions & 0 deletions packages/core/state/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { PersistedConfig, PersistedConfigKeys } from "../services/PersistentConf
import interaction, { InteractionStateBranch } from "./interaction";
import metadata, { MetadataStateBranch } from "./metadata";
import selection, { SelectionStateBranch } from "./selection";
import Annotation from "../entity/Annotation";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import belongs with the above grouping (so as a new line above line 6)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this import causes the app to go blank. I am completely lost on why.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled locally and tried this as well, also caused a load error for me when put anywhere from line 8 up:
Uncaught ReferenceError: Cannot access 'Annotation' before initialization


export { interaction, metadata, selection };

Expand Down Expand Up @@ -63,6 +64,13 @@ export function createReduxStore(options: CreateStoreOptions = {}) {
userSelectedApplications:
persistedConfig && persistedConfig[PersistedConfigKeys.UserSelectedApplications],
},
selection: {
displayAnnotations:
persistedConfig &&
persistedConfig[PersistedConfigKeys.DisplayAnnotations]?.map(
(annotation) => new Annotation(annotation)
),
},
});
return configureStore<State>({
middleware: [...(options.middleware || []), ...(middleware || [])],
Expand Down
21 changes: 14 additions & 7 deletions packages/core/state/metadata/logics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
} from "./actions";
import { AnnotationName, TOP_LEVEL_FILE_ANNOTATIONS } from "../../constants";
import AnnotationService from "../../services/AnnotationService";
import Annotation from "../../entity/Annotation";

/**
* Interceptor responsible for turning REQUEST_ANNOTATIONS action into a network call for available annotations. Outputs
Expand All @@ -20,14 +21,25 @@ const requestAnnotations = createLogic({
const { getState, httpClient } = deps;
const applicationVersion = interaction.selectors.getApplicationVersion(getState());
const baseUrl = interaction.selectors.getFileExplorerServiceBaseUrl(getState());
const displayAnnotations = selection.selectors.getAnnotationsToDisplay(getState());
const annotationService = new AnnotationService({
applicationVersion,
baseUrl,
httpClient,
});

let annotations: Annotation[] = [];

try {
const annotations = await annotationService.fetchAnnotations();
annotations = await annotationService.fetchAnnotations();
dispatch(receiveAnnotations(annotations));
} catch (err) {
console.error("Failed to fetch annotations", err);
done();
return;
}

if (!displayAnnotations.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I initially loaded this branch locally, got an error on load that caused the app to fail because displayAnnotations was set to [undefined] which technically has length 1. Brian and I were able to special case around it temporarily, but since then haven't been able to replicate the error. Wanted to check if anyone else (@SeanLeRoy ?) has seen this bug locally?

const defaultDisplayAnnotations = compact([
find(
TOP_LEVEL_FILE_ANNOTATIONS,
Expand All @@ -40,14 +52,9 @@ const requestAnnotations = createLogic({
(annotation) => annotation.name === AnnotationName.FILE_SIZE
),
]);

dispatch(receiveAnnotations(annotations));
dispatch(selection.actions.selectDisplayAnnotation(defaultDisplayAnnotations, true));
} catch (err) {
console.error("Failed to fetch annotations", err);
} finally {
done();
}
done();
},
type: REQUEST_ANNOTATIONS,
});
Expand Down
8 changes: 6 additions & 2 deletions packages/core/state/selection/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,15 +157,19 @@ export const DESELECT_DISPLAY_ANNOTATION = makeConstant(
);

export interface DeselectDisplayAnnotationAction {
payload: Annotation | Annotation[];
payload: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? why

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this made it a little easer to read mirroring the SelectDisplayAnnotationAction so that both have action.payload.annotation as the annotation rather than action.payload for one and action.payload.annotation for another

annotation: Annotation | Annotation[];
};
type: string;
}

export function deselectDisplayAnnotation(
annotation: Annotation | Annotation[]
): DeselectDisplayAnnotationAction {
return {
payload: annotation,
payload: {
annotation,
},
type: DESELECT_DISPLAY_ANNOTATION,
};
}
Expand Down
8 changes: 4 additions & 4 deletions packages/core/state/selection/reducer.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { makeReducer } from "@aics/redux-utils";
import { castArray, difference, omit, without } from "lodash";
import { castArray, difference, omit } from "lodash";

import interaction from "../interaction";
import { AnnotationName, PAST_YEAR_FILTER } from "../../constants";
Expand Down Expand Up @@ -123,9 +123,9 @@ export default makeReducer<SelectionStateBranch>(
sortColumn: action.payload,
}),
[DESELECT_DISPLAY_ANNOTATION]: (state, action) => {
const displayAnnotations = without(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this what without was doing? Why the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was the source of the reselect bug I was talking about, for some reason the annotations returned by the persist are slightly different and this dont get removed with the without function. The same is true for comparing the objects directly. This is what i was saying about not liking my solution as much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like lodash's without function doesn't support this kind of object comparison -- unless the objects share a reference, they aren't considered equal. I'm curious if this use of without worked when the payload to deselectDisplayAnnotation was previously just annotation instead of {annotation}?
But also am fine with the use of filter here

state.displayAnnotations,
...castArray(action.payload)
// remove deselected annotation from state.displayAnnotations
const displayAnnotations = state.displayAnnotations.filter(
(annotation) => annotation.name !== action.payload.annotation.name
);

const columnWidthsToPrune = difference(
Expand Down
19 changes: 10 additions & 9 deletions packages/desktop/src/renderer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { Provider } from "react-redux";

import FmsFileExplorer from "../../../core/App";
import { PersistedConfigKeys } from "../../../core/services";
import { createReduxStore, interaction } from "../../../core/state";
import { createReduxStore, interaction, selection } from "../../../core/state";

import ApplicationInfoServiceElectron from "../services/ApplicationInfoServiceElectron";
import ExecutionEnvServiceElectron from "../services/ExecutionEnvServiceElectron";
Expand Down Expand Up @@ -73,19 +73,20 @@ const store = createReduxStore({
store.subscribe(() => {
const state = store.getState();
const csvColumns = interaction.selectors.getCsvColumns(state);
const displayAnnotations = selection.selectors.getAnnotationsToDisplay(state);
const userSelectedApplications = interaction.selectors.getUserSelectedApplications(state);
const hasUsedApplicationBefore = interaction.selectors.hasUsedApplicationBefore(state);
const appState = {
[PersistedConfigKeys.CsvColumns]: csvColumns,
[PersistedConfigKeys.DisplayAnnotations]: displayAnnotations.map((annotation) => ({
annotationDisplayName: annotation.displayName,
annotationName: annotation.name,
description: annotation.description,
type: annotation.type,
})),
[PersistedConfigKeys.UserSelectedApplications]: userSelectedApplications,
[PersistedConfigKeys.HasUsedApplicationBefore]: true,
};
if (JSON.stringify(appState) !== JSON.stringify(persistentConfigService.getAll())) {
persistentConfigService.persist({
[PersistedConfigKeys.CsvColumns]: csvColumns,
[PersistedConfigKeys.UserSelectedApplications]: userSelectedApplications,
[PersistedConfigKeys.HasUsedApplicationBefore]: hasUsedApplicationBefore,
});
}
persistentConfigService.persist(appState);
});

function renderFmsFileExplorer() {
Expand Down
20 changes: 20 additions & 0 deletions packages/desktop/src/services/PersistentConfigServiceElectron.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,26 @@ const OPTIONS: Options<Record<string, unknown>> = {
type: "string",
},
},
[PersistedConfigKeys.DisplayAnnotations]: {
type: "array",
items: {
type: "object",
properties: {
annotationDisplayName: {
type: "string",
},
annotationName: {
type: "string",
},
description: {
type: "string",
},
type: {
type: "string",
},
},
},
},
// ImageJExecutable is Deprecated
[PersistedConfigKeys.ImageJExecutable]: {
type: "string",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,17 @@ describe(`${RUN_IN_RENDERER} PersistentConfigServiceElectron`, () => {
name: "ZEN",
},
];

const expectedDisplayAnnotations = [
{
annotationDisplayName: "Foo",
annotationName: "foo",
description: "foo-long",
type: "string",
units: "string",
},
];

service.persist(PersistedConfigKeys.AllenMountPoint, expectedAllenMountPoint);
service.persist(PersistedConfigKeys.CsvColumns, expectedCsvColumns);
service.persist(PersistedConfigKeys.ImageJExecutable, expectedImageJExecutable);
Expand All @@ -53,12 +64,15 @@ describe(`${RUN_IN_RENDERER} PersistentConfigServiceElectron`, () => {
expectedHasUsedApplicationBefore
);
service.persist(PersistedConfigKeys.UserSelectedApplications, expectedUserSelectedApps);
service.persist(PersistedConfigKeys.DisplayAnnotations, expectedDisplayAnnotations);

const expectedConfig = {
[PersistedConfigKeys.AllenMountPoint]: expectedAllenMountPoint,
[PersistedConfigKeys.CsvColumns]: expectedCsvColumns,
[PersistedConfigKeys.ImageJExecutable]: expectedImageJExecutable,
[PersistedConfigKeys.HasUsedApplicationBefore]: expectedHasUsedApplicationBefore,
[PersistedConfigKeys.UserSelectedApplications]: expectedUserSelectedApps,
[PersistedConfigKeys.DisplayAnnotations]: expectedDisplayAnnotations,
};

// Act
Expand All @@ -85,6 +99,15 @@ describe(`${RUN_IN_RENDERER} PersistentConfigServiceElectron`, () => {
name: "ImageJ/Fiji",
},
],
[PersistedConfigKeys.DisplayAnnotations]: [
{
annotationDisplayName: "Foo",
annotationName: "foo",
description: "foo-long",
type: "string",
units: "string",
},
],
};

// Act
Expand Down
Loading