apply various feedback

Author: MichaelBuessemeyer

frontend/javascripts/admin/rest_api.ts

@@ -2050,9 +2050,7 @@ export async function getAgglomeratesForSegmentsFromDatastore<T extends number |
     );
   });
   // Ensure that the values are bigint if the keys are bigint
-  const adaptToType = Utils.isBigInt(segmentIds[0])
-    ? (el: NumberLike) => BigInt(el)
-    : (el: NumberLike) => el;
+  const adaptToType = Utils.getAdaptToTypeFunctionFromList(segmentIds);
   const keyValues = _.zip(segmentIds, parseProtoListOfLong(listArrayBuffer).map(adaptToType));
   // @ts-ignore
   return new Map(keyValues);
@@ -2094,9 +2092,7 @@ export async function getAgglomeratesForSegmentsFromTracingstore<T extends numbe
   });
 
   // Ensure that the values are bigint if the keys are bigint
-  const adaptToType = Utils.isBigInt(segmentIds[0])
-    ? (el: NumberLike) => BigInt(el)
-    : (el: NumberLike) => el;
+  const adaptToType = Utils.getAdaptToTypeFunctionFromList(segmentIds);
 
   const keyValues = _.zip(segmentIds, parseProtoListOfLong(listArrayBuffer).map(adaptToType));
   // @ts-ignore

frontend/javascripts/libs/utils.ts

@@ -8,7 +8,7 @@ import type { BoundingBoxMinMaxType } from "types/bounding_box";
 import type { ArbitraryObject, Comparator } from "types/globals";
 import type { ColorObject, Point3, TypedArray, Vector3, Vector4, Vector6 } from "viewer/constants";
 import type { TreeGroup } from "viewer/model/types/tree_types";
-import type { BoundingBoxObject, NumberLike, SegmentGroup } from "viewer/store";
+import type { BoundingBoxObject, Mapping, NumberLike, SegmentGroup } from "viewer/store";
 
 type UrlParams = Record<string, string>;
 
@@ -1257,6 +1257,16 @@ export function isNumberMap(x: Map<NumberLike, NumberLike>): x is Map<number, nu
   return Boolean(typeof value[0] === "number");
 }
 
+export function getAdaptToTypeFunction(mapping: Mapping | null | undefined) {
+  return mapping && isNumberMap(mapping) ? (el: number) => el : (el: number) => BigInt(el);
+}
+
+export function getAdaptToTypeFunctionFromList<T extends number | bigint>(list: Array<T>) {
+  return list[0] == null || Boolean(typeof list[0] === "number")
+    ? (el: NumberLike) => el
+    : (el: NumberLike) => BigInt(el);
+}
+
 export function isBigInt(x: NumberLike): x is bigint {
   return typeof x === "bigint";
 }

frontend/javascripts/test/sagas/proofreading/proofreading_skeleton_interaction.spec.ts

@@ -767,3 +767,4 @@ describe("Proofreading (With Agglomerate Skeleton interactions)", () => {
 // TODOM: write tests for cutFromAllNeighbours -> new test file
 // TODOM: write tests for partitionedMinCut -> new test file
 // TODOM: Write a test that pulling update fro the backend does not yield diff tracing changes that would be pushed to the backend in a live collab scenario!
+// TODOM: Write test without injected version and test via spy or so that no rebasing was tried and thus only the update actions were sent to the server!

frontend/javascripts/viewer/model/reducers/save_reducer.ts

@@ -210,10 +210,6 @@ function SaveReducer(state: WebknossosState, action: Action): WebknossosState {
       });
     }
 
