Fix #12546: UI not updated upon editing breakpoint condition

[pisv]

What it does

Fixes #12546, which is a regression introduced by #12183, by ensuring that BreakpointManager.setMarkers fires a SourceBreakpointsChangeEvent when oldMarker === newMarker, as there is no way to actually detect a change in this case.

How to test

• Verify that UI not updated upon editing breakpoint condition #12546 is actually fixed by following the steps given in the issue description.

• Also verify that Multiple DebugEditorModels for same URI lead to stack overflow #11878, which was fixed by Fire breakpoint events only if breakpoint metadata changes #12183, is still fixed after this patch is applied, and there are no other regressions in breakpoint management in general.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[tsmaeder]

I don't know if this is the right fix: if we look at DebugSourceBreakpoint.updateOrigins(), we see that the breakpoint objects are updated in place. But that does not make sense if we have a model where we have change detection in the BreakpointManager. Shouldn't we try and clean up the update process so that the change detection works correctly?

[pisv]

If you ask me, I think it is the right fix, as it fixes the root issue.

The contract of the MarkerManager.setMarkers method, which the BreakpointManager overrides, states:

replaces the current markers for the given uri and owner with the given data

There is no hint in the contract that newMarkers must be new objects, i.e. a newMarker cannot be === to the corresponding oldMarker.

Obviously, if newMaker === oldMarker a change cannot be detected, and we cannot help but assume that the object might have been changed.

Also, I'd like to note that besides existing code in Theia itself, like DebugSourceBreakpoint.updateOrigins() and others, there might be external clients that have come to depend on the long-existing contract and behaviour of the setMarkers method, which was essentially broken by #12183.

[tsmaeder]

The contract of the MarkerManager.setMarkers method, which the BreakpointManager overrides, states:

replaces the current markers for the given uri and owner with the given data

Yes, but isn't DebugSourceBreakpoint.updateOrigins() violating that contract by reaching into the internal structure of the SourceBreakpoints? To me it seems like there is no clear separation of responsibility here. Since SourceBreakpoint has not change notification facility, some other instance has to take care of appropriately sending out change notification. Which component owns the SourceBreakpoint.raw field?

[pisv]

Perhaps, but I would not consider it as directly relevant to the proposed patch.

Basically, as a Theia user, I would just like the issue #12546, which was reported back in May, to be fixed.

Since it has not been fixed so far, I proposed a patch after due consideration of the underlying issue, which does fix a major part of the breakage introduced by #12183, which caused the issue in the first place.

Given that, I'd like to leave any further optimisations in the design or implementation of the related code, which are of course possible, to the esteemed committers or other contributors.

It seems fair, doesn't it?

[pisv]

OK. The change to BreakpointManager.setMarkers from #12183 would make sense in its current form if and only if all the instances of the SourceBreakpoint were (deeply) immutable. That probably means that all of the instances of any subclasses of the BaseBreakpoint would need to be made immutable too, and also the ExceptionBreakpoint, for the sake of the design symmetry. Of course, it would be an even more radical breaking change. But it does look like a better design.

I could try to make the change myself, but since it would be a big breaking change and a significant amount of work, I'd like to be sure upfront that we are really on the same page here, and such a change could actually be accepted.

[tsmaeder]

So I guess we'll have to live with your fix for the moment, even though, IMO, it's a workaround for the general bogusness surrounding this topic.

[tsmaeder]

I've filed #13052

[tsmaeder]

@pisv could you rebase and push to trigger running a the tests against the latest version of master?

[pisv]

So I guess we'll have to live with your fix for the moment, even though, IMO, it's a workaround for the general bogusness surrounding this topic.

Thank you for your review and approval, Thomas.

I agree with you regarding "the general bogusness surrounding this topic" (to put it mildly).

I still think though that the only clean solution in the presence of change detection in BreakpointManager.setMarkers would be to make the SourceBreakpoint immutable. This would allow us to keep the general contract of MarkerManager.setMarkers and avoid any unexpected behavior. Even without the change detection, it would be a better design IMO.

Having said that, I can understand that such a breaking change could be unacceptable.

could you rebase and push to trigger running a the tests against the latest version of master?

I have rebased and pushed the branch, but it looks like an intermittent build failure has occurred now.