Save sort order only on change (#2517)

Always base list sorting on id to ensure sorting stability

client/src/app/site/base/base-sort.service/base-sort-list.service.ts

@@ -1,6 +1,14 @@
 import { Directive } from '@angular/core';
 import { TranslateService } from '@ngx-translate/core';
-import { BehaviorSubject, Observable, Subscription } from 'rxjs';
+import {
+    BehaviorSubject,
+    distinctUntilChanged,
+    filter,
+    firstValueFrom,
+    isObservable,
+    Observable,
+    Subscription
+} from 'rxjs';
 import { SortListService } from 'src/app/ui/modules/list/definitions/sort-service';
 
 import { StorageService } from '../../../gateways/storage.service';
@@ -66,6 +74,15 @@ export abstract class BaseSortListService<V extends BaseViewModel>
         return this.sortDefinition!.sortAscending;
     }
 
+    public get hasSortOptionSelected(): boolean {
+        const defaultDef = this._defaultDefinitionSubject.value;
+        const current = this.sortDefinition;
+        if (!defaultDef || !current) {
+            return false;
+        }
+        return defaultDef.sortAscending !== current.sortAscending || defaultDef.sortProperty !== current.sortProperty;
+    }
+
     /**
      * set the property of the viewModel the sorting will be based on.
      * If the property stays the same, only the sort direction will be toggled,
@@ -87,7 +104,7 @@ export abstract class BaseSortListService<V extends BaseViewModel>
      * @returns the current sorting property
      */
     public get sortProperty(): OsSortProperty<V> {
-        return this.sortDefinition!.sortProperty;
+        return this.sortDefinition?.sortProperty;
     }
 
     /**
@@ -105,8 +122,46 @@ export abstract class BaseSortListService<V extends BaseViewModel>
         return [];
     }
 
-    public constructor(translate: TranslateService, private store: StorageService) {
+    public get defaultOption(): OsSortingOption<V> | undefined {
+        return this.getSortOptions().find(
+            option =>
+                this._defaultDefinitionSubject.value &&
+                this.isSameProperty(option.property, this._defaultDefinitionSubject.value.sortProperty)
+        );
+    }
+
+    public isSameProperty(a: OsSortProperty<V>, b: OsSortProperty<V>): boolean {
+        a = Array.isArray(a) ? a : [a];
+        b = Array.isArray(b) ? b : [b];
+        return a.equals(b);
+    }
+
+    private _defaultDefinitionSubject = new BehaviorSubject<OsSortingDefinition<V>>(null);
+
+    private _isDefaultSorting = false;
+
+    public constructor(
+        translate: TranslateService,
+        private store: StorageService,
+        defaultDefinition: OsSortingDefinition<V> | Observable<OsSortingDefinition<V>>
+    ) {
         super(translate);
+
+        this._defaultDefinitionSubject
+            .pipe(distinctUntilChanged((prev, curr) => prev?.sortProperty === curr?.sortProperty))
+            .subscribe(defaultDef => {
+                if (this._isDefaultSorting && defaultDef) {
+                    this.setSorting(defaultDef.sortProperty, defaultDef.sortAscending);
+                } else if (defaultDef && this.sortDefinition?.sortProperty === defaultDef?.sortProperty) {
+                    this.updateSortDefinitions();
+                }
+            });
+
+        if (isObservable(defaultDefinition)) {
+            defaultDefinition.subscribe(this._defaultDefinitionSubject);
+        } else {
+            this._defaultDefinitionSubject.next(defaultDefinition);
+        }
     }
 
     /**
@@ -136,10 +191,12 @@ export abstract class BaseSortListService<V extends BaseViewModel>
         return shouldHide;
     }
 
-    /**
-     * Enforce children to implement a method that returns the fault sorting
-     */
-    protected abstract getDefaultDefinition(): OsSortingDefinition<V> | Promise<OsSortingDefinition<V>>;
+    protected async getDefaultDefinition(): Promise<OsSortingDefinition<V>> {
+        if (this._defaultDefinitionSubject.value) {
+            return this._defaultDefinitionSubject.value;
+        }
+        return await firstValueFrom(this._defaultDefinitionSubject.pipe(filter(value => !!value)));
+    }
 
     /**
      * Defines the sorting properties, and returns an observable with sorted data
@@ -151,16 +208,45 @@ export abstract class BaseSortListService<V extends BaseViewModel>
         this.exitSortService();
 
         if (!this.sortDefinition) {
-            const storedDefinition = await this.store.get<OsSortingDefinition<V>>(`sorting_` + this.storageKey);
+            let [storedDefinition, sortProperty, sortAscending]: [OsSortingDefinition<V>, OsSortProperty<V>, boolean] =
+                await Promise.all([
+                    // TODO: Remove the sorting definition loading part and everything caused by 'transformDeprecated' at a later date, it is only here for backwards compatibility
+                    this.store.get<OsSortingDefinition<V>>(`sorting_` + this.storageKey),
+                    this.store.get<OsSortProperty<V>>(`sorting_property_` + this.storageKey),
+                    this.store.get<boolean>(`sorting_ascending_` + this.storageKey)
+                ]);
+
+            let transformDeprecated = !!storedDefinition;
+            if (transformDeprecated) {
+                this.store.remove(`sorting_` + this.storageKey);
+            }
+
+            if ((sortAscending ?? sortProperty) != null) {
+                storedDefinition = {
+                    sortAscending,
+                    sortProperty
+                };
+            }
 
             if (storedDefinition) {
                 this.sortDefinition = storedDefinition;
             }
 
             if (this.sortDefinition && this.sortDefinition.sortProperty) {
-                this.updateSortedData();
+                if (transformDeprecated) {
+                    this.updateSortDefinitions();
+                } else {
+                    this.calculateDefaultStatus();
+                    this.updateSortedData();
+                }
             } else {
-                this.sortDefinition = await this.getDefaultDefinition();
+                let defaultDef = await this.getDefaultDefinition();
+                sortAscending = sortAscending ?? defaultDef.sortAscending;
+                sortProperty = sortProperty ?? defaultDef.sortProperty;
+                this.sortDefinition = {
+                    sortAscending,
+                    sortProperty
+                };
                 this.updateSortDefinitions();
             }
         }
@@ -184,10 +270,14 @@ export abstract class BaseSortListService<V extends BaseViewModel>
      * @param property a sorting property of a view model
      * @param ascending ascending or descending
      */