-    case "SET_MAPPING": {
-      return state;
-    }
-
     case "PREPARE_REBASING": {
       const rebaseInfo = state.save.rebaseRelevantServerAnnotationState;
       console.error("Setting isRebasing = true");

frontend/javascripts/viewer/model/sagas/saving/save_saga.ts

@@ -1,7 +1,7 @@
 import { getAgglomeratesForSegmentsFromTracingstore, getUpdateActionLog } from "admin/rest_api";
 import ErrorHandling from "libs/error_handling";
 import Toast from "libs/toast";
-import { isNumberMap, sleep } from "libs/utils";
+import { getAdaptToTypeFunction, sleep } from "libs/utils";
 import _ from "lodash";
 import { buffers, type Channel } from "redux-saga";
 import { actionChannel, call, delay, flush, fork, put, race, takeEvery } from "typed-redux-saga";
@@ -404,8 +404,7 @@ export function* tryToIncorporateActions(
         case "updateUserBoundingBoxVisibilityInVolumeTracing":
         case "updateSegmentGroupsExpandedState": {
           if (areUnsavedChangesOfUser) {
-            // TODOM: Missing actions still need support in applyVolumeUpdateActionsFromServerAction
-            // TODOM: --------------------- User specific actions must be reapplied if local actions!!!!!! ------------------------------------------
+            // Maybe write tests for these? Maybe part of M4
             yield* put(applyVolumeUpdateActionsFromServerAction([action]));
           }
           break;
@@ -603,53 +602,61 @@ export function* tryToIncorporateActions(
   return { success: true };
 }
 
+type IdsToReloadPerMapping = Record<string, number[]>;
+
+// Gathers mapped agglomerate ids for unknown but relevant segments to apply the passed save queue entries correctly.
+// This is needed in case proofreading was done via mesh interactions whose mapping info is present in the meshes
+// but not in the activeMappingByLayer.mapping. Due to incorporating backend updates the agglomerate ids of the
+// meshes might be outdated, thus we reload this info and store it in the local mapping to perform the correct merge.
+// Returns a list of segment ids to reload for each needed volume / editable tracing id.
 function* getAllUnknownSegmentIdsInPendingUpdates(
   saveQueue: SaveQueueEntry[],
-): Saga<Record<string, number[]>> {
+): Saga<IdsToReloadPerMapping> {
   const activeMappingByLayer = yield* select(
     (store) => store.temporaryConfiguration.activeMappingByLayer,
   );
-  const idsToRequest = {} as Record<string, number[]>;
+  const idsToRequestByLayerId = {} as IdsToReloadPerMapping;
   for (const saveQueueEntry of saveQueue) {
     for (const action of saveQueueEntry.actions) {
       switch (action.name) {
         case "mergeAgglomerate":
         case "splitAgglomerate": {
           const { segmentId1, segmentId2, actionTracingId } = action.value;
-          const upToDateMapping = activeMappingByLayer[actionTracingId]?.mapping;
-          if (!upToDateMapping || segmentId1 == null || segmentId2 == null) {
+          const mappingSyncedWithBackend = activeMappingByLayer[actionTracingId]?.mapping;
+          if (!mappingSyncedWithBackend || segmentId1 == null || segmentId2 == null) {
             continue;
           }
-          const adaptToType =
-            upToDateMapping && isNumberMap(upToDateMapping)
-              ? (el: number) => el
-              : (el: number) => BigInt(el);
-          const upToDateAgglomerateId1 = (upToDateMapping as NumberLikeMap).get(
+
+          const adaptToType = getAdaptToTypeFunction(mappingSyncedWithBackend);
+          const updatedAgglomerateId1 = (mappingSyncedWithBackend as NumberLikeMap).get(
             adaptToType(segmentId1),
           );
-          const upToDateAgglomerateId2 = (upToDateMapping as NumberLikeMap).get(
+          const updatedAgglomerateId2 = (mappingSyncedWithBackend as NumberLikeMap).get(
             adaptToType(segmentId2),
           );
-          if (!upToDateAgglomerateId1) {
-            if (!(actionTracingId in idsToRequest)) {
-              idsToRequest[actionTracingId] = [];
+          if (!updatedAgglomerateId1) {
+            if (!(actionTracingId in idsToRequestByLayerId)) {
+              idsToRequestByLayerId[actionTracingId] = [];
             }
-            idsToRequest[actionTracingId].push(segmentId1);
+            idsToRequestByLayerId[actionTracingId].push(segmentId1);
           }
-          if (!upToDateAgglomerateId2) {
-            if (!(actionTracingId in idsToRequest)) {
-              idsToRequest[actionTracingId] = [];
+          if (!updatedAgglomerateId2) {
+            if (!(actionTracingId in idsToRequestByLayerId)) {
+              idsToRequestByLayerId[actionTracingId] = [];
             }
-            idsToRequest[actionTracingId].push(segmentId2);
+            idsToRequestByLayerId[actionTracingId].push(segmentId2);
           }
         }
       }
     }
   }
-  return idsToRequest;
+  return idsToRequestByLayerId;
 }
 
-function* addMissingSegmentsToLoadedMappings(idsToRequest: Record<string, number[]>): Saga<void> {
+// For each passed mapping reload the segment ids' mapping information and store it in the local mapping.
+// Needed after getAllUnknownSegmentIdsInPendingUpdates to load updated mapping info for segment ids of
+// mesh interaction proofreading actions to ensure reapplying these actions is done with up-to-date mapping info.
+function* addMissingSegmentsToLoadedMappings(idsToRequest: IdsToReloadPerMapping): Saga<void> {
   const annotationId = yield* select((state) => state.annotation.annotationId);
   const version = yield* select((state) => state.annotation.version);
   const tracingStoreUrl = yield* select((state) => state.annotation.tracingStore.url);
@@ -661,7 +668,7 @@ function* addMissingSegmentsToLoadedMappings(idsToRequest: Record<string, number
       continue;
     }
     const activeMapping = activeMappingByLayer[volumeTracingId];
-    // Ask the server to map the (split) segment ids. This creates a partial mapping
+    // Ask the server to map the segment ids needing reloading. This creates a partial mapping
     // that only contains these ids.
     const mappingWithMissingIds = yield* call(
       getAgglomeratesForSegmentsFromTracingstore,
@@ -684,6 +691,11 @@ function* addMissingSegmentsToLoadedMappings(idsToRequest: Record<string, number
   }
 }
 
+// Gathers info mapped info for segment ids from proofreading actions where the mapping is unknown.
+// This happens in case of mesh proofreading actions. To re-apply the user's changes in the rebasing
+// up-to-date mapping info is needed for all segments in all proofreading actions. Thus, the missing info
+// is first loaded and then the save queue update actions are remapped to update their agglomerate id infos
+// to apply them correctly during rebasing. Last the save queue is replaced with the updated save queue entries.
 function* updateSaveQueueEntriesToStateAfterRebase(): Saga<
   | {
       success: false;
@@ -710,8 +722,8 @@ function* updateSaveQueueEntriesToStateAfterRebase(): Saga<
           case "mergeAgglomerate":
           case "splitAgglomerate": {
             const { segmentId1, segmentId2, actionTracingId } = action.value;
-            const upToDateMapping = activeMappingByLayer[actionTracingId]?.mapping;
-            if (!upToDateMapping) {
+            const mappingSyncedWithBackend = activeMappingByLayer[actionTracingId]?.mapping;
+            if (!mappingSyncedWithBackend) {
               console.error(
                 "Found proofreading action without matching mapping in save queue. This should never happen.",
                 action,
@@ -728,14 +740,11 @@ function* updateSaveQueueEntriesToStateAfterRebase(): Saga<
               return null;
             }
 
-            const adaptToType =
-              upToDateMapping && isNumberMap(upToDateMapping)
-                ? (el: number) => el
-                : (el: number) => BigInt(el);
-            let upToDateAgglomerateId1 = (upToDateMapping as NumberLikeMap).get(
+            const adaptToType = getAdaptToTypeFunction(mappingSyncedWithBackend);
+            let upToDateAgglomerateId1 = (mappingSyncedWithBackend as NumberLikeMap).get(
               adaptToType(segmentId1),
             );
-            let upToDateAgglomerateId2 = (upToDateMapping as NumberLikeMap).get(
+            let upToDateAgglomerateId2 = (mappingSyncedWithBackend as NumberLikeMap).get(
               adaptToType(segmentId2),
             );
             if (!upToDateAgglomerateId1 || !upToDateAgglomerateId2) {

frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts

@@ -8,7 +8,7 @@ import {
 } from "admin/rest_api";
 import { V3 } from "libs/mjs";
 import Toast from "libs/toast";
-import { SoftError, isNumberMap } from "libs/utils";
+import { SoftError, getAdaptToTypeFunction, isNumberMap } from "libs/utils";
 import window from "libs/window";
 import _ from "lodash";
 import messages from "messages";
@@ -148,10 +148,6 @@ export default function* proofreadRootSaga(): Saga<void> {
   yield* takeEvery("TOGGLE_SEGMENT_IN_PARTITION", showToastIfSegmentOfOtherAgglomerateWasSelected);
 }
 
-function getAdaptToTypeFunction(mapping: Mapping | null | undefined) {
-  return mapping && isNumberMap(mapping) ? (el: number) => el : (el: number) => BigInt(el);
-}
-
 function* clearMinCutPartitionsOnMultiCutDeselect(
   action: UpdateUserSettingAction | EscapeAction,
 ): Saga<void> {
@@ -530,21 +526,6 @@ function* handleSkeletonProofreadingAction(action: Action): Saga<void> {
       targetAgglomerateId,
   );
 
-  // TODOM: just as below: check whether this is really needed
-  /*
-  if (action.type === "MERGE_TREES") {
-    console.log("Calling updateMappingWithMerge again after saving was done.");
-    // During saving, newer versions might have been pulled from the server.
-    yield* call(
-      updateMappingWithMerge,
-      volumeTracingId,
-      activeMapping,
-      sourceAgglomerateId,
-      targetAgglomerateId,
-    );
-  }
-    */
-
   if (action.type === "MIN_CUT_AGGLOMERATE_WITH_NODE_IDS" || action.type === "DELETE_EDGE") {
     if (sourceAgglomerateId !== targetAgglomerateId) {
       Toast.error(

frontend/javascripts/viewer/model/sagas/volume/update_actions.ts

@@ -933,7 +933,8 @@ export function splitAgglomerate(
     actionTracingId: string;
     segmentId1: number | undefined;
     segmentId2: number | undefined;
-    // Needed in live collab setting to notice changes of loaded agglomerates done by other users.
+    // agglomerateId is needed in live collab setting to notice changes of loaded agglomerates done by other users.
+    // Kept up-to-date in save queue by updateSaveQueueEntriesToStateAfterRebase saga.
     agglomerateId?: number | undefined;
     // For backwards compatibility reasons,
     // older segments are defined using their positions (and mag)
@@ -970,7 +971,8 @@ export function mergeAgglomerate(
     actionTracingId: string;
     segmentId1: number | undefined;
     segmentId2: number | undefined;
-    // Needed in live collab setting to notice changes of loaded agglomerates done by other users.
+    // agglomerateId1 and agglomerateId2 are needed in live collab setting to notice changes of loaded agglomerates done by other users.
+    // Kept up-to-date in save queue by updateSaveQueueEntriesToStateAfterRebase saga.
     agglomerateId1?: number;
     agglomerateId2?: number;
     // For backwards compatibility reasons,

frontend/javascripts/viewer/store.ts

@@ -625,14 +625,13 @@ export const combinedReducer = reduceReducers(
   VolumeTracingReducer,
   ProofreadingReducer,
   TaskReducer,
+  SaveReducer,
   FlycamReducer,
   FlycamInfoCacheReducer,
   ViewModeReducer,
   AnnotationReducer,
   UserReducer,
   UiReducer,
-  // SaveReducer needs to be behind Settings and Volumetracing reducer to react to changes SET MAPPING changes.
-  SaveReducer,
   ConnectomeReducer,
   OrganizationReducer,
 ) as Reducer;