UI not updated upon editing breakpoint condition

[pisv]

Steps to Reproduce:

1. Using Theia Electron (or Browser) Example on the master branch,
   add a source breakpoint in a JavaScript (or TypeScript) file.

2. Edit the breakpoint and set a condition (Expression or Hit Count).

3. Note that UI (e.g. the breakpoint's icon) is not updated after the condition is set.

Observed	Expected

Additional Information

Debugging revealed that no SourceBreakpointsChangeEvent is fired from the BreakpointManager.setMarkers method upon setting breakpoint condition (step 2).

See also #8851.

[tsmaeder]

@pisv by "UI" you mean the hover message on the breakpoint?

[pisv]

I mean that UI state in general is not updated in response to the action (e.g. the breakpoint's icon). The crux of the matter is that SourceBreakpointsChangeEvent is not fired from the BreakpointManager.setMarkers in this case. I've tried to patch it locally and can confirm that UI state is properly updated when the event is fired.

[pisv]

It looks like a regression introduced by #12183.

When a condition is set for a breakpoint, the DebugSourceBreakpoint.updateOrigins method updates the breakpoint.raw object using the given data before calling this.breakpoints.setBreakpoints. Therefore, when the BreakpointManager.setMarkers method is called from setBreakpoints, the breakpoint will have already been updated, so that the newMarker is deeply equal to the oldMarker at this line:

theia/packages/debug/src/browser/breakpoint/breakpoint-manager.ts

Line 78 in 248f6b6

didChangeMarkers ||= !!added.length || !deepEqual(oldMarker, newMarker);

So, didChangeMarkers stays false, and no event is fired in this case.

Here is the relevant call stack:

setMarkers (theia/packages/debug/src/browser/breakpoint/breakpoint-manager.ts:78)
setBreakpoints (theia/packages/debug/src/browser/breakpoint/breakpoint-manager.ts:114)
updateOrigins (theia/packages/debug/src/browser/model/debug-source-breakpoint.tsx:82)
acceptBreakpoint (theia/packages/debug/src/browser/editor/debug-editor-model.ts:398)

[pisv]

I could try to provide a patch, but I'm not quite sure what would be the proper way to fix the issue.

One way would be to add an optional parameter to both setBreakpoints and setMarkers methods of BreakpointManager, which would force sending the event even if didChangeMarkers is false (but changed.length > 0).

In that way, both setEnabled and updateOrigins methods of DebugSourceBreakpoint would call this.breakpoints.setBreakpoints with the force parameter turned on.

I have tried this approach locally, and it seems to work fine.

But I'm not quite sure if it is a proper way.

What do you think?

[tsmaeder]

Hmh...a "force notification" flag sounds like a code smell. Whenever we have mutable data, we need change notification in order for users of the data to react to those changes. In our case, DebugSourceBreakpoint is mutable, but it has no change notification. The assumption in breakpoint-manager.ts seems to be that all mutation of breakpoints is done through setBreakpoints() with the breakpoints themselves being immutable. Unfortunately, the breakpoint-related code seems to be a bit messy, mixing UI concerns (markers, etc.) with the breakpoint data model.
IMO, there are only two clean ways to solve this: either we keep the breakpoint objects mutable and introduce change notification on them or we make the breakpoints immutable (outside of the breakpoint manager) and delegate all updates to breakpoints to the breakpoint manager (either via setBreakpoints() or via more specific methods to update individual breakpoints).

[pisv]

@tsmaeder Thank you for your detailed comment. I has been very helpful to me.

Thinking a bit more about it, it seems that the root of the issue is that the fix in #12183 introduced a significant change in behavior to the BreakpointManager.setMarkers API. Although it has not been formally documented as a written contract, the former behavior was to set the markers and notify unconditionally:

theia/packages/debug/src/browser/breakpoint/breakpoint-manager.ts

Lines 62 to 84 in f0cd180

	override setMarkers(uri: URI, owner: string, newMarkers: SourceBreakpoint[]): Marker<SourceBreakpoint>[] {
	const result = super.setMarkers(uri, owner, newMarkers);
	const added: SourceBreakpoint[] = [];
	const removed: SourceBreakpoint[] = [];
	const changed: SourceBreakpoint[] = [];
	const oldMarkers = new Map(result.map(({ data }) => [data.id, data] as [string, SourceBreakpoint]));
	const ids = new Set<string>();
	for (const newMarker of newMarkers) {
	ids.add(newMarker.id);
	if (oldMarkers.has(newMarker.id)) {
	changed.push(newMarker);
	} else {
	added.push(newMarker);
	}
	}
	for (const [id, data] of oldMarkers.entries()) {
	if (!ids.has(id)) {
	removed.push(data);
	}
	}
	this.onDidChangeBreakpointsEmitter.fire({ uri, added, removed, changed });
	return result;
	}

Evidently, some client code (such as setEnabled and updateOrigins methods of DebugSourceBreakpoint) had come to depend on this former behavior.

Therefore, I guess there are two ways to handle the underlying issue: either adjust the client code to the new behavior of the BreakpointManager.setMarkers API (but note that this might also include external client code that is beyond our control), or (given that the breakage occurred relatively recently) restore the former behavior by default and make the new behavior dependent on a new optional parameter of the method (which would have probably been a proper way to handle a significant behavior change to the long-existing API in the first place, besides introducing an entirely new API as an alternative).

Having said that, I must admit that I might not fully understand everything involved in the issue.