From e0ca5216d8d89932bb8b710f1b9bf11fe48ab6f4 Mon Sep 17 00:00:00 2001 From: naronchen Date: Mon, 5 May 2025 15:58:15 -0400 Subject: [PATCH 01/11] console log percentage working --- .../NamedVersionSelector.tsx | 5 ++ .../src/api/VersionCompareManager.ts | 19 ++++++ .../src/widgets/ChangedElementsWidget.tsx | 8 ++- .../src/widgets/ProgressCoordinator.ts | 60 +++++++++++++++++++ 4 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 packages/changed-elements-react/src/widgets/ProgressCoordinator.ts diff --git a/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx b/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx index c567e161..b5adce73 100644 --- a/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx +++ b/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx @@ -80,6 +80,11 @@ export function NamedVersionSelectorWidget(props: Readonly { + console.log(`Overall progress: ${pct}%, message: ${msg}`); + }), ]; return () => cleanup.forEach((cb) => cb()); }, diff --git a/packages/changed-elements-react/src/api/VersionCompareManager.ts b/packages/changed-elements-react/src/api/VersionCompareManager.ts index ecf87381..19d7cca8 100644 --- a/packages/changed-elements-react/src/api/VersionCompareManager.ts +++ b/packages/changed-elements-react/src/api/VersionCompareManager.ts @@ -18,6 +18,7 @@ import { ChangesTooltipProvider } from "./ChangesTooltipProvider.js"; import { VersionCompareUtils, VersionCompareVerboseMessages } from "./VerboseMessages.js"; import { VersionCompare, type VersionCompareFeatureTracking, type VersionCompareOptions } from "./VersionCompare.js"; import { VisualizationHandler } from "./VisualizationHandler.js"; +import { ProgressCoordinator, ProgressStage } from "../widgets/ProgressCoordinator.js"; const LOGGER_CATEGORY = "Version-Compare"; @@ -35,6 +36,8 @@ export class VersionCompareManager { /** Changed Elements Manager responsible for maintaining the elements obtained from the service */ public changedElementsManager: ChangedElementsManager; + private progressCoordinator: ProgressCoordinator; + private _visualizationHandler: VisualizationHandler | undefined; private _hasTypeOfChange = false; private _hasPropertiesForFiltering = false; @@ -59,6 +62,14 @@ export class VersionCompareManager { const tooltipProvider = new ChangesTooltipProvider(this); IModelApp.viewManager.addToolTipProvider(tooltipProvider); } + + // define the weight for each stage of the progress + const weights: Record = { + [ProgressStage.OpenTargetImodel]: 80, + [ProgressStage.InitComparison]: 20, + }; + + this.progressCoordinator = new ProgressCoordinator(weights); } /** Create the proper visualization handler based on options */ @@ -153,6 +164,10 @@ export class VersionCompareManager { return this._targetIModel !== undefined; } + public get onOverallProgress() { + return this.progressCoordinator.onProgressChanged; + } + /** * Elements that should be ignored during initialization. Helpful for editing applications that may not want to * compare locally changed elements. @@ -409,6 +424,8 @@ export class VersionCompareManager { IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_openingTarget"), ); + this.progressCoordinator.update(ProgressStage.OpenTargetImodel, 0); + // Open the target version IModel const changesetId = targetVersion.changesetId; this._targetIModel = await CheckpointConnection.openRemote( @@ -417,6 +434,8 @@ export class VersionCompareManager { IModelVersion.asOfChangeSet(changesetId), ); + this.progressCoordinator.update(ProgressStage.OpenTargetImodel, 100); + // Keep metadata around for UI uses and other queries this.currentVersion = currentVersion; this.targetVersion = targetVersion; diff --git a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx index 90d21ab6..5b4792d8 100644 --- a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx +++ b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx @@ -182,6 +182,11 @@ export class ChangedElementsWidget extends Component { + // console.log(`Overall progress: ${pct}%, message: ${msg}`); + // } + private _refreshCheckboxesEvent = new BeEvent<() => void>(); constructor(props: ChangedElementsWidgetProps) { @@ -219,10 +224,11 @@ export class ChangedElementsWidget extends Component; + private progress: Record; + public readonly onProgressChanged = new BeEvent<(pct: number, message: string) => void>(); + + constructor(weights: Record){ + this.weights = weights; + + // init every stage as 0 + this.progress = Object.fromEntries( + Object.keys(weights).map(stage => [stage, 0]) + ) as Record; + } + + // @naron: if we want other operations to use this, probably shouldnt hardcode prefix + public getStageMessage(stage: ProgressStage): string { + const prefix = "VersionCompare:versionCompare.msg_"; + return IModelApp.localization.getLocalizedString(`${prefix}${stage}`); + } + + /** + * Updates the progress for a specific stage with overall percentage and raises the event. + * @param stage The stage to update. + * @param progress The progress percentage for the specified stage (0-100). + */ + public update(stage: ProgressStage, progress: number): void { + this.progress[stage] = progress; + const overallPercentage = this.getOverallPercentage(); + const msg = this.getStageMessage(stage); + this.onProgressChanged.raiseEvent(overallPercentage, msg); + } + + /** + * Computes the overall progress as a weighted percentage across all stages. + * Each stage contributes proportionally based on its assigned weight. + * + * @returns A number (0-100) representing the global progress. + */ + public getOverallPercentage(): number { + return Object.entries(this.weights).reduce((sum, [stage, weight]) => { + const p = this.progress[stage as ProgressStage] ?? 0; + return sum + (p / 100) * weight; + }, 0); + } + +} From 46b3bd6aa0b8d550dfd2f9d69489dab506d47b42 Mon Sep 17 00:00:00 2001 From: naronchen Date: Tue, 6 May 2025 16:48:01 -0400 Subject: [PATCH 02/11] all states created/used --- .../NamedVersionSelector.tsx | 4 --- .../src/api/ChangedElementDataCache.ts | 6 ++-- .../src/api/ChangedElementEntryCache.ts | 27 +++++++++++++-- .../src/api/ChangedElementsManager.ts | 19 ++++++++++- .../src/api/ElementQueries.ts | 6 ++-- .../src/api/VersionCompareManager.ts | 24 +++++++++----- .../src/widgets/ChangedElementsWidget.tsx | 15 ++++----- .../src/widgets/ProgressCoordinator.ts | 33 +++++++++++++------ 8 files changed, 94 insertions(+), 40 deletions(-) diff --git a/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx b/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx index b5adce73..2f007d84 100644 --- a/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx +++ b/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx @@ -81,10 +81,6 @@ export function NamedVersionSelectorWidget(props: Readonly { - console.log(`Overall progress: ${pct}%, message: ${msg}`); - }), ]; return () => cleanup.forEach((cb) => cb()); }, diff --git a/packages/changed-elements-react/src/api/ChangedElementDataCache.ts b/packages/changed-elements-react/src/api/ChangedElementDataCache.ts index 2380b2ed..2e653b2e 100644 --- a/packages/changed-elements-react/src/api/ChangedElementDataCache.ts +++ b/packages/changed-elements-react/src/api/ChangedElementDataCache.ts @@ -20,7 +20,7 @@ export abstract class ChangedElementDataCache { /** * Called whenever a request is done in bulk mode, useful for UI update of progress */ - public updateFunction?: () => void; + public updateFunction?: (pct: number) => void; constructor(protected _chunkSize: number = 500) { // No-op @@ -50,7 +50,7 @@ export abstract class ChangedElementDataCache { const piece = await this._request(elements.slice(i, i + this._chunkSize)); result.push(...piece); if (this.updateFunction) { - this.updateFunction(); + this.updateFunction(Math.floor(i / elements.length * 100)); } } return result; @@ -111,7 +111,7 @@ export abstract class ChangedElementDataCache { return []; // If everything has been cached already, populate the data and return if (this._containedInCache(elements)) { - return this._populateEntries(elements); + return this._populateEntries(elements); //@naron: loading also doesnt get here for openPlant3 } // Split the entries into the ones we have already cached and the ones we haven't diff --git a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts index 25879504..c2b29801 100644 --- a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts +++ b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts @@ -15,6 +15,7 @@ import { import { VersionCompareUtils, VersionCompareVerboseMessages } from "./VerboseMessages.js"; import { VersionCompare } from "./VersionCompare.js"; import { VersionCompareManager } from "./VersionCompareManager.js"; +import { ProgressCoordinator, ProgressStage } from "../widgets/ProgressCoordinator.js"; /** Changed property for a changed element */ export interface Checksums { @@ -81,6 +82,7 @@ export class ChangedElementEntryCache { } private _currentIModel: IModelConnection | undefined; private _targetIModel: IModelConnection | undefined; + private _progressCoordinator: ProgressCoordinator | undefined; private _progressLoadingEvent?: BeEvent<(message: string) => void>; private _currentLoadingMessage = ""; private _numSteps = 0; @@ -132,8 +134,10 @@ export class ChangedElementEntryCache { currentIModel: IModelConnection, targetIModel: IModelConnection, elements: Map, + progressCoordinator: ProgressCoordinator, progressLoadingEvent?: BeEvent<(message: string) => void>, ) { + this._progressCoordinator = progressCoordinator; this._progressLoadingEvent = progressLoadingEvent; elements.forEach((element: ChangedElement, elementId: string) => { const entry: ChangedElementEntry = { @@ -605,6 +609,7 @@ export class ChangedElementEntryCache { this._findTopParentChunkSize + 1; this._setCurrentLoadingMessage("msg_findingParents", numTopParentQueries); + this._progressCoordinator?.updateProgress(ProgressStage.FindParents); const currentTopParents = await this._findTopParents( this._currentIModel, currentEntryIds, @@ -643,14 +648,19 @@ export class ChangedElementEntryCache { (unchangedCurrentTopParents.length + unchangedTargetTopParents.length) / this._queryEntryChunkSize + 1; + + this._progressCoordinator?.addProgress(ProgressStage.FindParents, 100); + this._setCurrentLoadingMessage("msg_obtainingElementData", numEntryQueries); + this._progressCoordinator?.updateProgress(ProgressStage.ObtainElementData); + const currentParentEntries = await queryEntryDataBulk( this._currentIModel, VersionCompare.manager?.wantFastParentLoad ? unchangedCurrentTopParents : currentTopParents, this._queryEntryChunkSize, - this._updateLoadingProgress, + (pct) => this._progressCoordinator?.addProgress(ProgressStage.ObtainElementData, pct/2), // @naron: should we have different stage/msg ); const targetParentEntries = await queryEntryDataBulk( this._targetIModel, @@ -658,7 +668,7 @@ export class ChangedElementEntryCache { ? unchangedTargetTopParents : targetTopParents, this._queryEntryChunkSize, - this._updateLoadingProgress, + (pct) => this._progressCoordinator?.addProgress(ProgressStage.ObtainElementData, pct/2), ); // Put all data into arrays @@ -701,11 +711,15 @@ export class ChangedElementEntryCache { } } } + this._progressCoordinator?.updateProgress(ProgressStage.ObtainElementData, 100); + this._progressCoordinator?.updateProgress(ProgressStage.FindChildren, 0); // Load child elements of the root nodes if we are not using fast parent loading if (this._childrenCache && !VersionCompare.manager?.wantFastParentLoad) { // Set update function for UI updates - this._childrenCache.updateFunction = this._updateLoadingProgress; + this._childrenCache.updateFunction = (pct: number) => { + this._progressCoordinator?.addProgress(ProgressStage.FindChildren, pct); + } const numQueries = this._childrenCache.calculateNumberOfRequests( parentEntries.length, ); @@ -715,6 +729,7 @@ export class ChangedElementEntryCache { // Clean-up usage of update function this._childrenCache.updateFunction = undefined; } + this._progressCoordinator?.updateProgress(ProgressStage.FindChildren, 100); // Put together all entries const finalEntries: ChangedElementEntry[] = []; @@ -743,11 +758,17 @@ export class ChangedElementEntryCache { // For now, use the 6 steps (3 per iModel) to get the models this._setCurrentLoadingMessage("loadingModelNodes", 6); this._updateLoadingProgress(); + + this._progressCoordinator?.updateProgress(ProgressStage.LoadIModelNodes); + this._uiDataProvider = new ChangesTreeDataProvider(this._manager); await this._uiDataProvider.loadChangedModelNodes( this._currentIModel, this._targetIModel, + () => this._progressCoordinator?.addProgress(ProgressStage.LoadIModelNodes, 25), // since updateFunc gets called 4 times ); + + this._progressCoordinator?.updateProgress(ProgressStage.LoadIModelNodes, 100); } }; } diff --git a/packages/changed-elements-react/src/api/ChangedElementsManager.ts b/packages/changed-elements-react/src/api/ChangedElementsManager.ts index 9e5b6207..7da40a61 100644 --- a/packages/changed-elements-react/src/api/ChangedElementsManager.ts +++ b/packages/changed-elements-react/src/api/ChangedElementsManager.ts @@ -10,6 +10,7 @@ import { ChangedElementEntryCache, type ChangedElement, type Checksums } from ". import { ChangedElementsChildrenCache } from "./ChangedElementsChildrenCache.js"; import { ChangedElementsLabelsCache } from "./ChangedElementsLabelCache.js"; import { VersionCompareManager } from "./VersionCompareManager.js"; +import { ProgressCoordinator, ProgressStage } from "../widgets/ProgressCoordinator.js"; /** Properties that are not shown but still found by the agent */ const ignoredProperties = ["Checksum", "Version"]; @@ -449,11 +450,13 @@ export class ChangedElementsManager { public async generateEntries( currentIModel: IModelConnection, targetIModel: IModelConnection, + progressCoordinator: ProgressCoordinator, ): Promise { this._entryCache.initialize( currentIModel, targetIModel, this._changedElements, + progressCoordinator, this._progressLoadingEvent, ); } @@ -542,6 +545,7 @@ export class ChangedElementsManager { currentIModel: IModelConnection, targetIModel: IModelConnection, forward: boolean, + progressCoordinator: ProgressCoordinator, progressLoadingEvent?: BeEvent<(message: string) => void>, ): Promise> { // If we have model ids in the data already, simply accumulate the models from it instead of querying @@ -596,6 +600,12 @@ export class ChangedElementsManager { steps, ); + // @naron: not sure how to trigger it here, its always has the model ids in the data already and return early + progressCoordinator.updateProgress( + ProgressStage.ComputeChangedModels, + Math.floor(((lastStep ?? 0) + currentStep) / (steps === 0 ? 1 : steps) * 100), + ); + for await (const row of iModel.query(ecsql, QueryBinder.from(piece), { rowFormat: QueryRowFormat.UseJsPropertyNames, })) { @@ -1143,11 +1153,13 @@ export class ChangedElementsManager { * @param forward Whether we are comparing to a newer iModel or an older one (normally the older) * @param filterSpatial Whether to filter out non-spatial elements from the results * @param progressLoadingEvent Event raised every time the processing continues to provide UI messages to the user + * @param onOverallProgress Event raised every time the processing continues to provide UI messages to the user */ public async initialize( currentIModel: IModelConnection, targetIModel: IModelConnection, changedElements: ChangedElements[], + progressCoordinator: ProgressCoordinator, //@naron: can i just make it requried? wantedModelClasses?: string[], forward?: boolean, filterSpatial?: boolean, @@ -1170,11 +1182,14 @@ export class ChangedElementsManager { ); } + progressCoordinator.updateProgress(ProgressStage.ComputeChangedModels); + // Find changed models this._changedModels = await this.findChangedModels( currentIModel, targetIModel, forward ?? false, + progressCoordinator, progressLoadingEvent, ); @@ -1184,13 +1199,15 @@ export class ChangedElementsManager { ); } + progressCoordinator.updateProgress(ProgressStage.ComputeChangedModels, 100); + // Find unchanged models this._unchangedModels = await this.findUnchangedModels( currentIModel, this._changedModels, ); - await this.generateEntries(currentIModel, targetIModel); + await this.generateEntries(currentIModel, targetIModel, progressCoordinator); } } diff --git a/packages/changed-elements-react/src/api/ElementQueries.ts b/packages/changed-elements-react/src/api/ElementQueries.ts index 4de59e4d..62df5d11 100644 --- a/packages/changed-elements-react/src/api/ElementQueries.ts +++ b/packages/changed-elements-react/src/api/ElementQueries.ts @@ -128,10 +128,10 @@ export const queryEntryDataBulk = async ( iModel: IModelConnection, elementIds: string[], chunkSize = 1000, - updateFunc?: () => void, + updateFunc?: (pct: number) => void, ): Promise => { if (elementIds.length < chunkSize) { - return queryEntryData(iModel, elementIds); + return queryEntryData(iModel, elementIds); //@naron: not able to test on openPlant, always gets retuned here | change the chunk size to test } const final: ChangedElementQueryData[] = []; @@ -142,7 +142,7 @@ export const queryEntryDataBulk = async ( ); final.push(...data); if (updateFunc) { - updateFunc(); + updateFunc(Math.floor(i / elementIds.length * 100)); } } return final; diff --git a/packages/changed-elements-react/src/api/VersionCompareManager.ts b/packages/changed-elements-react/src/api/VersionCompareManager.ts index 19d7cca8..edee4511 100644 --- a/packages/changed-elements-react/src/api/VersionCompareManager.ts +++ b/packages/changed-elements-react/src/api/VersionCompareManager.ts @@ -65,8 +65,13 @@ export class VersionCompareManager { // define the weight for each stage of the progress const weights: Record = { - [ProgressStage.OpenTargetImodel]: 80, - [ProgressStage.InitComparison]: 20, + [ProgressStage.OpenTargetImodel]: 10, + [ProgressStage.InitComparison]: 10, + [ProgressStage.ComputeChangedModels]: 10, + [ProgressStage.FindParents]: 10, + [ProgressStage.ObtainElementData]: 10, + [ProgressStage.FindChildren]: 10, + [ProgressStage.LoadIModelNodes]: 40, }; this.progressCoordinator = new ProgressCoordinator(weights); @@ -328,6 +333,7 @@ export class VersionCompareManager { this._currentIModel, this._targetIModel, changedElements, + this.progressCoordinator, this.wantAllModels ? undefined : wantedModelClasses, false, this.filterSpatial, @@ -424,7 +430,7 @@ export class VersionCompareManager { IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_openingTarget"), ); - this.progressCoordinator.update(ProgressStage.OpenTargetImodel, 0); + this.progressCoordinator.updateProgress(ProgressStage.OpenTargetImodel); // Open the target version IModel const changesetId = targetVersion.changesetId; @@ -434,20 +440,18 @@ export class VersionCompareManager { IModelVersion.asOfChangeSet(changesetId), ); - this.progressCoordinator.update(ProgressStage.OpenTargetImodel, 100); + this.progressCoordinator.updateProgress(ProgressStage.OpenTargetImodel, 100); // Keep metadata around for UI uses and other queries this.currentVersion = currentVersion; this.targetVersion = targetVersion; - this.loadingProgressEvent.raiseEvent( - IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_getChangedElements"), - ); - this.loadingProgressEvent.raiseEvent( IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_initializingComparison"), ); + this.progressCoordinator.updateProgress(ProgressStage.InitComparison); + let wantedModelClasses = [ GeometricModel2dState.classFullName, GeometricModel3dState.classFullName, @@ -459,10 +463,14 @@ export class VersionCompareManager { if (this.ignoredElementIds !== undefined) { filteredChangedElements = this._filterIgnoredElementsFromChangesets(changedElements); } + + this.progressCoordinator.updateProgress(ProgressStage.InitComparison, 100); //@naron: is the following initialize also part of init comparison? if so, we need to maintain hierarchy of progress stages + await this.changedElementsManager.initialize( this._currentIModel, this._targetIModel, filteredChangedElements, + this.progressCoordinator, this.wantAllModels ? undefined : wantedModelClasses, false, this.filterSpatial, diff --git a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx index 5b4792d8..e7ef1a7c 100644 --- a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx +++ b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx @@ -142,7 +142,7 @@ export class ChangedElementsWidget extends Component { - // console.log(`Overall progress: ${pct}%, message: ${msg}`); - // } + private _onOverallProgress = (pct: number, msg: string): void => { + console.log(`Overall progress: ${pct}%, message: ${msg}`); + } private _refreshCheckboxesEvent = new BeEvent<() => void>(); @@ -224,16 +223,16 @@ export class ChangedElementsWidget extends Component; } - // @naron: if we want other operations to use this, probably shouldnt hardcode prefix public getStageMessage(stage: ProgressStage): string { - const prefix = "VersionCompare:versionCompare.msg_"; - return IModelApp.localization.getLocalizedString(`${prefix}${stage}`); + // const prefix = "VersionCompare:versionCompare."; + // return IModelApp.localization.getLocalizedString(`${prefix}${stage}`); //@naron: change this to loading + return "loading comparison" } /** @@ -37,13 +39,25 @@ export class ProgressCoordinator{ * @param stage The stage to update. * @param progress The progress percentage for the specified stage (0-100). */ - public update(stage: ProgressStage, progress: number): void { + public updateProgress(stage: ProgressStage, progress: number = 0): void { this.progress[stage] = progress; const overallPercentage = this.getOverallPercentage(); const msg = this.getStageMessage(stage); this.onProgressChanged.raiseEvent(overallPercentage, msg); } + /** + * Increment the progress for a specific stage with overall percentage and raises the event. + * @param stage The stage to update. + * @param progress The progress percentage for the specified stage (0-100). + */ + public addProgress(stage: ProgressStage, progress: number): void { + this.progress[stage] += progress; + const overallPercentage = this.getOverallPercentage(); + const msg = this.getStageMessage(stage); + this.onProgressChanged.raiseEvent(overallPercentage, msg); + } + /** * Computes the overall progress as a weighted percentage across all stages. * Each stage contributes proportionally based on its assigned weight. @@ -56,5 +70,4 @@ export class ProgressCoordinator{ return sum + (p / 100) * weight; }, 0); } - } From fe68874ea7d92d1187638ddc5d28810173880f4e Mon Sep 17 00:00:00 2001 From: naronchen Date: Wed, 7 May 2025 10:26:23 -0400 Subject: [PATCH 03/11] make optional and have progress stage --- .../src/api/ChangedElementEntryCache.ts | 2 +- .../src/api/ChangedElementsManager.ts | 14 ++++---- .../src/api/VersionCompareManager.ts | 32 +++++++++---------- .../src/widgets/ChangedElementsWidget.tsx | 10 +++--- .../src/widgets/ProgressCoordinator.ts | 25 +++++++++------ 5 files changed, 44 insertions(+), 39 deletions(-) diff --git a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts index c2b29801..e28c9dd8 100644 --- a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts +++ b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts @@ -134,7 +134,7 @@ export class ChangedElementEntryCache { currentIModel: IModelConnection, targetIModel: IModelConnection, elements: Map, - progressCoordinator: ProgressCoordinator, + progressCoordinator?: ProgressCoordinator, progressLoadingEvent?: BeEvent<(message: string) => void>, ) { this._progressCoordinator = progressCoordinator; diff --git a/packages/changed-elements-react/src/api/ChangedElementsManager.ts b/packages/changed-elements-react/src/api/ChangedElementsManager.ts index 7da40a61..ba6d5ffb 100644 --- a/packages/changed-elements-react/src/api/ChangedElementsManager.ts +++ b/packages/changed-elements-react/src/api/ChangedElementsManager.ts @@ -450,7 +450,7 @@ export class ChangedElementsManager { public async generateEntries( currentIModel: IModelConnection, targetIModel: IModelConnection, - progressCoordinator: ProgressCoordinator, + progressCoordinator?: ProgressCoordinator, ): Promise { this._entryCache.initialize( currentIModel, @@ -545,7 +545,7 @@ export class ChangedElementsManager { currentIModel: IModelConnection, targetIModel: IModelConnection, forward: boolean, - progressCoordinator: ProgressCoordinator, + progressCoordinator?: ProgressCoordinator, progressLoadingEvent?: BeEvent<(message: string) => void>, ): Promise> { // If we have model ids in the data already, simply accumulate the models from it instead of querying @@ -601,7 +601,7 @@ export class ChangedElementsManager { ); // @naron: not sure how to trigger it here, its always has the model ids in the data already and return early - progressCoordinator.updateProgress( + progressCoordinator?.updateProgress( ProgressStage.ComputeChangedModels, Math.floor(((lastStep ?? 0) + currentStep) / (steps === 0 ? 1 : steps) * 100), ); @@ -1159,10 +1159,10 @@ export class ChangedElementsManager { currentIModel: IModelConnection, targetIModel: IModelConnection, changedElements: ChangedElements[], - progressCoordinator: ProgressCoordinator, //@naron: can i just make it requried? wantedModelClasses?: string[], forward?: boolean, filterSpatial?: boolean, + progressCoordinator?: ProgressCoordinator, //@naron: can i just make it requried? progressLoadingEvent?: BeEvent<(message: string) => void>, ): Promise { this._progressLoadingEvent = progressLoadingEvent; @@ -1181,8 +1181,7 @@ export class ChangedElementsManager { IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_computingChangedModels"), ); } - - progressCoordinator.updateProgress(ProgressStage.ComputeChangedModels); + progressCoordinator?.updateProgress(ProgressStage.ComputeChangedModels); // Find changed models this._changedModels = await this.findChangedModels( @@ -1198,8 +1197,7 @@ export class ChangedElementsManager { IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_computingUnchangedModels"), ); } - - progressCoordinator.updateProgress(ProgressStage.ComputeChangedModels, 100); + progressCoordinator?.updateProgress(ProgressStage.ComputeChangedModels, 100); // Find unchanged models this._unchangedModels = await this.findUnchangedModels( diff --git a/packages/changed-elements-react/src/api/VersionCompareManager.ts b/packages/changed-elements-react/src/api/VersionCompareManager.ts index edee4511..72744a9b 100644 --- a/packages/changed-elements-react/src/api/VersionCompareManager.ts +++ b/packages/changed-elements-react/src/api/VersionCompareManager.ts @@ -63,7 +63,7 @@ export class VersionCompareManager { IModelApp.viewManager.addToolTipProvider(tooltipProvider); } - // define the weight for each stage of the progress + // define the weight for each stage of the progress @naron: just put the weights in ProgressCoordinator const weights: Record = { [ProgressStage.OpenTargetImodel]: 10, [ProgressStage.InitComparison]: 10, @@ -74,7 +74,17 @@ export class VersionCompareManager { [ProgressStage.LoadIModelNodes]: 40, }; - this.progressCoordinator = new ProgressCoordinator(weights); + const stageOrder: ProgressStage[] = [ + ProgressStage.OpenTargetImodel, + ProgressStage.InitComparison, + ProgressStage.ComputeChangedModels, + ProgressStage.FindParents, + ProgressStage.ObtainElementData, + ProgressStage.FindChildren, + ProgressStage.LoadIModelNodes, + ] + + this.progressCoordinator = new ProgressCoordinator(weights, stageOrder); } /** Create the proper visualization handler based on options */ @@ -138,7 +148,7 @@ export class VersionCompareManager { public versionCompareStopped = new BeEvent<() => void>(); /** Triggers during startup to show progress messages to any listener. */ - public loadingProgressEvent = new BeEvent<(message: string) => void>(); + public loadingProgressEvent = new BeEvent<(message: string) => void>(); // @naron: should I remove this Or leave it for startComparisonV1? /** Current Version for comparison. */ public currentVersion: NamedVersion | undefined; @@ -333,10 +343,10 @@ export class VersionCompareManager { this._currentIModel, this._targetIModel, changedElements, - this.progressCoordinator, this.wantAllModels ? undefined : wantedModelClasses, false, this.filterSpatial, + undefined, this.loadingProgressEvent, ); const changedElementEntries = this.changedElementsManager.entryCache.getAll(); @@ -426,10 +436,6 @@ export class VersionCompareManager { throw new Error("Cannot compare with an iModel lacking iModelId or iTwinId (aka projectId)"); } - this.loadingProgressEvent.raiseEvent( - IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_openingTarget"), - ); - this.progressCoordinator.updateProgress(ProgressStage.OpenTargetImodel); // Open the target version IModel @@ -446,10 +452,6 @@ export class VersionCompareManager { this.currentVersion = currentVersion; this.targetVersion = targetVersion; - this.loadingProgressEvent.raiseEvent( - IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_initializingComparison"), - ); - this.progressCoordinator.updateProgress(ProgressStage.InitComparison); let wantedModelClasses = [ @@ -470,11 +472,10 @@ export class VersionCompareManager { this._currentIModel, this._targetIModel, filteredChangedElements, - this.progressCoordinator, this.wantAllModels ? undefined : wantedModelClasses, false, this.filterSpatial, - this.loadingProgressEvent, + this.progressCoordinator, ); const changedElementEntries = this.changedElementsManager.entryCache.getAll(); @@ -490,9 +491,6 @@ export class VersionCompareManager { ); // Get the entries - this.loadingProgressEvent.raiseEvent( - IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_findingAssemblies"), - ); await this.changedElementsManager.entryCache.initialLoad(changedElementEntries.map((entry) => entry.id)); // Reset the select tool to allow external iModels to be located diff --git a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx index e7ef1a7c..79b629ff 100644 --- a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx +++ b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx @@ -183,7 +183,9 @@ export class ChangedElementsWidget extends Component { - console.log(`Overall progress: ${pct}%, message: ${msg}`); + const message = `${msg} progress: ${pct}%`; + console.log(message); + this.setState({ message, loading: true, description: "" }); } private _refreshCheckboxesEvent = new BeEvent<() => void>(); @@ -223,16 +225,16 @@ export class ChangedElementsWidget extends Component; private progress: Record; + private stageOrder: ProgressStage[]; // added to prevent race condition when updating progress public readonly onProgressChanged = new BeEvent<(pct: number, message: string) => void>(); - constructor(weights: Record){ + constructor(weights: Record, stageOrder: ProgressStage[]) { this.weights = weights; - + this.stageOrder = stageOrder; // init every stage as 0 this.progress = Object.fromEntries( Object.keys(weights).map(stage => [stage, 0]) ) as Record; } - public getStageMessage(stage: ProgressStage): string { + public getStageMessage(): string { // const prefix = "VersionCompare:versionCompare."; // return IModelApp.localization.getLocalizedString(`${prefix}${stage}`); //@naron: change this to loading return "loading comparison" @@ -41,9 +42,12 @@ export class ProgressCoordinator{ */ public updateProgress(stage: ProgressStage, progress: number = 0): void { this.progress[stage] = progress; - const overallPercentage = this.getOverallPercentage(); - const msg = this.getStageMessage(stage); - this.onProgressChanged.raiseEvent(overallPercentage, msg); + const overallPercentage = this.getOverallPercentage(); //@naron: there are race condition which this was returning 100% early + const cap = this.stageOrder + .slice(0, this.stageOrder.indexOf(stage) + 1) + .reduce((sum, s) => sum + this.weights[s], 0); + const msg = this.getStageMessage(); + this.onProgressChanged.raiseEvent(Math.min(cap, overallPercentage), msg); } /** @@ -53,9 +57,12 @@ export class ProgressCoordinator{ */ public addProgress(stage: ProgressStage, progress: number): void { this.progress[stage] += progress; - const overallPercentage = this.getOverallPercentage(); - const msg = this.getStageMessage(stage); - this.onProgressChanged.raiseEvent(overallPercentage, msg); + const overallPercentage = this.getOverallPercentage(); //@naron: break this part into a private function + const cap = this.stageOrder + .slice(0, this.stageOrder.indexOf(stage) + 1) + .reduce((sum, s) => sum + this.weights[s], 0); + const msg = this.getStageMessage(); + this.onProgressChanged.raiseEvent(Math.min(cap, overallPercentage), msg); } /** From 762506d31eeb57ec9d78b19cf14ed9c550ac6231 Mon Sep 17 00:00:00 2001 From: naronchen Date: Wed, 7 May 2025 17:46:42 -0400 Subject: [PATCH 04/11] handle race condition, unwire stage message --- .../src/api/ChangedElementEntryCache.ts | 34 ++++---- .../src/api/ChangedElementsManager.ts | 16 ++-- .../src/api/VersionCompareManager.ts | 56 ++++++------- .../src/widgets/ChangedElementsWidget.tsx | 8 +- .../src/widgets/ProgressCoordinator.ts | 83 ++++++++----------- 5 files changed, 93 insertions(+), 104 deletions(-) diff --git a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts index e28c9dd8..0bfd44ef 100644 --- a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts +++ b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts @@ -14,8 +14,8 @@ import { } from "./ElementQueries.js"; import { VersionCompareUtils, VersionCompareVerboseMessages } from "./VerboseMessages.js"; import { VersionCompare } from "./VersionCompare.js"; -import { VersionCompareManager } from "./VersionCompareManager.js"; -import { ProgressCoordinator, ProgressStage } from "../widgets/ProgressCoordinator.js"; +import { VersionCompareManager, VersionCompareProgressStage } from "./VersionCompareManager.js"; +import { ProgressCoordinator } from "../widgets/ProgressCoordinator.js"; /** Changed property for a changed element */ export interface Checksums { @@ -82,7 +82,7 @@ export class ChangedElementEntryCache { } private _currentIModel: IModelConnection | undefined; private _targetIModel: IModelConnection | undefined; - private _progressCoordinator: ProgressCoordinator | undefined; + private _progressCoordinator: ProgressCoordinator | undefined; private _progressLoadingEvent?: BeEvent<(message: string) => void>; private _currentLoadingMessage = ""; private _numSteps = 0; @@ -134,7 +134,7 @@ export class ChangedElementEntryCache { currentIModel: IModelConnection, targetIModel: IModelConnection, elements: Map, - progressCoordinator?: ProgressCoordinator, + progressCoordinator?: ProgressCoordinator, progressLoadingEvent?: BeEvent<(message: string) => void>, ) { this._progressCoordinator = progressCoordinator; @@ -608,8 +608,8 @@ export class ChangedElementEntryCache { (currentEntryIds.length + targetEntryIds.length) / this._findTopParentChunkSize + 1; - this._setCurrentLoadingMessage("msg_findingParents", numTopParentQueries); - this._progressCoordinator?.updateProgress(ProgressStage.FindParents); + this._setCurrentLoadingMessage("msg_findingParents", numTopParentQueries); //@naron: should we remove occurences for this. I left it for startComparisonV1 + this._progressCoordinator?.updateProgress(VersionCompareProgressStage.FindParents); const currentTopParents = await this._findTopParents( this._currentIModel, currentEntryIds, @@ -649,10 +649,10 @@ export class ChangedElementEntryCache { this._queryEntryChunkSize + 1; - this._progressCoordinator?.addProgress(ProgressStage.FindParents, 100); + this._progressCoordinator?.updateProgress(VersionCompareProgressStage.FindParents, 100); this._setCurrentLoadingMessage("msg_obtainingElementData", numEntryQueries); - this._progressCoordinator?.updateProgress(ProgressStage.ObtainElementData); + this._progressCoordinator?.updateProgress(VersionCompareProgressStage.ObtainElementData); const currentParentEntries = await queryEntryDataBulk( this._currentIModel, @@ -660,7 +660,7 @@ export class ChangedElementEntryCache { ? unchangedCurrentTopParents : currentTopParents, this._queryEntryChunkSize, - (pct) => this._progressCoordinator?.addProgress(ProgressStage.ObtainElementData, pct/2), // @naron: should we have different stage/msg + (pct) => this._progressCoordinator?.addProgress(VersionCompareProgressStage.ObtainElementData, pct/2), // @naron: hardcoded /2 cuz we call queryEntryDataBulk twice ); const targetParentEntries = await queryEntryDataBulk( this._targetIModel, @@ -668,7 +668,7 @@ export class ChangedElementEntryCache { ? unchangedTargetTopParents : targetTopParents, this._queryEntryChunkSize, - (pct) => this._progressCoordinator?.addProgress(ProgressStage.ObtainElementData, pct/2), + (pct) => this._progressCoordinator?.addProgress(VersionCompareProgressStage.ObtainElementData, pct/2), ); // Put all data into arrays @@ -711,14 +711,14 @@ export class ChangedElementEntryCache { } } } - this._progressCoordinator?.updateProgress(ProgressStage.ObtainElementData, 100); + this._progressCoordinator?.updateProgress(VersionCompareProgressStage.ObtainElementData, 100); - this._progressCoordinator?.updateProgress(ProgressStage.FindChildren, 0); + this._progressCoordinator?.updateProgress(VersionCompareProgressStage.FindChildren, 0); // Load child elements of the root nodes if we are not using fast parent loading if (this._childrenCache && !VersionCompare.manager?.wantFastParentLoad) { // Set update function for UI updates this._childrenCache.updateFunction = (pct: number) => { - this._progressCoordinator?.addProgress(ProgressStage.FindChildren, pct); + this._progressCoordinator?.addProgress(VersionCompareProgressStage.FindChildren, pct); } const numQueries = this._childrenCache.calculateNumberOfRequests( parentEntries.length, @@ -729,7 +729,7 @@ export class ChangedElementEntryCache { // Clean-up usage of update function this._childrenCache.updateFunction = undefined; } - this._progressCoordinator?.updateProgress(ProgressStage.FindChildren, 100); + this._progressCoordinator?.updateProgress(VersionCompareProgressStage.FindChildren, 100); // Put together all entries const finalEntries: ChangedElementEntry[] = []; @@ -759,16 +759,16 @@ export class ChangedElementEntryCache { this._setCurrentLoadingMessage("loadingModelNodes", 6); this._updateLoadingProgress(); - this._progressCoordinator?.updateProgress(ProgressStage.LoadIModelNodes); + this._progressCoordinator?.updateProgress(VersionCompareProgressStage.LoadIModelNodes); this._uiDataProvider = new ChangesTreeDataProvider(this._manager); await this._uiDataProvider.loadChangedModelNodes( this._currentIModel, this._targetIModel, - () => this._progressCoordinator?.addProgress(ProgressStage.LoadIModelNodes, 25), // since updateFunc gets called 4 times + () => this._progressCoordinator?.addProgress(VersionCompareProgressStage.LoadIModelNodes, 25), // @naron: hardcoded since updateFunc gets called 4 times ); - this._progressCoordinator?.updateProgress(ProgressStage.LoadIModelNodes, 100); + this._progressCoordinator?.updateProgress(VersionCompareProgressStage.LoadIModelNodes, 100); } }; } diff --git a/packages/changed-elements-react/src/api/ChangedElementsManager.ts b/packages/changed-elements-react/src/api/ChangedElementsManager.ts index ba6d5ffb..ef3d4eff 100644 --- a/packages/changed-elements-react/src/api/ChangedElementsManager.ts +++ b/packages/changed-elements-react/src/api/ChangedElementsManager.ts @@ -9,8 +9,8 @@ import { IModelApp, IModelConnection, ModelState } from "@itwin/core-frontend"; import { ChangedElementEntryCache, type ChangedElement, type Checksums } from "./ChangedElementEntryCache.js"; import { ChangedElementsChildrenCache } from "./ChangedElementsChildrenCache.js"; import { ChangedElementsLabelsCache } from "./ChangedElementsLabelCache.js"; -import { VersionCompareManager } from "./VersionCompareManager.js"; -import { ProgressCoordinator, ProgressStage } from "../widgets/ProgressCoordinator.js"; +import { VersionCompareManager, VersionCompareProgressStage } from "./VersionCompareManager.js"; +import { ProgressCoordinator } from "../widgets/ProgressCoordinator.js"; /** Properties that are not shown but still found by the agent */ const ignoredProperties = ["Checksum", "Version"]; @@ -450,7 +450,7 @@ export class ChangedElementsManager { public async generateEntries( currentIModel: IModelConnection, targetIModel: IModelConnection, - progressCoordinator?: ProgressCoordinator, + progressCoordinator?: ProgressCoordinator, ): Promise { this._entryCache.initialize( currentIModel, @@ -545,7 +545,7 @@ export class ChangedElementsManager { currentIModel: IModelConnection, targetIModel: IModelConnection, forward: boolean, - progressCoordinator?: ProgressCoordinator, + progressCoordinator?: ProgressCoordinator, progressLoadingEvent?: BeEvent<(message: string) => void>, ): Promise> { // If we have model ids in the data already, simply accumulate the models from it instead of querying @@ -602,7 +602,7 @@ export class ChangedElementsManager { // @naron: not sure how to trigger it here, its always has the model ids in the data already and return early progressCoordinator?.updateProgress( - ProgressStage.ComputeChangedModels, + VersionCompareProgressStage.ComputeChangedModels, Math.floor(((lastStep ?? 0) + currentStep) / (steps === 0 ? 1 : steps) * 100), ); @@ -1162,7 +1162,7 @@ export class ChangedElementsManager { wantedModelClasses?: string[], forward?: boolean, filterSpatial?: boolean, - progressCoordinator?: ProgressCoordinator, //@naron: can i just make it requried? + progressCoordinator?: ProgressCoordinator, //@naron: can i just make it requried? progressLoadingEvent?: BeEvent<(message: string) => void>, ): Promise { this._progressLoadingEvent = progressLoadingEvent; @@ -1181,7 +1181,7 @@ export class ChangedElementsManager { IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_computingChangedModels"), ); } - progressCoordinator?.updateProgress(ProgressStage.ComputeChangedModels); + progressCoordinator?.updateProgress(VersionCompareProgressStage.ComputeChangedModels); // Find changed models this._changedModels = await this.findChangedModels( @@ -1197,7 +1197,7 @@ export class ChangedElementsManager { IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.msg_computingUnchangedModels"), ); } - progressCoordinator?.updateProgress(ProgressStage.ComputeChangedModels, 100); + progressCoordinator?.updateProgress(VersionCompareProgressStage.ComputeChangedModels, 100); // Find unchanged models this._unchangedModels = await this.findUnchangedModels( diff --git a/packages/changed-elements-react/src/api/VersionCompareManager.ts b/packages/changed-elements-react/src/api/VersionCompareManager.ts index 72744a9b..fb37ebf3 100644 --- a/packages/changed-elements-react/src/api/VersionCompareManager.ts +++ b/packages/changed-elements-react/src/api/VersionCompareManager.ts @@ -18,10 +18,20 @@ import { ChangesTooltipProvider } from "./ChangesTooltipProvider.js"; import { VersionCompareUtils, VersionCompareVerboseMessages } from "./VerboseMessages.js"; import { VersionCompare, type VersionCompareFeatureTracking, type VersionCompareOptions } from "./VersionCompare.js"; import { VisualizationHandler } from "./VisualizationHandler.js"; -import { ProgressCoordinator, ProgressStage } from "../widgets/ProgressCoordinator.js"; +import { ProgressCoordinator } from "../widgets/ProgressCoordinator.js"; const LOGGER_CATEGORY = "Version-Compare"; +export enum VersionCompareProgressStage { + OpenTargetImodel, + InitComparison, + ComputeChangedModels, + FindParents, + ObtainElementData, + FindChildren, + LoadIModelNodes, +} + /** * Main orchestrator for version compare functionality and workflows. This class does the following: * @@ -36,13 +46,24 @@ export class VersionCompareManager { /** Changed Elements Manager responsible for maintaining the elements obtained from the service */ public changedElementsManager: ChangedElementsManager; - private progressCoordinator: ProgressCoordinator; + private progressCoordinator: ProgressCoordinator; private _visualizationHandler: VisualizationHandler | undefined; private _hasTypeOfChange = false; private _hasPropertiesForFiltering = false; private _hasParentIds = false; + // define stage and order + private progressStages = [ + { stage: VersionCompareProgressStage.OpenTargetImodel, weight: 10 }, + { stage: VersionCompareProgressStage.InitComparison, weight: 10 }, + { stage: VersionCompareProgressStage.ComputeChangedModels,weight: 10 }, + { stage: VersionCompareProgressStage.FindParents, weight: 10 }, + { stage: VersionCompareProgressStage.ObtainElementData, weight: 10 }, + { stage: VersionCompareProgressStage.FindChildren, weight: 10 }, + { stage: VersionCompareProgressStage.LoadIModelNodes, weight: 40 }, + ] as const; + /** Version Compare ITwinLocalization Namespace */ public static namespace = "VersionCompare"; @@ -63,28 +84,7 @@ export class VersionCompareManager { IModelApp.viewManager.addToolTipProvider(tooltipProvider); } - // define the weight for each stage of the progress @naron: just put the weights in ProgressCoordinator - const weights: Record = { - [ProgressStage.OpenTargetImodel]: 10, - [ProgressStage.InitComparison]: 10, - [ProgressStage.ComputeChangedModels]: 10, - [ProgressStage.FindParents]: 10, - [ProgressStage.ObtainElementData]: 10, - [ProgressStage.FindChildren]: 10, - [ProgressStage.LoadIModelNodes]: 40, - }; - - const stageOrder: ProgressStage[] = [ - ProgressStage.OpenTargetImodel, - ProgressStage.InitComparison, - ProgressStage.ComputeChangedModels, - ProgressStage.FindParents, - ProgressStage.ObtainElementData, - ProgressStage.FindChildren, - ProgressStage.LoadIModelNodes, - ] - - this.progressCoordinator = new ProgressCoordinator(weights, stageOrder); + this.progressCoordinator = new ProgressCoordinator(this.progressStages); } /** Create the proper visualization handler based on options */ @@ -436,7 +436,7 @@ export class VersionCompareManager { throw new Error("Cannot compare with an iModel lacking iModelId or iTwinId (aka projectId)"); } - this.progressCoordinator.updateProgress(ProgressStage.OpenTargetImodel); + this.progressCoordinator.updateProgress(VersionCompareProgressStage.OpenTargetImodel); // Open the target version IModel const changesetId = targetVersion.changesetId; @@ -446,13 +446,13 @@ export class VersionCompareManager { IModelVersion.asOfChangeSet(changesetId), ); - this.progressCoordinator.updateProgress(ProgressStage.OpenTargetImodel, 100); + this.progressCoordinator.updateProgress(VersionCompareProgressStage.OpenTargetImodel, 100); // Keep metadata around for UI uses and other queries this.currentVersion = currentVersion; this.targetVersion = targetVersion; - this.progressCoordinator.updateProgress(ProgressStage.InitComparison); + this.progressCoordinator.updateProgress(VersionCompareProgressStage.InitComparison); let wantedModelClasses = [ GeometricModel2dState.classFullName, @@ -466,7 +466,7 @@ export class VersionCompareManager { filteredChangedElements = this._filterIgnoredElementsFromChangesets(changedElements); } - this.progressCoordinator.updateProgress(ProgressStage.InitComparison, 100); //@naron: is the following initialize also part of init comparison? if so, we need to maintain hierarchy of progress stages + this.progressCoordinator.updateProgress(VersionCompareProgressStage.InitComparison, 100); //@naron: is the following initialize also part of init comparison? if so, we need to maintain hierarchy of progress stages await this.changedElementsManager.initialize( this._currentIModel, diff --git a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx index 79b629ff..67950405 100644 --- a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx +++ b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx @@ -182,9 +182,9 @@ export class ChangedElementsWidget extends Component { - const message = `${msg} progress: ${pct}%`; - console.log(message); + private _onOverallProgress = (pct: number): void => { + const message = `Loading results: ${pct}%`; + console.log(message); //@naron: delete this this.setState({ message, loading: true, description: "" }); } @@ -234,7 +234,7 @@ export class ChangedElementsWidget extends Component; - private progress: Record; - private stageOrder: ProgressStage[]; // added to prevent race condition when updating progress - public readonly onProgressChanged = new BeEvent<(pct: number, message: string) => void>(); +export class ProgressCoordinator{ + private progress: Array<{ stage: StageType; currentProgress: number}>; + private weights: Record; + public readonly onProgressChanged = new BeEvent<(pct: number) => void>(); - constructor(weights: Record, stageOrder: ProgressStage[]) { - this.weights = weights; - this.stageOrder = stageOrder; - // init every stage as 0 - this.progress = Object.fromEntries( - Object.keys(weights).map(stage => [stage, 0]) - ) as Record; - } + constructor(progressStages: ReadonlyArray<{ stage: StageType; weight: number}>) { + this.progress = progressStages.map((stage) => ({ + stage: stage.stage, + currentProgress: 0, + })); - public getStageMessage(): string { - // const prefix = "VersionCompare:versionCompare."; - // return IModelApp.localization.getLocalizedString(`${prefix}${stage}`); //@naron: change this to loading - return "loading comparison" + this.weights = Object.fromEntries( + progressStages.map((stage) => [stage.stage, stage.weight]) + ) as Record; } /** @@ -40,14 +25,8 @@ export class ProgressCoordinator{ * @param stage The stage to update. * @param progress The progress percentage for the specified stage (0-100). */ - public updateProgress(stage: ProgressStage, progress: number = 0): void { - this.progress[stage] = progress; - const overallPercentage = this.getOverallPercentage(); //@naron: there are race condition which this was returning 100% early - const cap = this.stageOrder - .slice(0, this.stageOrder.indexOf(stage) + 1) - .reduce((sum, s) => sum + this.weights[s], 0); - const msg = this.getStageMessage(); - this.onProgressChanged.raiseEvent(Math.min(cap, overallPercentage), msg); + public updateProgress(stage: StageType, pct: number = 0): void { + this.modifyProgress(stage, () => pct); } /** @@ -55,14 +34,8 @@ export class ProgressCoordinator{ * @param stage The stage to update. * @param progress The progress percentage for the specified stage (0-100). */ - public addProgress(stage: ProgressStage, progress: number): void { - this.progress[stage] += progress; - const overallPercentage = this.getOverallPercentage(); //@naron: break this part into a private function - const cap = this.stageOrder - .slice(0, this.stageOrder.indexOf(stage) + 1) - .reduce((sum, s) => sum + this.weights[s], 0); - const msg = this.getStageMessage(); - this.onProgressChanged.raiseEvent(Math.min(cap, overallPercentage), msg); + public addProgress(stage: StageType, pct: number): void { + this.modifyProgress(stage, curr => curr + pct); } /** @@ -72,9 +45,25 @@ export class ProgressCoordinator{ * @returns A number (0-100) representing the global progress. */ public getOverallPercentage(): number { - return Object.entries(this.weights).reduce((sum, [stage, weight]) => { - const p = this.progress[stage as ProgressStage] ?? 0; - return sum + (p / 100) * weight; - }, 0); + return this.progress.reduce( + (sum, { stage, currentProgress }) => + sum + (currentProgress / 100) * this.weights[stage], + 0, + ); + } + + // helper that updates the progress and raises the event + private modifyProgress(stage: StageType, mutator: (curr: number) => number): void { + const idx = this.progress.findIndex(s => s.stage === stage); + if (idx === -1) return; + + this.progress[idx].currentProgress = mutator(this.progress[idx].currentProgress); + + const overallPct = this.getOverallPercentage(); + const cap = this.progress + .slice(0, idx + 1) // we only want to sum the weights of the stages up to and including the current one to prevent race conditions + .reduce((sum, s) => sum + this.weights[s.stage], 0); + + this.onProgressChanged.raiseEvent(Math.min(cap, overallPct)); } } From 467e01c64315852c98d1401ff096818767b1a70c Mon Sep 17 00:00:00 2001 From: naronchen Date: Thu, 8 May 2025 09:30:50 -0400 Subject: [PATCH 05/11] cleanup --- .../src/NamedVersionSelector/NamedVersionSelector.tsx | 1 - .../src/api/ChangedElementDataCache.ts | 2 +- .../src/api/ChangedElementEntryCache.ts | 8 ++++---- .../src/api/ChangedElementsManager.ts | 3 +-- packages/changed-elements-react/src/api/ElementQueries.ts | 2 +- .../src/api/VersionCompareManager.ts | 2 +- .../src/widgets/ChangedElementsWidget.tsx | 6 +++--- 7 files changed, 11 insertions(+), 13 deletions(-) diff --git a/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx b/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx index 2f007d84..c567e161 100644 --- a/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx +++ b/packages/changed-elements-react/src/NamedVersionSelector/NamedVersionSelector.tsx @@ -80,7 +80,6 @@ export function NamedVersionSelectorWidget(props: Readonly cleanup.forEach((cb) => cb()); }, diff --git a/packages/changed-elements-react/src/api/ChangedElementDataCache.ts b/packages/changed-elements-react/src/api/ChangedElementDataCache.ts index 2e653b2e..6d52e8b5 100644 --- a/packages/changed-elements-react/src/api/ChangedElementDataCache.ts +++ b/packages/changed-elements-react/src/api/ChangedElementDataCache.ts @@ -111,7 +111,7 @@ export abstract class ChangedElementDataCache { return []; // If everything has been cached already, populate the data and return if (this._containedInCache(elements)) { - return this._populateEntries(elements); //@naron: loading also doesnt get here for openPlant3 + return this._populateEntries(elements); } // Split the entries into the ones we have already cached and the ones we haven't diff --git a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts index 0bfd44ef..86a5cd91 100644 --- a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts +++ b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts @@ -82,7 +82,7 @@ export class ChangedElementEntryCache { } private _currentIModel: IModelConnection | undefined; private _targetIModel: IModelConnection | undefined; - private _progressCoordinator: ProgressCoordinator | undefined; + private _progressCoordinator?: ProgressCoordinator; private _progressLoadingEvent?: BeEvent<(message: string) => void>; private _currentLoadingMessage = ""; private _numSteps = 0; @@ -608,7 +608,7 @@ export class ChangedElementEntryCache { (currentEntryIds.length + targetEntryIds.length) / this._findTopParentChunkSize + 1; - this._setCurrentLoadingMessage("msg_findingParents", numTopParentQueries); //@naron: should we remove occurences for this. I left it for startComparisonV1 + this._setCurrentLoadingMessage("msg_findingParents", numTopParentQueries); this._progressCoordinator?.updateProgress(VersionCompareProgressStage.FindParents); const currentTopParents = await this._findTopParents( this._currentIModel, @@ -660,7 +660,7 @@ export class ChangedElementEntryCache { ? unchangedCurrentTopParents : currentTopParents, this._queryEntryChunkSize, - (pct) => this._progressCoordinator?.addProgress(VersionCompareProgressStage.ObtainElementData, pct/2), // @naron: hardcoded /2 cuz we call queryEntryDataBulk twice + (pct) => this._progressCoordinator?.addProgress(VersionCompareProgressStage.ObtainElementData, pct/2), ); const targetParentEntries = await queryEntryDataBulk( this._targetIModel, @@ -765,7 +765,7 @@ export class ChangedElementEntryCache { await this._uiDataProvider.loadChangedModelNodes( this._currentIModel, this._targetIModel, - () => this._progressCoordinator?.addProgress(VersionCompareProgressStage.LoadIModelNodes, 25), // @naron: hardcoded since updateFunc gets called 4 times + () => this._progressCoordinator?.addProgress(VersionCompareProgressStage.LoadIModelNodes, 25), ); this._progressCoordinator?.updateProgress(VersionCompareProgressStage.LoadIModelNodes, 100); diff --git a/packages/changed-elements-react/src/api/ChangedElementsManager.ts b/packages/changed-elements-react/src/api/ChangedElementsManager.ts index ef3d4eff..ec37cd3a 100644 --- a/packages/changed-elements-react/src/api/ChangedElementsManager.ts +++ b/packages/changed-elements-react/src/api/ChangedElementsManager.ts @@ -600,7 +600,6 @@ export class ChangedElementsManager { steps, ); - // @naron: not sure how to trigger it here, its always has the model ids in the data already and return early progressCoordinator?.updateProgress( VersionCompareProgressStage.ComputeChangedModels, Math.floor(((lastStep ?? 0) + currentStep) / (steps === 0 ? 1 : steps) * 100), @@ -1162,7 +1161,7 @@ export class ChangedElementsManager { wantedModelClasses?: string[], forward?: boolean, filterSpatial?: boolean, - progressCoordinator?: ProgressCoordinator, //@naron: can i just make it requried? + progressCoordinator?: ProgressCoordinator, progressLoadingEvent?: BeEvent<(message: string) => void>, ): Promise { this._progressLoadingEvent = progressLoadingEvent; diff --git a/packages/changed-elements-react/src/api/ElementQueries.ts b/packages/changed-elements-react/src/api/ElementQueries.ts index 62df5d11..57a2ba15 100644 --- a/packages/changed-elements-react/src/api/ElementQueries.ts +++ b/packages/changed-elements-react/src/api/ElementQueries.ts @@ -131,7 +131,7 @@ export const queryEntryDataBulk = async ( updateFunc?: (pct: number) => void, ): Promise => { if (elementIds.length < chunkSize) { - return queryEntryData(iModel, elementIds); //@naron: not able to test on openPlant, always gets retuned here | change the chunk size to test + return queryEntryData(iModel, elementIds); } const final: ChangedElementQueryData[] = []; diff --git a/packages/changed-elements-react/src/api/VersionCompareManager.ts b/packages/changed-elements-react/src/api/VersionCompareManager.ts index fb37ebf3..27f6fe8f 100644 --- a/packages/changed-elements-react/src/api/VersionCompareManager.ts +++ b/packages/changed-elements-react/src/api/VersionCompareManager.ts @@ -466,7 +466,7 @@ export class VersionCompareManager { filteredChangedElements = this._filterIgnoredElementsFromChangesets(changedElements); } - this.progressCoordinator.updateProgress(VersionCompareProgressStage.InitComparison, 100); //@naron: is the following initialize also part of init comparison? if so, we need to maintain hierarchy of progress stages + this.progressCoordinator.updateProgress(VersionCompareProgressStage.InitComparison, 100); await this.changedElementsManager.initialize( this._currentIModel, diff --git a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx index 67950405..ad1bee70 100644 --- a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx +++ b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx @@ -142,7 +142,7 @@ export class ChangedElementsWidget extends Component Date: Thu, 8 May 2025 09:37:50 -0400 Subject: [PATCH 06/11] lint --- .../src/widgets/ChangedElementsWidget.tsx | 2 +- .../src/widgets/ProgressCoordinator.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx index ad1bee70..c2dd8b8e 100644 --- a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx +++ b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx @@ -184,7 +184,7 @@ export class ChangedElementsWidget extends Component { const message = `Loading results: ${pct}%`; - console.log(message); //@naron: delete this + // console.log(message); //@naron: delete this this.setState({ message, loading: true, description: "" }); } diff --git a/packages/changed-elements-react/src/widgets/ProgressCoordinator.ts b/packages/changed-elements-react/src/widgets/ProgressCoordinator.ts index 625e35c9..7889a3c7 100644 --- a/packages/changed-elements-react/src/widgets/ProgressCoordinator.ts +++ b/packages/changed-elements-react/src/widgets/ProgressCoordinator.ts @@ -5,18 +5,18 @@ import { BeEvent } from "@itwin/core-bentley"; export class ProgressCoordinator{ - private progress: Array<{ stage: StageType; currentProgress: number}>; + private progress: Array<{ stage: StageType; currentProgress: number;}>; private weights: Record; public readonly onProgressChanged = new BeEvent<(pct: number) => void>(); - constructor(progressStages: ReadonlyArray<{ stage: StageType; weight: number}>) { + constructor(progressStages: ReadonlyArray<{ stage: StageType; weight: number;}>) { this.progress = progressStages.map((stage) => ({ stage: stage.stage, currentProgress: 0, })); this.weights = Object.fromEntries( - progressStages.map((stage) => [stage.stage, stage.weight]) + progressStages.map((stage) => [stage.stage, stage.weight]), ) as Record; } From 7d532343e3878dd1a1947b99a7f1c08ef69b95e8 Mon Sep 17 00:00:00 2001 From: naronchen Date: Mon, 12 May 2025 12:35:42 -0400 Subject: [PATCH 07/11] resolve comments --- .../public/locales/en/VersionCompare.json | 1 + .../src/api/ChangedElementEntryCache.ts | 13 ++- .../src/api/ElementQueries.ts | 6 +- .../src/api/VersionCompareManager.ts | 23 ++--- .../src/tests/ProgressCoordinator.test.ts | 91 +++++++++++++++++++ .../src/widgets/ChangedElementsWidget.tsx | 7 +- .../src/widgets/ProgressCoordinator.ts | 18 ++-- 7 files changed, 129 insertions(+), 30 deletions(-) create mode 100644 packages/changed-elements-react/src/tests/ProgressCoordinator.test.ts diff --git a/packages/changed-elements-react/public/locales/en/VersionCompare.json b/packages/changed-elements-react/public/locales/en/VersionCompare.json index f053e0a2..f53c8453 100644 --- a/packages/changed-elements-react/public/locales/en/VersionCompare.json +++ b/packages/changed-elements-react/public/locales/en/VersionCompare.json @@ -25,6 +25,7 @@ "loadingComparison": "Loading version comparison", "loadingNamedVersions": "Loading named versions", "comparisonNotActive": "No version comparison active.", + "LoadingResults": "Loading results: {{percent}}%", "cantHideDuringContext": "Cannot show/hide unchanged while emphasizing/isolating/hiding. Clear it first.", "comparisonGetStarted": "Click on the \"+\" comparison button to get started.", "showAll": "Show All", diff --git a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts index 86a5cd91..cb7e2be6 100644 --- a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts +++ b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts @@ -654,13 +654,20 @@ export class ChangedElementEntryCache { this._setCurrentLoadingMessage("msg_obtainingElementData", numEntryQueries); this._progressCoordinator?.updateProgress(VersionCompareProgressStage.ObtainElementData); + const OnObtainDataProgress = (pct: number) => { + this._progressCoordinator?.addProgress( + VersionCompareProgressStage.ObtainElementData, + pct / 2, // hardcoded to 50% cuz once called on currentIModel and once on targetIModel + ); + } + const currentParentEntries = await queryEntryDataBulk( this._currentIModel, VersionCompare.manager?.wantFastParentLoad ? unchangedCurrentTopParents : currentTopParents, this._queryEntryChunkSize, - (pct) => this._progressCoordinator?.addProgress(VersionCompareProgressStage.ObtainElementData, pct/2), + OnObtainDataProgress, ); const targetParentEntries = await queryEntryDataBulk( this._targetIModel, @@ -668,7 +675,7 @@ export class ChangedElementEntryCache { ? unchangedTargetTopParents : targetTopParents, this._queryEntryChunkSize, - (pct) => this._progressCoordinator?.addProgress(VersionCompareProgressStage.ObtainElementData, pct/2), + OnObtainDataProgress, ); // Put all data into arrays @@ -765,7 +772,7 @@ export class ChangedElementEntryCache { await this._uiDataProvider.loadChangedModelNodes( this._currentIModel, this._targetIModel, - () => this._progressCoordinator?.addProgress(VersionCompareProgressStage.LoadIModelNodes, 25), + () => this._progressCoordinator?.addProgress(VersionCompareProgressStage.LoadIModelNodes, 25), // hardcoded 25% as its called 4 times in load ); this._progressCoordinator?.updateProgress(VersionCompareProgressStage.LoadIModelNodes, 100); diff --git a/packages/changed-elements-react/src/api/ElementQueries.ts b/packages/changed-elements-react/src/api/ElementQueries.ts index 57a2ba15..1bba9e10 100644 --- a/packages/changed-elements-react/src/api/ElementQueries.ts +++ b/packages/changed-elements-react/src/api/ElementQueries.ts @@ -121,7 +121,7 @@ export const queryEntryData = async ( * @param iModel IModel to query * @param elementIds Ids of element to query for * @param chunkSize Chunk size for each query. Defaults to 1000 - * @param updateFunc [optional] called each time we process a chunk + * @param updateFunc [optional] called after each processed chunk with cumulative percent complete (0–100) * @returns Array of query data */ export const queryEntryDataBulk = async ( @@ -142,7 +142,9 @@ export const queryEntryDataBulk = async ( ); final.push(...data); if (updateFunc) { - updateFunc(Math.floor(i / elementIds.length * 100)); + const processed = Math.min(i + chunkSize, elementIds.length); + const pct = Math.floor((processed / elementIds.length) * 100); + updateFunc(pct); } } return final; diff --git a/packages/changed-elements-react/src/api/VersionCompareManager.ts b/packages/changed-elements-react/src/api/VersionCompareManager.ts index 27f6fe8f..a45f8a81 100644 --- a/packages/changed-elements-react/src/api/VersionCompareManager.ts +++ b/packages/changed-elements-react/src/api/VersionCompareManager.ts @@ -22,6 +22,7 @@ import { ProgressCoordinator } from "../widgets/ProgressCoordinator.js"; const LOGGER_CATEGORY = "Version-Compare"; +// different progress stages in version compare, also implies ordering of those stages export enum VersionCompareProgressStage { OpenTargetImodel, InitComparison, @@ -54,15 +55,15 @@ export class VersionCompareManager { private _hasParentIds = false; // define stage and order - private progressStages = [ - { stage: VersionCompareProgressStage.OpenTargetImodel, weight: 10 }, - { stage: VersionCompareProgressStage.InitComparison, weight: 10 }, - { stage: VersionCompareProgressStage.ComputeChangedModels,weight: 10 }, - { stage: VersionCompareProgressStage.FindParents, weight: 10 }, - { stage: VersionCompareProgressStage.ObtainElementData, weight: 10 }, - { stage: VersionCompareProgressStage.FindChildren, weight: 10 }, - { stage: VersionCompareProgressStage.LoadIModelNodes, weight: 40 }, - ] as const; + private weights: Record = { + [VersionCompareProgressStage.OpenTargetImodel]: 10, + [VersionCompareProgressStage.InitComparison]: 5, + [VersionCompareProgressStage.ComputeChangedModels]: 17, + [VersionCompareProgressStage.FindParents]: 17, + [VersionCompareProgressStage.ObtainElementData]: 17, + [VersionCompareProgressStage.FindChildren]: 17, + [VersionCompareProgressStage.LoadIModelNodes]: 17, + } /** Version Compare ITwinLocalization Namespace */ public static namespace = "VersionCompare"; @@ -84,7 +85,7 @@ export class VersionCompareManager { IModelApp.viewManager.addToolTipProvider(tooltipProvider); } - this.progressCoordinator = new ProgressCoordinator(this.progressStages); + this.progressCoordinator = new ProgressCoordinator(this.weights); } /** Create the proper visualization handler based on options */ @@ -148,7 +149,7 @@ export class VersionCompareManager { public versionCompareStopped = new BeEvent<() => void>(); /** Triggers during startup to show progress messages to any listener. */ - public loadingProgressEvent = new BeEvent<(message: string) => void>(); // @naron: should I remove this Or leave it for startComparisonV1? + public loadingProgressEvent = new BeEvent<(message: string) => void>(); /** Current Version for comparison. */ public currentVersion: NamedVersion | undefined; diff --git a/packages/changed-elements-react/src/tests/ProgressCoordinator.test.ts b/packages/changed-elements-react/src/tests/ProgressCoordinator.test.ts new file mode 100644 index 00000000..6826b038 --- /dev/null +++ b/packages/changed-elements-react/src/tests/ProgressCoordinator.test.ts @@ -0,0 +1,91 @@ +/*--------------------------------------------------------------------------------------------- +* Copyright (c) Bentley Systems, Incorporated. All rights reserved. +* See LICENSE.md in the project root for license terms and full copyright notice. +*--------------------------------------------------------------------------------------------*/ +import { describe, expect, it, vi, beforeEach } from "vitest"; +import { ProgressCoordinator } from "../widgets/ProgressCoordinator.js"; + +describe("ProgressCoordinator", () => { + const enum stages { + Stage1, + Stage2, + Stage3, + }; + + const weights: Record = { + [stages.Stage1]: 10, + [stages.Stage2]: 20, + [stages.Stage3]: 70, + }; + + let progressCoordinator: ProgressCoordinator; + let callback: ReturnType; + + beforeEach(() => { + progressCoordinator = new ProgressCoordinator(weights); + callback = vi.fn(); + progressCoordinator.onProgressChanged.addListener(callback); + }); + + it("starts as 0% and no events", () => { + expect(progressCoordinator.getOverallPercentage()).toBe(0); + expect(callback).not.toHaveBeenCalled(); + }); + + it("updatesProgress for a specific stage", () => { + progressCoordinator.updateProgress(stages.Stage1, 50); + expect(progressCoordinator.getOverallPercentage()).toBe(5); // 50% * 10% = 5% + expect(callback).toHaveBeenCalledWith(5); + }); + + it("addProgress accumulates progress for a specific stage", () => { + progressCoordinator.updateProgress(stages.Stage1, 50); + + progressCoordinator.addProgress(stages.Stage1, 50); + expect(progressCoordinator.getOverallPercentage()).toBe(10); // 10% * 10% = 10% + expect(callback).toHaveBeenCalledWith(5); + + progressCoordinator.addProgress(stages.Stage2, 75); + expect(progressCoordinator.getOverallPercentage()).toBe(25); //10% + 20% * 75% = 25% + expect(callback).toHaveBeenCalledWith(25); + }); + + it("ignores unknown stages in updateProgress or addProgress", () => { + progressCoordinator.updateProgress(100 as unknown as stages, 50); + expect(progressCoordinator.getOverallPercentage()).toBe(0); + expect(callback).not.toHaveBeenCalled(); + + progressCoordinator.addProgress(100 as unknown as stages, 50); + expect(progressCoordinator.getOverallPercentage()).toBe(0); + expect(callback).not.toHaveBeenCalled(); + }); + + it("when updateProgress or addProgress out of bounds, should be clamped to 100", () => { + progressCoordinator.updateProgress(stages.Stage1, 150); + expect(progressCoordinator.getOverallPercentage()).toBe(10); // 10% * 100% = 10% + expect(callback).toHaveBeenCalledWith(10); + + progressCoordinator.addProgress(stages.Stage2, 999); + expect(progressCoordinator.getOverallPercentage()).toBe(30); // 10% * 100% + 20% * 100% = 30% + expect(callback).toHaveBeenCalledWith(30); + + }); + + it("race condition, when multiple updates are called, the last one should be the one that is used", () => { + progressCoordinator.updateProgress(stages.Stage1, 50); + progressCoordinator.updateProgress(stages.Stage1, 75); + progressCoordinator.updateProgress(stages.Stage1, 60); + + expect(progressCoordinator.getOverallPercentage()).toBe(6); // 10% * 60% = 6% + expect(callback).toHaveBeenCalledWith(6); + + progressCoordinator.updateProgress(stages.Stage1, 100); + progressCoordinator.updateProgress(stages.Stage2, 100); + progressCoordinator.updateProgress(stages.Stage3, 100); + progressCoordinator.updateProgress(stages.Stage1, 50); + + expect(callback).toHaveBeenCalledWith(5); + expect(progressCoordinator.getOverallPercentage()).toBe(95); // 10% * 50% + 20% * 100% + 70% * 100% = 95% + }); + +}) diff --git a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx index c2dd8b8e..494e9056 100644 --- a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx +++ b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx @@ -182,9 +182,8 @@ export class ChangedElementsWidget extends Component { - const message = `Loading results: ${pct}%`; - // console.log(message); //@naron: delete this + private _onOverallProgress = (percent: number): void => { + const message = IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.LoadingResults", { percent: percent }); this.setState({ message, loading: true, description: "" }); } @@ -234,7 +233,7 @@ export class ChangedElementsWidget extends Component{ private weights: Record; public readonly onProgressChanged = new BeEvent<(pct: number) => void>(); - constructor(progressStages: ReadonlyArray<{ stage: StageType; weight: number;}>) { - this.progress = progressStages.map((stage) => ({ - stage: stage.stage, - currentProgress: 0, - })); + constructor(weights: Record) { + this.weights = weights; - this.weights = Object.fromEntries( - progressStages.map((stage) => [stage.stage, stage.weight]), - ) as Record; + this.progress = Object.keys(weights) + .map(key => Number(key) as StageType) + .sort((a, b) => a - b) + .map(stage => ({ stage, currentProgress: 0 })); } /** @@ -26,7 +24,7 @@ export class ProgressCoordinator{ * @param progress The progress percentage for the specified stage (0-100). */ public updateProgress(stage: StageType, pct: number = 0): void { - this.modifyProgress(stage, () => pct); + this.modifyProgress(stage, () => Math.min(100, Math.max(0, pct))); } /** @@ -35,7 +33,7 @@ export class ProgressCoordinator{ * @param progress The progress percentage for the specified stage (0-100). */ public addProgress(stage: StageType, pct: number): void { - this.modifyProgress(stage, curr => curr + pct); + this.modifyProgress(stage, curr => Math.min(100, Math.max(0, curr + pct))); } /** From 35bfefa1105a932f44ee01272a80a92802be1987 Mon Sep 17 00:00:00 2001 From: naronchen Date: Mon, 12 May 2025 16:34:56 -0400 Subject: [PATCH 08/11] push changeset --- .changeset/purple-seas-visit.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/purple-seas-visit.md diff --git a/.changeset/purple-seas-visit.md b/.changeset/purple-seas-visit.md new file mode 100644 index 00000000..17cdda7c --- /dev/null +++ b/.changeset/purple-seas-visit.md @@ -0,0 +1,5 @@ +--- +"@itwin/changed-elements-react": patch +--- + +- swap stage progress with overall loading progress during version comparison From 63dd0e34b28c0e9ed3a8e097ba5d0e39065d104e Mon Sep 17 00:00:00 2001 From: naronchen Date: Mon, 12 May 2025 16:39:50 -0400 Subject: [PATCH 09/11] lint fix --- .../src/tests/ProgressCoordinator.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/changed-elements-react/src/tests/ProgressCoordinator.test.ts b/packages/changed-elements-react/src/tests/ProgressCoordinator.test.ts index 6826b038..ec6105aa 100644 --- a/packages/changed-elements-react/src/tests/ProgressCoordinator.test.ts +++ b/packages/changed-elements-react/src/tests/ProgressCoordinator.test.ts @@ -10,13 +10,13 @@ describe("ProgressCoordinator", () => { Stage1, Stage2, Stage3, - }; + } const weights: Record = { [stages.Stage1]: 10, [stages.Stage2]: 20, [stages.Stage3]: 70, - }; + } let progressCoordinator: ProgressCoordinator; let callback: ReturnType; From e296ea8420d87bef9f314495df0f0d3166915474 Mon Sep 17 00:00:00 2001 From: naronchen Date: Tue, 13 May 2025 14:10:27 -0400 Subject: [PATCH 10/11] call on ProgressEvent --- .../src/api/ChangedElementDataCache.ts | 2 +- .../src/api/ChangedElementEntryCache.ts | 12 +++++++----- .../changed-elements-react/src/api/ElementQueries.ts | 6 +++--- .../src/widgets/ChangedElementsWidget.tsx | 8 ++++++-- .../src/widgets/ProgressCoordinator.ts | 10 +++++----- 5 files changed, 22 insertions(+), 16 deletions(-) diff --git a/packages/changed-elements-react/src/api/ChangedElementDataCache.ts b/packages/changed-elements-react/src/api/ChangedElementDataCache.ts index 6d52e8b5..02701bc5 100644 --- a/packages/changed-elements-react/src/api/ChangedElementDataCache.ts +++ b/packages/changed-elements-react/src/api/ChangedElementDataCache.ts @@ -20,7 +20,7 @@ export abstract class ChangedElementDataCache { /** * Called whenever a request is done in bulk mode, useful for UI update of progress */ - public updateFunction?: (pct: number) => void; + public updateFunction?: (percent: number) => void; constructor(protected _chunkSize: number = 500) { // No-op diff --git a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts index cb7e2be6..158c820f 100644 --- a/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts +++ b/packages/changed-elements-react/src/api/ChangedElementEntryCache.ts @@ -654,10 +654,10 @@ export class ChangedElementEntryCache { this._setCurrentLoadingMessage("msg_obtainingElementData", numEntryQueries); this._progressCoordinator?.updateProgress(VersionCompareProgressStage.ObtainElementData); - const OnObtainDataProgress = (pct: number) => { + const OnObtainDataProgress = (percent: number) => { this._progressCoordinator?.addProgress( VersionCompareProgressStage.ObtainElementData, - pct / 2, // hardcoded to 50% cuz once called on currentIModel and once on targetIModel + percent / 2, // hardcoded to 50% cuz once called on currentIModel and once on targetIModel ); } @@ -724,8 +724,8 @@ export class ChangedElementEntryCache { // Load child elements of the root nodes if we are not using fast parent loading if (this._childrenCache && !VersionCompare.manager?.wantFastParentLoad) { // Set update function for UI updates - this._childrenCache.updateFunction = (pct: number) => { - this._progressCoordinator?.addProgress(VersionCompareProgressStage.FindChildren, pct); + this._childrenCache.updateFunction = (percent: number) => { + this._progressCoordinator?.addProgress(VersionCompareProgressStage.FindChildren, percent); } const numQueries = this._childrenCache.calculateNumberOfRequests( parentEntries.length, @@ -768,11 +768,13 @@ export class ChangedElementEntryCache { this._progressCoordinator?.updateProgress(VersionCompareProgressStage.LoadIModelNodes); + const model_nodes_increment = 25; // 4 steps progress uin load changed model nodes + this._uiDataProvider = new ChangesTreeDataProvider(this._manager); await this._uiDataProvider.loadChangedModelNodes( this._currentIModel, this._targetIModel, - () => this._progressCoordinator?.addProgress(VersionCompareProgressStage.LoadIModelNodes, 25), // hardcoded 25% as its called 4 times in load + () => this._progressCoordinator?.addProgress(VersionCompareProgressStage.LoadIModelNodes, model_nodes_increment), ); this._progressCoordinator?.updateProgress(VersionCompareProgressStage.LoadIModelNodes, 100); diff --git a/packages/changed-elements-react/src/api/ElementQueries.ts b/packages/changed-elements-react/src/api/ElementQueries.ts index 1bba9e10..1c5f61ba 100644 --- a/packages/changed-elements-react/src/api/ElementQueries.ts +++ b/packages/changed-elements-react/src/api/ElementQueries.ts @@ -128,7 +128,7 @@ export const queryEntryDataBulk = async ( iModel: IModelConnection, elementIds: string[], chunkSize = 1000, - updateFunc?: (pct: number) => void, + updateFunc?: (percent: number) => void, ): Promise => { if (elementIds.length < chunkSize) { return queryEntryData(iModel, elementIds); @@ -143,8 +143,8 @@ export const queryEntryDataBulk = async ( final.push(...data); if (updateFunc) { const processed = Math.min(i + chunkSize, elementIds.length); - const pct = Math.floor((processed / elementIds.length) * 100); - updateFunc(pct); + const percent = Math.floor((processed / elementIds.length) * 100); + updateFunc(percent); } } return final; diff --git a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx index 494e9056..7d5b0ed8 100644 --- a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx +++ b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx @@ -183,8 +183,12 @@ export class ChangedElementsWidget extends Component { - const message = IModelApp.localization.getLocalizedString("VersionCompare:versionCompare.LoadingResults", { percent: percent }); - this.setState({ message, loading: true, description: "" }); + this._onProgressEvent( + IModelApp.localization.getLocalizedString( + "VersionCompare:versionCompare.LoadingResults", + { percent } + ) + ); } private _refreshCheckboxesEvent = new BeEvent<() => void>(); diff --git a/packages/changed-elements-react/src/widgets/ProgressCoordinator.ts b/packages/changed-elements-react/src/widgets/ProgressCoordinator.ts index 1d81a043..6e2ecc3a 100644 --- a/packages/changed-elements-react/src/widgets/ProgressCoordinator.ts +++ b/packages/changed-elements-react/src/widgets/ProgressCoordinator.ts @@ -7,7 +7,7 @@ import { BeEvent } from "@itwin/core-bentley"; export class ProgressCoordinator{ private progress: Array<{ stage: StageType; currentProgress: number;}>; private weights: Record; - public readonly onProgressChanged = new BeEvent<(pct: number) => void>(); + public readonly onProgressChanged = new BeEvent<(percent: number) => void>(); constructor(weights: Record) { this.weights = weights; @@ -23,8 +23,8 @@ export class ProgressCoordinator{ * @param stage The stage to update. * @param progress The progress percentage for the specified stage (0-100). */ - public updateProgress(stage: StageType, pct: number = 0): void { - this.modifyProgress(stage, () => Math.min(100, Math.max(0, pct))); + public updateProgress(stage: StageType, percent: number = 0): void { + this.modifyProgress(stage, () => Math.min(100, Math.max(0, percent))); } /** @@ -32,8 +32,8 @@ export class ProgressCoordinator{ * @param stage The stage to update. * @param progress The progress percentage for the specified stage (0-100). */ - public addProgress(stage: StageType, pct: number): void { - this.modifyProgress(stage, curr => Math.min(100, Math.max(0, curr + pct))); + public addProgress(stage: StageType, percent: number): void { + this.modifyProgress(stage, curr => Math.min(100, Math.max(0, curr + percent))); } /** From e293554508843d7261aa74a2c565465fd49d3b05 Mon Sep 17 00:00:00 2001 From: naronchen Date: Tue, 13 May 2025 14:26:33 -0400 Subject: [PATCH 11/11] lint --- .../src/widgets/ChangedElementsWidget.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx index 7d5b0ed8..e5b418ad 100644 --- a/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx +++ b/packages/changed-elements-react/src/widgets/ChangedElementsWidget.tsx @@ -186,8 +186,8 @@ export class ChangedElementsWidget extends Component