Pass EditorWidgetFactory options to TextEditorProvider

[colin-grant-work]

What it does

This PR fixes #9746 by giving DiffEditors knowledge of the options used to create them. This allows them to set the correct value for alwaysRevealFirst in the options passed to the DiffNavigator. It may also make it possible to fix some of the lifecycle difficulties we previously had with EditorManager.revealSelection in a manner more consistent with VSCode's / Monaco's approach.

How to test

1. Please follow the steps outlined in Diff Editor always puts cursor on last changed line when opened #9746.
2. You should observe that, when you open the diff editor for the first time, the correct selection is revealed and the editor does not scroll away to the next change in the file.

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

Signed-off-by: Colin Grant colin.grant@ericsson.com

[colin-grant-work]

I want to add some caveats / discussion points here:

• The bug is caused by a quirk in how VSCode treats one setting (alwaysRevealFirst) for diff editors, and it may be reasonable to conclude that VSCode's behavior is a bug, so it may not be ours to fix.
• This solution involves some dubious work to get around the implementation of the WidgetManager, which probably isn't safe since callers shouldn't know exactly how the WidgetManager is implemented. In particular, callers shouldn't know that the WidgetManager uses JSON.stringify to generate what it considers the definitive ID of a widget.
• The fact that I want to get around certain aspects of how the WidgetManager is implemented may indicate that we've conflated two things that ought to be kept distinct: the options that identify a widget 'sufficiently' for some purpose and the options that might be useful in case the WidgetManager creates a new widget rather than returning an old one. Specifically, for the EditorManager, if an editor with a given set of {type, uri, counter} options already exists, that editor is fine to pass to any caller who asks for those options. However, if such an editor doesn't exist, it may be useful to pass extra information to the EditorWidgetFactory, but at the moment, we can't do that legitimately: the WidgetManager implementation implies that if the options are different (or stringify differently), a new widget should be created, and no distinction is made between taking one 'off the shelf' and creating a new one. It would probably be good if there weren't situations where those things diverged - i.e. where creating a new widget with the minimum identifying information was wrong - but we can't always guarantee that if we don't have total control of the widget's initialization routine (as we don't control the Monaco classes).

[colin-grant-work]

This line is the actual fix. It needs to be known when the DiffNavigator is created because it's supposed to be private and inaccessible thereafter.

[msujew]

I can confirm that the issue exists on master and is fixed by this change:

• Using the testing steps provided, the diff view is opened with the first change select ✔️
• Other diff views (scm/compare opened editors) continue working as expected ✔️

This solution involves some dubious work to get around the implementation of the WidgetManager

The code looks alright to me. I don't see a way around that right now.

Using the Create file-copy command I noticed that the copied file is opened twice in the same area. Though this is not related to this change and also happens on master.

[colin-grant-work]

Closing for now. It does fix the problem, and may be a reasonable solution, but only if the underlying changes solve more problems than just this one.