-    public setSorting(property: keyof V, ascending: boolean): void {
-        this.sortDefinition!.sortProperty = property;
-        this.sortDefinition!.sortAscending = ascending;
-        this.updateSortDefinitions();
+    public setSorting(property: OsSortProperty<V>, ascending: boolean): void {
+        if (!this.sortDefinition) {
+            this.sortDefinition = { sortProperty: property, sortAscending: ascending };
+        } else {
+            this.sortDefinition!.sortProperty = property;
+            this.sortDefinition!.sortAscending = ascending;
+            this.updateSortDefinitions();
+        }
     }
 
     /**
@@ -229,32 +319,47 @@ export abstract class BaseSortListService<V extends BaseViewModel>
         return itemProperty.charAt(0).toUpperCase() + itemProperty.slice(1);
     }
 
-    /**
-     * Saves the current sorting definitions to the local store
-     */
-    private updateSortDefinitions(): void {
-        this.updateSortedData();
-        this.store.set(`sorting_` + this.storageKey, this.sortDefinition);
-    }
-
     /**
      * Recreates the sorting function. Is supposed to be called on init and
      * every time the sorting (property, ascending/descending) or the language changes
      */
     protected async updateSortedData(): Promise<void> {
         const alternativeProperty = (await this.getDefaultDefinition()).sortProperty;
         if (this.inputData) {
-            this.inputData.sort((itemA, itemB) =>
-                this.sortItems(
-                    itemA,
-                    itemB,
-                    this.shouldHideOption({ property: this.sortProperty }, false)
-                        ? alternativeProperty
-                        : this.sortProperty,
-                    this.ascending
+            this.outputSubject.next(
+                [...this.inputData].sort(
+                    (itemA, itemB) =>
+                        this.sortItems(
+                            itemA,
+                            itemB,
+                            this.shouldHideOption({ property: this.sortProperty }, false)
+                                ? alternativeProperty
+                                : this.sortProperty,
+                            this.ascending
+                        ) || itemA.id - itemB.id
                 )
             );
-            this.outputSubject.next(this.inputData);
         }
     }
+
+    /**
+     * Saves the current sorting definitions to the local store
+     */
+    private updateSortDefinitions(): void {
+        this.calculateDefaultStatus();
+        this.updateSortedData();
+        if (this._isDefaultSorting) {
+            this.store.remove(`sorting_property_` + this.storageKey);
+        } else {
+            this.store.set(`sorting_property_` + this.storageKey, this.sortDefinition?.sortProperty);
+        }
+        this.store.set(`sorting_ascending_` + this.storageKey, this.sortDefinition?.sortAscending);
+    }
+
+    private calculateDefaultStatus(): void {
+        this._isDefaultSorting = this.isSameProperty(
+            this.sortDefinition.sortProperty,
+            this._defaultDefinitionSubject.value?.sortProperty
+        );
+    }
 }

client/src/app/site/pages/meetings/pages/assignments/pages/assignment-list/services/assignment-sort-list.service.ts

@@ -1,5 +1,7 @@
 import { Injectable } from '@angular/core';
-import { BaseSortListService, OsSortingDefinition, OsSortingOption } from 'src/app/site/base/base-sort.service';
+import { TranslateService } from '@ngx-translate/core';
+import { StorageService } from 'src/app/gateways/storage.service';
+import { BaseSortListService, OsSortingOption } from 'src/app/site/base/base-sort.service';
 import { ViewAssignment } from 'src/app/site/pages/meetings/pages/assignments';
 
 import { AssignmentListServiceModule } from './assignment-list-service.module';
@@ -26,22 +28,17 @@ export class AssignmentSortListService extends BaseSortListService<ViewAssignmen
         { property: `id`, label: `Creation date` }
     ];
 
+    constructor(translate: TranslateService, store: StorageService) {
+        super(translate, store, {
+            sortProperty: `title`,
+            sortAscending: true
+        });
+    }
+
     /**
      * @override
      */
     protected getSortOptions(): OsSortingOption<ViewAssignment>[] {
         return this.assignmentSortOptions;
     }
-
-    /**
-     * Required by parent
-     *
-     * @returns the default sorting strategy
-     */
-    public async getDefaultDefinition(): Promise<OsSortingDefinition<ViewAssignment>> {
-        return {
-            sortProperty: `title`,
-            sortAscending: true
-        };
-    }
 }

