Copilot commented on code in PR #4262:
URL: https://github.com/apache/streampipes/pull/4262#discussion_r2939904562
##########
ui/src/app/editor/components/pipeline/pipeline.component.ts:
##########
@@ -158,6 +162,16 @@ export class PipelineComponent implements OnInit,
OnDestroy {
this.readonly,
);
if (!this.readonly) {
+ this.shortcutReg = this.shortcutService.register('pipeline', [
+ {
+ key: 'Delete',
+ action: () => this.deleteSelectedElement(),
+ },
+ {
+ key: 'Backspace',
+ action: () => this.deleteSelectedElement(),
+ },
+ ]);
Review Comment:
Delete/Backspace are always intercepted when this component is active
(service prevents default by default), even when no element is hovered/eligible
for deletion. This can unintentionally block browser Backspace navigation
without performing any delete. Consider setting `preventDefault: false` and
only calling `event.preventDefault()` when `deleteSelectedElement()` actually
deletes something (or when `currentMouseOverElement` is set).
##########
ui/src/app/chart-shared/components/charts/table/table-widget.component.html:
##########
@@ -56,22 +57,50 @@
isHighlightedColumn(column),
}"
>
- <button
- type="button"
- class="sort-trigger"
- (click)="sortBy(column)"
- >
- <span>
- @if (column === 'time') {
- {{ 'Time' | translate }}
- } @else {
- {{ headerLabel(column) }}
- }
- </span>
- <mat-icon class="sort-icon">
- {{ sortIcon(column) }}
- </mat-icon>
- </button>
+ <div class="th-inner">
+ <button
+ type="button"
+ class="sort-trigger"
+ (click)="sortBy(column)"
+ >
+ <span>
+ @if (column === 'time') {
+ {{ 'Time' | translate }}
+ } @else {
+ {{ headerLabel(column) }}
+ }
+ </span>
+ <mat-icon class="sort-icon">
+ {{ sortIcon(column) }}
+ </mat-icon>
+ </button>
+ <button
+ type="button"
+ class="column-filter-trigger"
+ [attr.data-cy]="
+ 'column-filter-trigger-' +
+ column
+ "
+ [ngClass]="{
+ 'filter-active':
+ hasActiveFilter(column),
+ 'filter-open':
+ isColumnFilterOpen(column),
+ }"
+ (click)="
+ toggleColumnFilter(
+ column,
+ $event
+ )
+ "
+ >
+ <mat-icon
+ class="column-filter-icon"
+ >
+ filter_list
+ </mat-icon>
+ </button>
Review Comment:
The column filter icon button has no accessible name (no `aria-label`,
`title`, or visible text). Add an `aria-label` (and optionally a tooltip) so
screen readers can announce what this control does.
##########
ui/src/app/chart-shared/components/charts/table/table-widget.component.ts:
##########
@@ -43,6 +45,43 @@ interface NumericColumnStats {
max: number;
}
+interface AdvancedFilter {
+ type: string;
+ value: string;
+ value2?: string;
+}
+
+const BLANKS_LABEL = '(Blanks)';
+const DROPDOWN_MAX_WIDTH = 300;
+const DROPDOWN_EDGE_PADDING = 3;
+
+const NO_INPUT_TYPES = new Set(['Top 10', 'Above average', 'Below average']);
+
+const NUMERIC_FILTER_OPTIONS = [
+ 'Equals',
+ 'Does not equal',
+ 'Greater than',
+ 'Greater than or equal to',
+ 'Less than',
+ 'Less than or equal to',
+ 'Between',
+ 'Top 10',
+ 'Above average',
+ 'Below average',
+];
+
+const TEXT_FILTER_OPTIONS = [
+ 'Equals',
+ 'Does not equal',
+ 'Begins with',
+ 'Ends with',
+ 'Contains',
+ 'Does not contain',
+];
+
+const TIMESTAMP_FILTER_OPTIONS = ['Before', 'After', 'Between'];
+const TIMESTAMP_MASK = 'yyyy-mm-dd HH:mm:ss.SSS';
Review Comment:
`TIMESTAMP_MASK` uses `yyyy-mm-dd ...` but elsewhere (e.g. the table cell
date pipe) the month token is `MM`. Using `mm` here is inconsistent and
commonly reads as “minutes”, which will confuse users and can break the mask
logic expectations. Consider changing this to `yyyy-MM-dd HH:mm:ss.SSS`.
##########
ui/src/app/chart/components/chart-view/chart-view.component.ts:
##########
@@ -145,6 +149,10 @@ export class ChartViewComponent
@ViewChild('panel', { static: false }) outerPanel: ElementRef;
ngOnInit() {
+ this.shortcutReg = this.shortcutService.register('chart-view', [
+ { key: 's', ctrl: true, action: () => this.onShortcutSave() },
Review Comment:
The shortcut is registered unconditionally, and `KeyboardShortcutService`
prevents the browser default even when `editMode` is false (so Ctrl/Cmd+S does
nothing but still blocks the browser Save shortcut). Consider setting
`preventDefault: false` for this action and calling `event.preventDefault()`
only when `editMode` is true, or register/unregister the shortcut when
entering/leaving edit mode.
##########
ui/projects/streampipes/shared-ui/src/lib/services/keyboard-shortcut.service.ts:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+import { Injectable, NgZone, OnDestroy } from '@angular/core';
+import { Subject } from 'rxjs';
+import { filter } from 'rxjs/operators';
+
+export interface ShortcutAction {
+ key: string;
+ ctrl?: boolean;
+ shift?: boolean;
+ action: (event: KeyboardEvent) => void;
+ preventDefault?: boolean;
+}
+
+export type ShortcutRegistration = { unregister: () => void };
+
+@Injectable({ providedIn: 'root' })
+export class KeyboardShortcutService implements OnDestroy {
+ private keydown$ = new Subject<KeyboardEvent>();
+ private registrations: Map<string, ShortcutAction[]> = new Map();
+ private listener = (e: KeyboardEvent) => this.keydown$.next(e);
+
+ constructor(private ngZone: NgZone) {
+ this.ngZone.runOutsideAngular(() =>
+ document.addEventListener('keydown', this.listener, true),
+ );
+ this.keydown$
+ .pipe(filter(e => !this.isInputFocused(e)))
+ .subscribe(e => this.ngZone.run(() => this.handleEvent(e)));
+ }
+
+ ngOnDestroy(): void {
+ document.removeEventListener('keydown', this.listener, true);
+ this.keydown$.complete();
+ }
+
+ register(id: string, actions: ShortcutAction[]): ShortcutRegistration {
+ this.registrations.set(id, actions);
+ return { unregister: () => this.registrations.delete(id) };
+ }
+
+ unregister(id: string): void {
+ this.registrations.delete(id);
+ }
+
+ private handleEvent(event: KeyboardEvent): void {
+ const key = event.key.toLowerCase();
+ const ctrl = event.ctrlKey || event.metaKey;
+ const shift = event.shiftKey;
+
+ for (const actions of this.registrations.values()) {
+ const match = actions.find(
+ a =>
+ a.key.toLowerCase() === key &&
+ !!a.ctrl === ctrl &&
+ !!a.shift === shift,
+ );
+ if (match) {
+ if (match.preventDefault !== false) {
+ event.preventDefault();
+ }
+ match.action(event);
+ return;
Review Comment:
`handleEvent` calls `event.preventDefault()` as soon as a key combo matches,
even if the action ends up being a no-op due to component-level guards. This
makes it easy to unintentionally block browser defaults (e.g. Ctrl/Cmd+S)
outside the intended scope. Consider changing the API so actions can indicate
whether they handled the event (e.g., return boolean) and only preventDefault
when handled, or default `preventDefault` to false and require callers to opt
in explicitly.
##########
ui/src/app/chart-shared/components/charts/table/table-widget.component.html:
##########
@@ -154,4 +183,315 @@
</mat-paginator>
</div>
}
+ @if (openFilterColumn) {
+ <div
+ class="column-filter-dropdown"
+ [ngStyle]="dropdownStyle"
+ (click)="onFilterDropdownClick($event)"
+ >
+ @if (hasActiveFilter(openFilterColumn)) {
+ <button
+ type="button"
+ class="clear-filter-btn"
+ (click)="clearColumnFilter(openFilterColumn)"
+ >
+ <mat-icon class="clear-filter-icon"
+ >filter_list_off</mat-icon
+ >
+ Clear Filter
+ </button>
+ }
+ <button
+ type="button"
+ class="advanced-filter-btn"
+ data-cy="column-advanced-filter-expand-btn"
+ (click)="toggleAdvancedPanel(openFilterColumn, $event)"
+ >
+ {{ getAdvancedFilterLabel(openFilterColumn) }}
+ <mat-icon class="advanced-arrow-icon">{{
+ showAdvancedPanel === openFilterColumn
+ ? 'expand_more'
+ : 'chevron_right'
+ }}</mat-icon>
+ </button>
+ @if (showAdvancedPanel === openFilterColumn) {
+ <div class="advanced-filter-panel">
+ <div class="advanced-filter-options">
+ @for (
+ opt of getAdvancedFilterOptions(openFilterColumn);
+ track opt
+ ) {
+ <button
+ type="button"
+ class="advanced-option-btn"
+ [ngClass]="{
+ 'advanced-option-selected':
+ selectedAdvancedType === opt,
+ }"
+ (click)="selectAdvancedType(opt)"
+ >
+ {{ opt }}
+ </button>
+ }
+ </div>
+ @if (selectedAdvancedType) {
+ <div class="advanced-filter-inputs">
+ @if (needsInput(selectedAdvancedType)) {
+ <div
+ [class.ts-mask-wrapper]="
+ openFilterColumn === 'time'
+ "
+ >
+ @if (openFilterColumn === 'time') {
+ <div
+ class="ts-overlay"
+ aria-hidden="true"
+ >
+ <span>{{
+ getTimestampTyped(
+ advancedInputValue
+ )
+ }}</span
+ ><span class="ts-faded">{{
+ getTimestampTemplate(
+ advancedInputValue
+ )
+ }}</span>
+ </div>
+ }
+ <input
+ type="text"
+ [class]="
+ openFilterColumn === 'time'
+ ? 'advanced-input ts-input'
+ : 'advanced-input'
+ "
+ title="Filter value"
+ [placeholder]="
+ openFilterColumn === 'time'
+ ? ''
+ : 'Value...'
+ "
+ [ngModel]="advancedInputValue"
+ (input)="
+ openFilterColumn === 'time'
+ ? onTimestampInput(
+ 'value',
+ $event
+ )
+ : null
+ "
+ (ngModelChange)="
+ openFilterColumn !== 'time'
+ ? (advancedInputValue = $event)
+ : null
+ "
+ (mousedown)="
+ openFilterColumn === 'time'
+ ? repositionToEnd($event)
+ : null
+ "
+ (click)="$event.stopPropagation()"
+ (keydown)="
+ onAdvancedInputKeydown(
+ $event,
+ openFilterColumn,
+ 1
+ )
+ "
+ />
+ </div>
+ }
+ @if (needsSecondInput(selectedAdvancedType)) {
+ <span class="advanced-and-label">and</span>
+ <div
+ [class.ts-mask-wrapper]="
+ openFilterColumn === 'time'
+ "
+ >
+ @if (openFilterColumn === 'time') {
+ <div
+ class="ts-overlay"
+ aria-hidden="true"
+ >
+ <span>{{
+ getTimestampTyped(
+ advancedInputValue2
+ )
+ }}</span
+ ><span class="ts-faded">{{
+ getTimestampTemplate(
+ advancedInputValue2
+ )
+ }}</span>
+ </div>
+ }
+ <input
+ type="text"
+ [class]="
+ openFilterColumn === 'time'
+ ? 'advanced-input ts-input'
+ : 'advanced-input'
+ "
+ title="Filter value"
+ [placeholder]="
+ openFilterColumn === 'time'
+ ? ''
+ : 'Value...'
+ "
+ [ngModel]="advancedInputValue2"
+ (input)="
+ openFilterColumn === 'time'
+ ? onTimestampInput(
+ 'value2',
+ $event
+ )
+ : null
+ "
+ (ngModelChange)="
+ openFilterColumn !== 'time'
+ ? (advancedInputValue2 =
$event)
+ : null
+ "
+ (mousedown)="
+ openFilterColumn === 'time'
+ ? repositionToEnd($event)
+ : null
+ "
+ (click)="$event.stopPropagation()"
+ (keydown)="
+ onAdvancedInputKeydown(
+ $event,
+ openFilterColumn,
+ 2
+ )
+ "
+ />
+ </div>
+ }
+ <div class="advanced-filter-actions">
+ <button
+ type="button"
+ class="advanced-apply-btn"
+ data-cy="column-advanced-filter-apply-btn"
+ (click)="
+ applyAdvancedFilter(openFilterColumn)
+ "
+ >
+ OK
+ </button>
+ <button
+ type="button"
+ class="advanced-cancel-btn"
+ (click)="cancelAdvancedPanel()"
+ >
+ Cancel
+ </button>
+ @if (hasAdvancedFilter(openFilterColumn)) {
+ <button
+ type="button"
+ class="advanced-clear-btn"
+ (click)="
+ clearAdvancedFilter(
+ openFilterColumn
+ )
+ "
+ >
+ Clear
+ </button>
+ }
+ </div>
+ </div>
+ }
+ </div>
+ }
+ <div [class.ts-mask-wrapper]="openFilterColumn === 'time'">
+ @if (openFilterColumn === 'time') {
+ <div class="ts-overlay" aria-hidden="true">
+ <span>{{
+ getTimestampTyped(
+ columnSearchTerms[openFilterColumn] || ''
+ )
+ }}</span
+ ><span class="ts-faded">{{
+ getTimestampTemplate(
+ columnSearchTerms[openFilterColumn] || ''
+ )
+ }}</span>
+ </div>
+ }
+ <input
+ #filterSearchInput
+ type="text"
+ class="column-filter-search"
+ [class.ts-input]="openFilterColumn === 'time'"
+ title="Search"
+ [placeholder]="
+ openFilterColumn === 'time' ? '' : 'Search...'
+ "
+ [ngModel]="columnSearchTerms[openFilterColumn]"
+ (ngModelChange)="
+ openFilterColumn !== 'time'
+ ? onColumnSearchChange(openFilterColumn, $event)
+ : null
+ "
+ (input)="
+ openFilterColumn === 'time'
+ ? onTimestampSearchInput($event)
+ : null
+ "
+ (mousedown)="
+ openFilterColumn === 'time'
+ ? repositionToEnd($event)
+ : null
+ "
+ (click)="$event.stopPropagation()"
+ (keydown)="onSearchKeydown($event)"
+ />
+ </div>
+ <div class="column-filter-controls">
+ <mat-checkbox
+ [checked]="areAllValuesSelected(openFilterColumn)"
+ [indeterminate]="
+ !areAllValuesSelected(openFilterColumn) &&
+ columnFilters[openFilterColumn]?.size > 0
+ "
+ (change)="toggleAllValues(openFilterColumn)"
+ >
+ (Select All)
+ </mat-checkbox>
+ @if (hasSearchOrAdvanced(openFilterColumn)) {
+ <mat-checkbox
+
[checked]="areDisplayedValuesSelected(openFilterColumn)"
+ (change)="toggleDisplayedValues(openFilterColumn)"
+ >
+ (Select All Displayed)
Review Comment:
More newly added filter dropdown labels are hard-coded (e.g. "(Select All)",
"(Select All Displayed)") instead of being translated. Please replace these
with i18n keys and use the `translate` pipe to match the rest of the UI.
##########
ui/src/app/chart-shared/components/charts/table/table-widget.component.html:
##########
@@ -154,4 +183,315 @@
</mat-paginator>
</div>
}
+ @if (openFilterColumn) {
+ <div
+ class="column-filter-dropdown"
+ [ngStyle]="dropdownStyle"
+ (click)="onFilterDropdownClick($event)"
+ >
+ @if (hasActiveFilter(openFilterColumn)) {
+ <button
+ type="button"
+ class="clear-filter-btn"
+ (click)="clearColumnFilter(openFilterColumn)"
+ >
+ <mat-icon class="clear-filter-icon"
+ >filter_list_off</mat-icon
+ >
+ Clear Filter
Review Comment:
Several newly added UI strings in the filter dropdown are hard-coded (e.g.
"Clear Filter") while this template otherwise uses the translate pipe. For
consistency and localization support, these labels should be moved to
translation keys and piped through `translate`.
##########
ui/src/app/dashboard/components/panel/dashboard-panel.component.ts:
##########
@@ -131,6 +135,11 @@ export class DashboardPanelComponent
this.dataExplorerSharedService.defaultObservableGenerator();
public ngOnInit() {
+ this.shortcutReg = this.shortcutService.register('dashboard-panel', [
+ { key: 'e', action: () => this.onShortcutEdit() },
+ { key: 's', ctrl: true, action: () => this.onShortcutSave() },
Review Comment:
`Ctrl/Cmd+S` is always registered and will `preventDefault()` even when
`editMode` is off (so it blocks the browser’s Save shortcut but does not
persist anything). To match the intended scope, either register/unregister
based on `editMode`, or set `preventDefault: false` and only prevent default
inside the action when `editMode` is true.
##########
ui/src/app/editor/components/pipeline-assembly/pipeline-assembly.component.ts:
##########
@@ -92,14 +101,28 @@ export class PipelineAssemblyComponent implements
AfterViewInit {
private router: Router,
private jsplumbService: JsplumbService,
private translateService: TranslateService,
+ private shortcutService: KeyboardShortcutService,
) {}
ngAfterViewInit() {
+ this.shortcutReg = this.shortcutService.register('pipeline-assembly', [
+ { key: 's', ctrl: true, action: () => this.onShortcutSave() },
+ ]);
this.jsplumbBridge = this.jsPlumbFactoryService.getJsplumbBridge(
Review Comment:
The save shortcut is registered even when `readonly` / no pipeline model is
present, but `KeyboardShortcutService` still prevents the default browser
Ctrl/Cmd+S behavior. Consider either registering only when saving is possible,
or set `preventDefault: false` and call `event.preventDefault()` only when
`submit()` will run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
