-
Notifications
You must be signed in to change notification settings - Fork 54
HybridManager: improve widget disposal #2085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: releases/26.2
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 2010, 2025 BSI Business Systems Integration AG | ||
| * Copyright (c) 2010, 2026 BSI Business Systems Integration AG | ||
| * | ||
| * This program and the accompanying materials are made | ||
| * available under the terms of the Eclipse Public License 2.0 | ||
|
|
@@ -8,8 +8,8 @@ | |
| * SPDX-License-Identifier: EPL-2.0 | ||
| */ | ||
| import { | ||
| AnyDoEntity, App, Event, EventHandler, EventListener, EventMapOf, Form, HybridActionContextElements, HybridActionEvent, HybridManagerEventMap, HybridManagerWidgetAddEvent, HybridManagerWidgetRemoveEvent, InitModelOf, ObjectOrChildModel, | ||
| scout, Session, UuidPool, Widget | ||
| AnyDoEntity, App, arrays, DisposeWidgetsHybridActionDo, Event, EventHandler, EventListener, EventMapOf, Form, HybridActionContextElements, HybridActionEvent, HybridManagerEventMap, HybridManagerWidgetAddEvent, | ||
| HybridManagerWidgetRemoveEvent, InitModelOf, ObjectOrChildModel, objects, scout, Session, UuidPool, Widget | ||
| } from '../../index'; | ||
|
|
||
| /** | ||
|
|
@@ -22,6 +22,18 @@ export class HybridManager extends Widget { | |
|
|
||
| widgets: Record<string, Widget>; | ||
|
|
||
| /** | ||
| * Set of {@link HybridManagerWidget}s that will be disposed in the next batch. | ||
| * See {@link #_callDisposeWidgets} for more information. | ||
| */ | ||
| protected _disposeWidgetsNextBatch: Set<HybridManagerWidget> = null; | ||
| /** | ||
| * Set of ids that were scheduled in former batches. | ||
| * See {@link #_callDisposeWidgets} for more information. | ||
| */ | ||
| protected _disposeWidgetsScheduledWidgetIds = new Set<string>(); | ||
| protected _widgetDestroyHandler = this._onWidgetDestroy.bind(this); | ||
|
|
||
| constructor() { | ||
| super(); | ||
|
|
||
|
|
@@ -61,15 +73,44 @@ export class HybridManager extends Widget { | |
| // widgets | ||
|
|
||
| protected _setWidgets(widgets: Record<string, ObjectOrChildModel<Widget>>) { | ||
| const presentWidgetIds = new Set<string>(); | ||
| for (const [id, widgetOrModel] of Object.entries(widgets)) { | ||
| let widgetId: string; | ||
| if (typeof widgetOrModel === 'string') { | ||
| widgetId = widgetOrModel; | ||
| } else if (objects.isObject(widgetOrModel)) { | ||
| widgetId = widgetOrModel.id; | ||
| } | ||
|
|
||
| if (!widgetId?.length) { | ||
| continue; | ||
| } | ||
|
|
||
| // collect widget id | ||
| presentWidgetIds.add(widgetId); | ||
|
|
||
| if (this._disposeWidgetsScheduledWidgetIds.has(widgetId)) { | ||
| // do not accept disposed widgets | ||
| delete widgets[id]; | ||
| } | ||
| } | ||
|
|
||
| for (const id of this._disposeWidgetsScheduledWidgetIds) { | ||
| if (!presentWidgetIds.has(id)) { | ||
| // widget is no longer present -> no need to remember id any longer | ||
| this._disposeWidgetsScheduledWidgetIds.delete(id); | ||
| } | ||
| } | ||
|
|
||
| widgets = this._ensureWidgets(widgets); | ||
|
|
||
| const removedWidgets: Record<string, Widget> = {}; | ||
| const removedWidgets: Record<string, HybridManagerWidget> = {}; | ||
| for (const [id, widget] of Object.entries(this.widgets)) { | ||
| if (!widgets[id] || widgets[id] !== widget) { | ||
| removedWidgets[id] = widget; | ||
| } | ||
| } | ||
| const addedWidgets: Record<string, Widget> = {}; | ||
| const addedWidgets: Record<string, HybridManagerWidget> = {}; | ||
| for (const [id, widget] of Object.entries(widgets as Record<string, Widget>)) { | ||
| if (!this.widgets[id] || this.widgets[id] !== widget) { | ||
| addedWidgets[id] = widget; | ||
|
|
@@ -79,8 +120,16 @@ export class HybridManager extends Widget { | |
|
|
||
| this._setProperty('widgets', widgets); | ||
|
|
||
| Object.entries(addedWidgets).forEach(([id, widget]) => this._triggerWidgetAdd(id, widget)); | ||
| Object.entries(removedWidgets).forEach(([id, widget]) => this._triggerWidgetRemove(id, widget)); | ||
| Object.entries(addedWidgets).forEach(([id, widget]) => { | ||
| this._installHybridManagerWidget(widget, id); | ||
| // trigger widgetAdd event | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need to add a comment if the method call is meaningful enough and basically the same as the comment |
||
| this._triggerWidgetAdd(id, widget); | ||
| }); | ||
| Object.entries(removedWidgets).forEach(([id, widget]) => { | ||
| this._uninstallHybridManagerWidget(widget); | ||
| // trigger widgetRemove event | ||
| this._triggerWidgetRemove(id, widget); | ||
| }); | ||
| } | ||
|
|
||
| protected _ensureWidgets(modelsOrWidgets: Record<string, ObjectOrChildModel<Widget>>): Record<string, Widget> { | ||
|
|
@@ -92,6 +141,26 @@ export class HybridManager extends Widget { | |
| return result; | ||
| } | ||
|
|
||
| protected _installHybridManagerWidget(widget: HybridManagerWidget, remoteId: string) { | ||
| if (!widget || !remoteId?.length) { | ||
| return; | ||
| } | ||
|
|
||
| // mark widget with remote id and add destroy listener | ||
| widget.__remoteId = remoteId; | ||
| widget.one('destroy', this._widgetDestroyHandler); | ||
| } | ||
|
|
||
| protected _uninstallHybridManagerWidget(widget: HybridManagerWidget) { | ||
| if (!widget?.__remoteId?.length) { | ||
| return; | ||
| } | ||
|
|
||
| // clear remote id marker and remove destroy listener | ||
| delete widget.__remoteId; | ||
| widget.off('destroy', this._widgetDestroyHandler); | ||
| } | ||
|
|
||
| protected _triggerWidgetAdd(id: string, widget: Widget) { | ||
| this.trigger(`widgetAdd:${id}`, {widget} as HybridManagerWidgetAddEvent); | ||
| } | ||
|
|
@@ -221,6 +290,55 @@ export class HybridManager extends Widget { | |
| return form; | ||
| } | ||
|
|
||
| /** | ||
| * Calls the hybrid action with the action type 'scout.DisposeWidgets' to dispose the given widgets on the UI server. | ||
| */ | ||
| disposeWidgets(widgets: Widget | Widget[]) { | ||
| this._callDisposeWidgets(widgets); | ||
| } | ||
|
|
||
| /** | ||
| * Calls the hybrid action with the action type 'scout.DisposeWidgets' to dispose the given widgets on the UI server. | ||
| * The given widgets are collected in {@link #_disposeWidgetsNextBatch} and sent in one hybrid action. | ||
| * After the hybrid action is called the ids of the {@link HybridManagerWidget}s in {@link #_disposeWidgetsNextBatch} are transferred to {@link #_disposeWidgetsScheduledWidgetIds} and {@link #_disposeWidgetsNextBatch} is reset. | ||
| */ | ||
| protected _callDisposeWidgets(widgets: HybridManagerWidget | HybridManagerWidget[]) { | ||
| // filter remote widgets | ||
| widgets = arrays.ensure(widgets).filter(widget => !!widget.__remoteId); | ||
|
|
||
| // nothing to dispose | ||
| if (!widgets.length) { | ||
| return; | ||
| } | ||
|
|
||
| // if next batch was not created already, create it and queue microtask to sent hybrid action | ||
| if (!this._disposeWidgetsNextBatch) { | ||
| this._disposeWidgetsNextBatch = new Set<HybridManagerWidget>(); | ||
| queueMicrotask(() => { | ||
| // collect remote ids, transfer widget ids to this._disposeWidgetsScheduledWidgetIds and reset next batch | ||
| const remoteIds: string[] = []; | ||
| for (const widget of [...this._disposeWidgetsNextBatch]) { | ||
| remoteIds.push(widget.__remoteId); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| this._disposeWidgetsScheduledWidgetIds.add(widget.id); | ||
| } | ||
| this._disposeWidgetsNextBatch = null; | ||
|
|
||
| // call hybrid action | ||
| this.callAction('scout.DisposeWidgets', scout.create(DisposeWidgetsHybridActionDo, {ids: remoteIds})); | ||
| }); | ||
| } | ||
|
|
||
| // remove destroy listener and add remote id to next batch | ||
| for (const widget of widgets) { | ||
| widget.off('destroy', this._widgetDestroyHandler); | ||
| this._disposeWidgetsNextBatch.add(widget); | ||
| } | ||
| } | ||
|
|
||
| protected _onWidgetDestroy(event: Event<Widget>) { | ||
| this._callDisposeWidgets(event.source); | ||
| } | ||
|
|
||
| // event support | ||
|
|
||
| override one<K extends string & keyof EventMapOf<this['self']>>(type: K | `${K}:${string}`, handler: EventHandler<EventMapOf<this>[K] & Event<this>>) { | ||
|
|
@@ -251,3 +369,7 @@ interface HybridManagerForm extends Form { | |
| */ | ||
| __closeTriggered?: boolean; | ||
| } | ||
|
|
||
| export interface HybridManagerWidget extends Widget { | ||
| __remoteId?: string; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 2010, 2025 BSI Business Systems Integration AG | ||
| * Copyright (c) 2010, 2026 BSI Business Systems Integration AG | ||
| * | ||
| * This program and the accompanying materials are made | ||
| * available under the terms of the Eclipse Public License 2.0 | ||
|
|
@@ -11,6 +11,8 @@ | |
|
|
||
| import java.util.Collection; | ||
| import java.util.Map; | ||
| import java.util.Map.Entry; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import org.eclipse.scout.rt.client.ui.IWidget; | ||
| import org.slf4j.Logger; | ||
|
|
@@ -30,13 +32,20 @@ public class DisposeWidgetsHybridAction extends AbstractHybridAction<DisposeWidg | |
|
|
||
| @Override | ||
| public void execute(DisposeWidgetsHybridActionDo data) { | ||
| for (String id : data.getIds()) { | ||
| IWidget widget = hybridManager().getWidgetById(id); | ||
| if (widget != null) { | ||
| widget.dispose(); | ||
| LOG.debug("Disposed hybrid widget with id {}", id); | ||
| } | ||
| } | ||
| // get all widgets that need to be disposed | ||
| Map<String, IWidget> widgets = hybridManager().getWidgets().entrySet().stream() | ||
| .filter(entry -> data.getIds().contains(entry.getKey()) && entry.getValue() != null) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not loop over data.getIds() and call getWidgetById()? It would be faster because looping over getWidgets() and using getIds().contains is O(n^2) |
||
| .collect(Collectors.toMap(Entry::getKey, Entry::getValue)); | ||
|
|
||
| // remove widgets from hybrid manager before disposing the widgets to reduce the update count of the hybrid managers widgets property | ||
| hybridManager().removeWidgetsById(widgets.keySet()); | ||
|
|
||
| // dispose widgets | ||
| widgets.forEach((id, widget) -> { | ||
| widget.dispose(); | ||
| LOG.debug("Disposed hybrid widget with id {}", id); | ||
| }); | ||
|
|
||
| fireHybridActionEndEvent(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name is a little cumbersome... Maybe call it widgetIdsBeingDisposed and the other one _widgetsToBeDisposed?