Merge pull request #1701 from davidmundelius/sort-merged-signals

Fix sorting of merged signals in package panel

Author: benedikt-richter

CONTRIBUTORS.md

@@ -25,5 +25,6 @@ SPDX-License-Identifier: CC0-1.0
 - **[Ruiyun Xie](https://github.com/mayayunx)**
 - **[Sebastian Thomas](https://github.com/sebathomas)** (<sebastian.thomas@tngtech.com>)
 - **[Vasily Pozdnyakov](https://github.com/vasily-pozdnyakov)** (<vasily.pozdnyakov@tngtech.com>)
+- **[David Mundelius](https://github.com/davidmundelius)** (<david.mundelius@tngtech.com>)
 
 [Contributors list on GitHub](https://github.com/opossum-tool/OpossumUI/contributors).

src/Frontend/Components/AggregatedAttributionsPanel/__tests__/accordion-panel-helper.test.ts

@@ -16,6 +16,7 @@ import {
 import {
   getContainedManualDisplayPackageInfosWithCount,
   getExternalDisplayPackageInfosWithCount,
+  sortDisplayPackageInfosWithCountByCountAndPackageName,
 } from '../accordion-panel-helpers';
 import { PanelAttributionData } from '../../../util/get-contained-packages';
 import { PackagePanelTitle } from '../../../enums/enums';
@@ -195,6 +196,54 @@ describe('getExternalDisplayPackageInfosWithCount', () => {
       )
     ).toEqual([expectedPackageCardIds, expectedDisplayPackageInfosWithCount]);
   });
+
+  it('sorts ordinary and merged attributions according to the count', () => {
+    const testAttributionIdsWithCount: Array<AttributionIdWithCount> = [
+      { attributionId: 'uuidToMerge1', count: 3 },
+      { attributionId: 'uuidToMerge2', count: 2 },
+      { attributionId: 'uuidNotToMerge', count: 1 },
+    ];
+    const testAttributions: Attributions = {
+      uuidToMerge1: { packageName: 'Typescript' },
+      uuidToMerge2: { packageName: 'Typescript' },
+      uuidNotToMerge: { packageName: 'React' },
+    };
+    const testExternalAttributionsToHashes: AttributionsToHashes = {
+      uuidToMerge1: 'a',
+      uuidToMerge2: 'a',
+    };
+
+    const expectedPackageCardIds = [
+      `${testPackagePanelTitle}-1`,
+      `${testPackagePanelTitle}-0`,
+    ];
+
+    const expectedDisplayPackageInfosWithCount: DisplayPackageInfosWithCount = {
+      [expectedPackageCardIds[0]]: {
+        count: 5,
+        displayPackageInfo: {
+          attributionIds: ['uuidToMerge1', 'uuidToMerge2'],
+          packageName: 'Typescript',
+        },
+      },
+      [expectedPackageCardIds[1]]: {
+        count: 1,
+        displayPackageInfo: {
+          attributionIds: ['uuidNotToMerge'],
+          packageName: 'React',
+        },
+      },
+    };
+
+    expect(
+      getExternalDisplayPackageInfosWithCount(
+        testAttributionIdsWithCount,
+        testAttributions,
+        testExternalAttributionsToHashes,
+        testPackagePanelTitle
+      )
+    ).toEqual([expectedPackageCardIds, expectedDisplayPackageInfosWithCount]);
+  });
 });
 
 describe('getContainedManualDisplayPackageInfosWithCount', () => {
@@ -227,11 +276,13 @@ describe('getContainedManualDisplayPackageInfosWithCount', () => {
       resourcesToAttributions: testResourcesToAttributions,
       resourcesWithAttributedChildren: testResourcesWithAttributedChildren,
     };
+
     const expectedPackageCardIds = [
-      `${testPackagePanelTitle}-0`,
       `${testPackagePanelTitle}-1`,
       `${testPackagePanelTitle}-2`,
+      `${testPackagePanelTitle}-0`,
     ];
+
     const expectedDisplayPackageInfosWithCount: DisplayPackageInfosWithCount = {
       [expectedPackageCardIds[0]]: {
         displayPackageInfo: {
@@ -265,3 +316,57 @@ describe('getContainedManualDisplayPackageInfosWithCount', () => {
     ).toEqual([expectedPackageCardIds, expectedDisplayPackageInfosWithCount]);
   });
 });
+
+describe('sortDisplayPackageInfosWithCountByCountAndPackageName', () => {
+  it('sorts items correctly', () => {
+    const initialPackageCardIds: Array<string> = [
+      'pcid1',
+      'pcid2',
+      'pcid3',
+      'pcid4',
+      'pcid5',
+      'pcid6',
+    ];
+    const testDisplayPackageInfosWithCount: DisplayPackageInfosWithCount = {
+      pcid1: {
+        displayPackageInfo: { attributionIds: ['uuid1'] },
+        count: 10,
+      },
+      pcid2: {
+        displayPackageInfo: { attributionIds: ['uuid2'], packageName: 'c' },
+        count: 11,
+      },
+      pcid3: {
+        displayPackageInfo: { attributionIds: ['uuid3'], packageName: 'b' },
+        count: 10,
+      },
+      pcid4: {
+        displayPackageInfo: { attributionIds: ['uuid4'], packageName: 'e' },
+        count: 1,
+      },
+      pcid5: {
+        displayPackageInfo: { attributionIds: ['uuid5'], packageName: 'z' },
+        count: 10,
+      },
+      pcid6: {
+        displayPackageInfo: { attributionIds: ['uuid6'], packageName: 'd' },
+        count: 1,
+      },
+    };
+    const expectedPackageCardIds: Array<string> = [
+      'pcid2',
+      'pcid3',
+      'pcid5',
+      'pcid1',
+      'pcid6',
+      'pcid4',
+    ];
+
+    const result = initialPackageCardIds.sort(
+      sortDisplayPackageInfosWithCountByCountAndPackageName(
+        testDisplayPackageInfosWithCount
+      )
+    );
+    expect(result).toEqual(expectedPackageCardIds);
+  });
+});

src/Frontend/Components/AggregatedAttributionsPanel/accordion-panel-helpers.ts

@@ -6,6 +6,7 @@
 import {
   Attributions,
   AttributionsToHashes,
+  DisplayPackageInfo,
   PackageInfo,
 } from '../../../shared/shared-types';
 import { PackagePanelTitle } from '../../enums/enums';
@@ -68,6 +69,12 @@ export function getContainedManualDisplayPackageInfosWithCount(args: {
     }
   );
 
+  packageCardIds.sort(
+    sortDisplayPackageInfosWithCountByCountAndPackageName(
+      displayPackageInfosWithCount
+    )
+  );
+
   return [packageCardIds, displayPackageInfosWithCount];
 }
 
@@ -112,6 +119,12 @@ export function getExternalDisplayPackageInfosWithCount(
     indexCounter
   );
 
+  packageCardIds.sort(
+    sortDisplayPackageInfosWithCountByCountAndPackageName(
+      displayPackageInfosWithCount
+    )
+  );
+
   return [packageCardIds, displayPackageInfosWithCount];
 }
 
@@ -132,3 +145,35 @@ function addMergedSignals(
     indexCounter++;
   });
 }
+
+//exported for testing
+export function sortDisplayPackageInfosWithCountByCountAndPackageName(
+  displayPackageInfosWithCount: DisplayPackageInfosWithCount
+) {
+  return function (id1: string, id2: string): number {
+    if (
+      displayPackageInfosWithCount[id1].count !==
+      displayPackageInfosWithCount[id2].count
+    ) {
+      return (
+        displayPackageInfosWithCount[id2].count -
+        displayPackageInfosWithCount[id1].count
+      );
+    }
+
+    const p1: DisplayPackageInfo =
+      displayPackageInfosWithCount[id1].displayPackageInfo;
+    const p2: DisplayPackageInfo =
+      displayPackageInfosWithCount[id2].displayPackageInfo;
+    if (p1?.packageName && p2?.packageName) {
+      return p1.packageName.toLowerCase() < p2.packageName.toLowerCase()
+        ? -1
+        : 1;
+    } else if (p1?.packageName) {
+      return -1;
+    } else if (p2?.packageName) {
+      return 1;
+    }
+    return 0;
+  };
+}

src/Frontend/util/__tests__/get-contained-packages.test.ts

@@ -7,10 +7,7 @@ import {
   Attributions,
   ResourcesToAttributions,
 } from '../../../shared/shared-types';
-import {
-  computeAggregatedAttributionsFromChildren,
-  sortByCountAndPackageName,
-} from '../get-contained-packages';
+import { computeAggregatedAttributionsFromChildren } from '../get-contained-packages';
 import { AttributionIdWithCount } from '../../types/types';
 
 describe('computeAggregatedAttributionsFromChildren', () => {
@@ -29,16 +26,16 @@ describe('computeAggregatedAttributionsFromChildren', () => {
     .add('samplepath/subfolder')
     .add('samplepath2/subfolder/subsubfolder');
 
-  it('selects aggregated children and sorts correctly', () => {
+  it('selects aggregated children', () => {
     const expectedResult: Array<AttributionIdWithCount> = [
-      {
-        count: 2,
-        attributionId: 'uuid_2',
-      },
       {
         count: 1,
         attributionId: 'uuid_1',
       },
+      {
+        count: 2,
+        attributionId: 'uuid_2',
+      },
       {
         count: 1,
         attributionId: 'uuid_3',
@@ -79,83 +76,3 @@ describe('computeAggregatedAttributionsFromChildren', () => {
     expect(result).toEqual(expectedResult);
   });
 });
-
-describe('sortByCountAndPackageName', () => {
-  it('sorts items correctly', () => {
-    const initialAttributionIdsWithCount: Array<AttributionIdWithCount> = [
-      {
-        attributionId: 'uuid1',
-        count: 10,
-      },
-      {
-        attributionId: 'uuid2',
-        count: 11,
-      },
-      {
-        attributionId: 'uuid3',
-        count: 10,
-      },
-      {
-        attributionId: 'uuid4',
-        count: 1,
-      },
-      {
-        attributionId: 'uuid5',
-        count: 10,
-      },
-      {
-        attributionId: 'uuid6',
-        count: 1,
-      },
-    ];
-    const testAttributions: Attributions = {
-      uuid1: {},
-      uuid2: {
-        packageName: 'c',
-      },
-      uuid3: {
-        packageName: 'b',
-      },
-      uuid4: {
-        packageName: 'e',
-      },
-      uuid5: {
-        packageName: 'z',
-      },
-      uuid6: {
-        packageName: 'd',
-      },
-    };
-    const expectedAttributionIdsWithCount: Array<AttributionIdWithCount> = [
-      {
-        attributionId: 'uuid2',
-        count: 11,
-      },
-      {
-        attributionId: 'uuid3',
-        count: 10,
-      },
-      {
-        attributionId: 'uuid5',
-        count: 10,
-      },
-      {
-        attributionId: 'uuid1',
-        count: 10,
-      },
-      {
-        attributionId: 'uuid6',
-        count: 1,
-      },
-      {
-        attributionId: 'uuid4',
-        count: 1,
-      },
-    ];
-
-    const result = initialAttributionIdsWithCount.sort(
-      sortByCountAndPackageName(testAttributions)
-    );
-    expect(result).toEqual(expectedAttributionIdsWithCount);
-  });
-});

src/Frontend/util/get-contained-packages.ts

@@ -5,7 +5,6 @@
 import {
   AttributionData,
   Attributions,
-  PackageInfo,
   ResourcesToAttributions,
   ResourcesWithAttributedChildren,
 } from '../../shared/shared-types';
@@ -81,36 +80,8 @@ export function computeAggregatedAttributionsFromChildren(
     });
   });
 
-  return Object.keys(attributionCount)
-    .map((attributionId: string) => ({
-      attributionId,
-      count: attributionCount[attributionId],
-    }))
-    .sort(sortByCountAndPackageName(attributions));
-}
-
-export function sortByCountAndPackageName(
-  attributions: Readonly<Attributions>
-) {
-  return function (
-    a1: AttributionIdWithCount,
-    a2: AttributionIdWithCount
-  ): number {
-    if (a1.count && a2.count && a1.count !== a2.count) {
-      return a2.count - a1.count;
-    }
-
-    const p1: PackageInfo = attributions[a1.attributionId];
-    const p2: PackageInfo = attributions[a2.attributionId];
-    if (p1?.packageName && p2?.packageName) {
-      return p1.packageName.toLowerCase() < p2.packageName.toLowerCase()
-        ? -1
-        : 1;
-    } else if (p1?.packageName) {
-      return -1;
-    } else if (p2?.packageName) {
-      return 1;
-    }
-    return 0;
-  };
+  return Object.keys(attributionCount).map((attributionId: string) => ({
+    attributionId,
+    count: attributionCount[attributionId],
+  }));
 }