client/src/app/site/pages/meetings/pages/mediafiles/modules/mediafile-list/services/mediafile-list-sort.service/mediafile-list-sort.service.ts

@@ -1,7 +1,7 @@
 import { Injectable } from '@angular/core';
 import { TranslateService } from '@ngx-translate/core';
 import { StorageService } from 'src/app/gateways/storage.service';
-import { BaseSortListService, OsSortingDefinition, OsSortingOption } from 'src/app/site/base/base-sort.service';
+import { BaseSortListService, OsSortingOption } from 'src/app/site/base/base-sort.service';
 
 import { ViewMediafile } from '../../../../view-models';
 import { MediafileListServiceModule } from '../mediafile-list-service.module';
@@ -26,7 +26,10 @@ export class MediafileListSortService extends BaseSortListService<ViewMediafile>
     ];
 
     public constructor(translate: TranslateService, store: StorageService) {
-        super(translate, store);
+        super(translate, store, {
+            sortProperty: `title`,
+            sortAscending: true
+        });
     }
 
     /**
@@ -35,16 +38,4 @@ export class MediafileListSortService extends BaseSortListService<ViewMediafile>
     protected getSortOptions(): OsSortingOption<ViewMediafile>[] {
         return this.mediafilesSortOptions;
     }
-
-    /**
-     * Required by parent
-     *
-     * @returns the default sorting strategy
-     */
-    public async getDefaultDefinition(): Promise<OsSortingDefinition<ViewMediafile>> {
-        return {
-            sortProperty: `title`,
-            sortAscending: true
-        };
-    }
 }

