Skip to content
Open
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
50 changes: 49 additions & 1 deletion src/app/modules/map/ol/lib/charts/chart-utils.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { expect, describe, it } from 'vitest';
import { extentFromBounds, resolveLayerMaxZoom } from './chart-utils';
import {
extentFromBounds,
isChartInView,
resolveLayerMaxZoom
} from './chart-utils';

describe('resolveLayerMaxZoom', () => {
it('returns chart max when over-zoom disabled', () => {
Expand Down Expand Up @@ -32,4 +36,48 @@ describe('extentFromBounds', () => {
it('returns undefined for invalid bounds values', () => {
expect(extentFromBounds([90, 90, 90, 300])).toBe(undefined);
});

it('returns a transformed extent for valid mid-range bounds', () => {
const extent = extentFromBounds([-10, -10, 10, 10]);
expect(extent).toBeDefined();
expect(extent).toHaveLength(4);
expect(extent?.every((n) => Number.isFinite(n))).toBe(true);
});

it('rejects bounds that touch the +/-180 / +/-90 edges', () => {
expect(extentFromBounds([-180, 0, 10, 10])).toBe(undefined);
expect(extentFromBounds([-10, -90, 10, 10])).toBe(undefined);
expect(extentFromBounds([-10, -10, 180, 10])).toBe(undefined);
expect(extentFromBounds([-10, -10, 10, 90])).toBe(undefined);
});

it('accepts bounds just inside the edges', () => {
expect(extentFromBounds([-179.99, -89.99, 179.99, 89.99])).toBeDefined();
});
});

describe('isChartInView', () => {
const extent = [10, 40, 20, 50];

it('keeps a chart whose bounds overlap the extent', () => {
expect(isChartInView([15, 45, 30, 60], extent)).toBe(true);
});

it('keeps a chart fully contained within the extent', () => {
expect(isChartInView([12, 42, 18, 48], extent)).toBe(true);
});

it('drops a chart whose bounds are disjoint from the extent', () => {
expect(isChartInView([30, 45, 40, 60], extent)).toBe(false);
});

it('keeps a chart that only touches the extent edge', () => {
expect(isChartInView([20, 40, 30, 50], extent)).toBe(true);
});

it('keeps charts with missing or malformed bounds (treated as global)', () => {
expect(isChartInView(undefined, extent)).toBe(true);
expect(isChartInView([10, 40, 20], extent)).toBe(true);
expect(isChartInView([], extent)).toBe(true);
});
});
30 changes: 29 additions & 1 deletion src/app/modules/map/ol/lib/charts/chart-utils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Extent } from 'ol/extent';
import { Extent, intersects } from 'ol/extent';
import { transformExtent } from 'ol/proj';

export function resolveLayerMaxZoom(
Expand Down Expand Up @@ -28,3 +28,31 @@ export function extentFromBounds(bounds?: number[]): Extent | undefined {
}
return transformExtent(bounds, 'EPSG:4326', 'EPSG:3857');
}

/**
* Determines whether a chart should remain visible when the chart list is
* filtered to the current map view.
*
* Charts without valid bounds metadata are treated as global (not tied to a
* region) and are always kept. Both `bounds` and `extent` are EPSG:4326
* [minLon, minLat, maxLon, maxLat], so they are compared directly without
* re-projection.
*
* Limitation: this is a plain min/max overlap test and does not account for a
* viewport that crosses the antimeridian (+/-180 longitude); near the dateline a
* chart may be wrongly included or excluded.
*
* Example:
* bounds=[10, 40, 20, 50], extent=[15, 45, 30, 60] -> true (overlap)
* bounds=[10, 40, 20, 50], extent=[30, 45, 40, 60] -> false (disjoint)
* bounds=undefined, extent=[...] -> true (global chart)
*/
export function isChartInView(
bounds: number[] | undefined,
extent: Extent
): boolean {
if (!Array.isArray(bounds) || bounds.length !== 4) {
return true;
}
return intersects(bounds, extent);
Comment on lines +41 to +57

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Handle wrapped view extents before intersecting.

This helper is documented to return wrong results when the viewport crosses the antimeridian, so the new In view filter will misclassify charts for dateline-adjacent views. Split wrapped extents into two longitude ranges (or normalize them first) before calling intersects, and add a regression test for that case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/modules/map/ol/lib/charts/chart-utils.ts` around lines 41 - 57,
isChartInView currently calls intersects directly, so wrapped viewport extents
crossing the antimeridian can be misclassified; update this helper to detect a
wrapped extent and split or normalize it into two longitude ranges before
testing overlap. Keep the existing global-chart fallback for undefined bounds,
and add a regression test around isChartInView covering a dateline-crossing view
to verify charts near +/-180 are classified correctly.

}
17 changes: 13 additions & 4 deletions src/app/modules/skresources/components/charts/chartlist.html
Original file line number Diff line number Diff line change
Expand Up @@ -92,20 +92,29 @@
matTooltip="Display Chart Boundaries"
matTooltipPosition="above"
>
Boundaries
Bounds
</mat-slide-toggle>
<mat-slide-toggle
style="margin-left: 12px"
[hideIcon]="true"
[checked]="inViewOnly"
(change)="toggleInViewOnly($event.checked)"
matTooltip="Show only charts visible in the current map view"
matTooltipPosition="above"
>
In view
</mat-slide-toggle>
<button
mat-button
mat-icon-button
(click)="showChartLayers(true)"
matTooltip="Re-order Charts"
matTooltipPosition="above"
>
<mat-icon>import_export</mat-icon>
Re-order
</button>

<button
mat-button
mat-icon-button
Comment on lines 107 to +117

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Restore an accessible name for the icon-only actions.

Changing these controls to icon-only buttons removes their explicit label for assistive tech. Add an aria-label (or visible text) for both actions; matTooltip is not enough here.

Suggested fix
       <button
         mat-icon-button
+        aria-label="Re-order Charts"
         (click)="showChartLayers(true)"
         matTooltip="Re-order Charts"
         matTooltipPosition="above"
       >
         <mat-icon>import_export</mat-icon>
       </button>

       <button
         mat-icon-button
+        aria-label="Add Chart"
         matTooltip="Add Chart"
         matTooltipPosition="above"
         [matMenuTriggerFor]="addchartmenu"
       >
         <mat-icon>add</mat-icon>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<button
mat-button
mat-icon-button
(click)="showChartLayers(true)"
matTooltip="Re-order Charts"
matTooltipPosition="above"
>
<mat-icon>import_export</mat-icon>
Re-order
</button>
<button
mat-button
mat-icon-button
<button
mat-icon-button
aria-label="Re-order Charts"
(click)="showChartLayers(true)"
matTooltip="Re-order Charts"
matTooltipPosition="above"
>
<mat-icon>import_export</mat-icon>
</button>
<button
mat-icon-button
aria-label="Add Chart"
matTooltip="Add Chart"
matTooltipPosition="above"
[matMenuTriggerFor]="addchartmenu"
>
<mat-icon>add</mat-icon>
</button>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/modules/skresources/components/charts/chartlist.html` around lines
107 - 117, The icon-only action buttons in chartlist.html no longer have an
explicit accessible name, so add an aria-label or visible text to both button
elements near showChartLayers and the adjacent action button. Keep the existing
matTooltip if desired, but ensure each control has a unique, descriptive label
for assistive tech that matches its action.

matTooltip="Add Chart"
matTooltipPosition="above"
[matMenuTriggerFor]="addchartmenu"
Expand Down
37 changes: 37 additions & 0 deletions src/app/modules/skresources/components/charts/chartlist.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
import { SKResourceGroupService } from '../groups/groups.service';
import { SKChart } from '../../resource-classes';
import { takeUntilDestroyed } from '@angular/core/rxjs-interop';
import { isChartInView } from 'src/app/modules/map/ol/lib/charts/chart-utils';

@Component({
selector: 'chart-list',
Expand Down Expand Up @@ -69,6 +70,7 @@ export class ChartListComponent extends ResourceListBase {
protected override filteredList = signal<FBCharts>([]);

displayChartLayers = false;
protected inViewOnly = false;

protected app = inject(AppFacade);
private worker = inject(SKWorkerService);
Expand All @@ -91,6 +93,13 @@ export class ChartListComponent extends ResourceListBase {
this.initItems(true);
}
});
// re-apply the filter as the map view changes while the in-view filter is on
effect(() => {
this.app.mapExtent();
if (this.inViewOnly) {
this.doFilter();
}
});
}

ngOnInit() {
Expand Down Expand Up @@ -283,6 +292,34 @@ export class ChartListComponent extends ResourceListBase {
}
}

/**
* @description Toggle filtering of the chart list to the current map view.
*/
protected toggleInViewOnly(checked: boolean) {
this.inViewOnly = checked;
this.doFilter();
}

/**
* @description Filter and sort charts by name, additionally restricting the
* list to charts visible in the current map view when the in-view filter is on.
*/
protected override doFilter() {
const text = this.filterText?.toLowerCase() ?? '';
const extent = this.app.mapExtent();
const useExtent =
this.inViewOnly && Array.isArray(extent) && extent.length === 4;
const fl = this.fullList.filter((item) => {
if (text && !item[1].name?.toLowerCase().includes(text)) {
return false;
}
return useExtent ? isChartInView(item[1].bounds, extent) : true;
});
fl.sort((a, b) => a[1].name.localeCompare(b[1].name));
this.filteredList.update(() => fl.slice(0));
this.alignSelections();
}

/**
* @description Add new chart source to resources
* */
Expand Down
Loading