Do not clear the editor model when the editor is hidden

[pisv]

What it does

TL;DR This PR proposes to reconsider the change introduced by #14909.

So, #14909 has caused regressions all around Theia and probably even outside Theia (in Theia extensions, as opposed to VS Code extensions) because it significantly changed the way that hidden text editors had been behaving in Theia for years. And, unfortunately, in its nature it is also the worst kind of a breaking change possible, one that the compiler would never tell you about.

Setting the editor model to null when the editor is hidden detaches the model from the editor, which has the following effects (the list is not exhaustive):

• All decorations owned by the editor are removed from the model;

• All view zones, overlay widgets, etc. are removed;

• The monaco-editor node is removed from the DOM.

While the editor is hidden, calling TextEditor.deltaDecorations API has no effect now, since the model is detached from the editor. (I have not checked it yet, but it seems likely that other editor-related APIs in Theia might also behave differently in this state now.)

When the editor becomes visible again and the model is reattached to the editor, the monaco-editor node hierarchy is recreated from scratch and some editor components (e.g. overlay widgets) are recreated from the data stored in the model, while others such as decorations owned by the editor and view zones are not automatically recreated and would need to recreated manually now.

Of course, such a drastic change in well-established behavior has a vast potential for regressions, which has already materialised in practice: after two months and a couple of releases, their list is still growing.

This PR proposes to reconsider the change introduced by #14909. It reverts the effect of #14909 in a non-disruptive way, making as few changes as possible. It fixes #15426, #15496, #15497, #15498, and any other known or potential issues caused by #14909. The fix should be safe to apply, since any code that have already been adapted to the change in #14909 is expected to keep working, while any code that have not been adapted, and is currently broken, will function normally again. Of course, it does mean that #14880, which was fixed by #14909, will need to be fixed in some other way.

How to test

Verify that all known regressions from #14909 are now fixed, including those that have already been fixed by adapting the code to the change in #14909. To the best of my knowledge, the list of currently known regressions is:

• Chat Input is unusable #14924
• Editors are no longer extractable #15038
• Changing active editor deletes all breakpoints #15057
• Diff markers lost when diff editor hidden #15178
• Dirty-diff decorations lost when editor hidden #15426
• Peek view for conditional breakpoint looks broken after switching editor tabs #15496
• Debug exception widget looks broken after switching editor tabs #15497
• Debug decorations for current stack frame are lost after switching editor tabs #15498

There are also #15201 and #15234, which may be related, but have not been confirmed as regressions.

Follow-ups

Breaking changes

• This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

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]

@pisv so how do we fix the original issue #14880? It breaks basic code navigation in the editor. On a more general level: VS Code hides editors which are not shown, so we must assume that more issues like #14880 will arise in the future.
While we might be able to patch monaco so that it works when being hidden, we'd have to evolve such patches on every monaco uplift. I think tracking down the places in Theia where we make the assumption that editors are hidden on the dom level is the saner way going forward even if it causes some noise right now.

[pisv]

Personally, I've never encountered #14880 in practice, and from its description, it seems that it occurs only under some conditions on Linux. So, although I don't know how to fix it, I'm far more concerned about the havoc that #14909 is causing. I'd bet that not every regression even in Theia itself has been discovered yet, not speaking of external Theia extensions.

Regarding VS Code, it has an entirely different editor lifecycle than Theia. An editor instance in VS Code is reused if possible when switching editor tabs. It is just given another editor input with another model; i.e. it is not hidden, it is reinitialized from scratch. As far as I can tell, there is no way to replicate this behavior in Theia without a nuclear breaking change.

Having said that, it is entirely your choice. I've created this PR mostly as a place for discussion among Theia committers (and for the sake of convenience, in case it is accepted).

Basically, there are two ways forward:

• Find and fix every regression from Empty hidden editors #14909 both in Theia and in Theia adopters' extensions.

• Revert Empty hidden editors #14909 and try to find another way to fix Symbols in Monaco stop to be recognized and cannot be navigated to #14880.

[sdirix]

Thank you @pisv for this suggestion and for identifying the current issues with model clearing.

I agree with @tsmaeder that we should generally aim to match VS Code's behavior to avoid potential issues. However, this argument is less compelling if what we're implementing isn't actually what VS Code does as mentioned by @pisv.

#14880 is a major regression and was internally reported by many Theia IDE Dogfooders. It's very easy to experience during typical development. Whether it's worse than all the other issues combined? 🤷

I agree with your suggested approaches. If we can find a reliable simple fix for #14880 without clearing the editor models, then this would be the best way forward for now. We can then discuss at a later point whether we want to reintroduce editor hiding.

If we can't find a reliable simple fix for #14880 then I'm not sure whether trading issues is worth it. We would then either need to:

• Not only revert the editor model clearing but also the Monaco upgrade Update monaco-editor-core to 1.96.3 #14737 to fix all issues. Then, to not fall behind, we would need a branch with the Monaco upgrade + Editor model clearing + all fixes for it and then merge again later. This is a lot of effort.
• Only do editor model clearing on Linux if Symbols in Monaco stop to be recognized and cannot be navigated to #14880 is considered worse than the other issues. However this opens Pandora's Box with very different behaviors on different platforms. So I would rather avoid it
• Find and fix all issues with editor model clearing

[pisv]

Speaking from the perspective of a Theia adopter (Theia as a platform, and not just the Theia IDE), I'd like to add one point, which seems important to me. #14909 is a serious breaking change in well-established behavior. If the decision is made to keep the current behavior, and find and fix all regressions, those regressions may also be lurking in external Theia extensions (as opposed to VS Code extensions). Therefore, this breaking change will need to be communicated to Theia adopters as such, with a clear migration guide.

[pisv]

Just a thought regarding #14880. If something in Monaco really started to make assumptions about the specifics of the editor lifecycle in the VS Code workbench, then it looks like a bug that I'd say should be reported and fixed upstream. I would expect that by design, nothing in Monaco should depend on the specifics of its use in the VS Code workbench, including the editor lifecycle of the workbench. Otherwise, it would break proper layering and limit Monaco reusability outside VS Code.

[tsmaeder]

If something in Monaco really started to make assumptions about the specifics of the editor lifecycle in the VS Code workbench

The API of the ICodeEditor underlying our MonacoEditor implementation has had the option to set the text model (including null for a long time, we just chose to ignore this fact and never set the model on the underlying implementation. VS Code has been doing that for a long time, though. So we have not been programming to the API, we've been programming to the implementation. This is not a bug in VS Code, so why should they fix it?

[pisv]

I meant something entirely different. I was speaking about the underlying cause of the issue #14880, not about the specific fix in #14909. From your comment that "we might be able to patch monaco so that it works when being hidden", and also from @sdirix comment that "we would need to revert Monaco upgrade to fix it", I thought that something in Monaco started to make assumptions about the specifics of the editor lifecycle in the VS Code workbench, and that caused the issue in Theia. Does it make sense?