client/src/app/site/pages/meetings/pages/motions/pages/motion-blocks/services/motion-block-sort.service/motion-block-sort.service.ts

@@ -1,7 +1,7 @@
 import { Injectable } from '@angular/core';
 import { TranslateService } from '@ngx-translate/core';
 import { StorageService } from 'src/app/gateways/storage.service';
-import { BaseSortListService, OsSortingDefinition, OsSortingOption } from 'src/app/site/base/base-sort.service';
+import { BaseSortListService, OsSortingOption } from 'src/app/site/base/base-sort.service';
 import { ViewMotionBlock } from 'src/app/site/pages/meetings/pages/motions';
 
 import { MotionBlockServiceModule } from '../motion-block-service.module';
@@ -28,7 +28,10 @@ export class MotionBlockSortService extends BaseSortListService<ViewMotionBlock>
     ];
 
     public constructor(translate: TranslateService, store: StorageService) {
-        super(translate, store);
+        super(translate, store, {
+            sortProperty: `title`,
+            sortAscending: true
+        });
     }
 
     /**
@@ -37,11 +40,4 @@ export class MotionBlockSortService extends BaseSortListService<ViewMotionBlock>
     protected getSortOptions(): OsSortingOption<ViewMotionBlock>[] {
         return this.MotionBlockSortOptions;
     }
-
-    protected async getDefaultDefinition(): Promise<OsSortingDefinition<ViewMotionBlock>> {
-        return {
-            sortProperty: `title`,
-            sortAscending: true
-        };
-    }
 }

client/src/app/site/pages/meetings/pages/motions/services/list/amendment-list-sort.service/amendment-list-sort.service.ts

@@ -1,6 +1,9 @@
 import { Injectable } from '@angular/core';
-import { OsSortingDefinition, OsSortingOption } from 'src/app/site/base/base-sort.service';
+import { TranslateService } from '@ngx-translate/core';
+import { StorageService } from 'src/app/gateways/storage.service';
+import { OsSortingOption } from 'src/app/site/base/base-sort.service';
 import { ViewMotion } from 'src/app/site/pages/meetings/pages/motions';
+import { MeetingSettingsService } from 'src/app/site/pages/meetings/services/meeting-settings.service';
 
 import { MotionListSortService } from '../motion-list-sort.service';
 import { MotionsListServiceModule } from '../motions-list-service.module';
@@ -21,14 +24,14 @@ export class AmendmentListSortService extends MotionListSortService {
         }
     ];
 
-    protected override getSortOptions(): OsSortingOption<ViewMotion>[] {
-        return this.amendmentSortOptions.concat(super.getSortOptions());
+    constructor(translate: TranslateService, store: StorageService, meetingSettingsService: MeetingSettingsService) {
+        super(translate, store, meetingSettingsService, {
+            sortProperty: `title`,
+            sortAscending: true
+        });
     }
 
-    protected override async getDefaultDefinition(): Promise<OsSortingDefinition<ViewMotion>> {
-        return {
-            sortProperty: `parentAndLineNumber`,
-            sortAscending: true
-        };
+    protected override getSortOptions(): OsSortingOption<ViewMotion>[] {
+        return this.amendmentSortOptions.concat(super.getSortOptions());
     }
 }