Add Dirty Diff Peek View

[pisv]

What it does

Closes #4544.

Allows the user to browse through dirty diff changes similarly to VS Code:

• Go to Next Change/Go to Previous Change (Alt+F5/Shift+Alt+F5) commands move the cursor to the next/previous change.

• Show Next Change/Show Previous Change (Alt+F3/Shift+Alt+F3) commands show a peek view with the inline diff of the next/previous change.

The peek view can also be opened by clicking on a dirty diff decoration in the gutter. Clicking on the same decoration again closes the peek view.

Basic actions for browsing through the changes and closing the peek view are provided by the peek view itself in its action bar. Other actions such as Revert Change and Stage Change can be contributed to the action bar by Theia extensions and VS Code plugins.

The implementation fully supports and has been tested with @theia/git:

and vscode.git:

How to test

Verify that the following steps work correctly regardless of whether @theia/git or vscode.git is used.

0. Open a file with unstaged changes. The editor should be focused.

1. Move the cursor to the beginning of the next/previous change by using Go to Next Change/Go to Previous Change (Alt+F5/Shift+Alt+F5) commands.

2. Browse through the changes by using Show Next Change/Show Previous Change (Alt+F3/Shift+Alt+F3) commands. Press ESC to close the peek view.

3. Click on a dirty diff decoration in the gutter to show the corresponding change. Click on the same decoration again to close the peek view.

4. Open the peek view again and verify that each of the actions in its action bar (browsing through the changes, closing the peek view, reverting the change, staging the change) works as expected.

Review checklist

• [x] 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 what am I missing here? Here's what I do:

1. I changed the electron example to use the VS Code git extension because of https://github.com/eclipse-theia/theia/issues/12651
2. I open Theia on the Theia source code itself
3. I open a *.tsx file and i add "asdf" to some line
4. save

At this point, I do not get a "dirty diff" annotation in the editor gutter and I have not "Go to next Change" command availlable in command palette. Do I need to enable something?

[pisv]

@tsmaeder Thank you for taking a look at it.

It is very strange that you don't even get dirty diff annotations in the editor gutter. Something seems to be missing indeed. It works for me locally using the following steps, which are based on your steps:

0. Check out the PR branch.

1. Modify the root package.json to remove "vscode.git" and "vscode.git-base" from "theiaPluginsExcludeIds". Modify examples/electron/package.json to remove "@theia/git" from "dependencies".

2. Clean-build and run Theia Electron Example:

   yarn clean
   yarn
   yarn download:plugins
   yarn electron build
   yarn electron start
   

3. Open the theia workspace.

4. Open a *.tsx file (or any other committed or staged source file for that matter) and modify it in any way (e.g. by adding "asdf" to some line, as you did). Saving the file should not be necessary, but it should not hurt either.

At this point, I get the proper dirty diff annotation(s) in the editor gutter and can browse through the changes using the Alt+F5/Shift+Alt+F5 or Alt+F3/Shift+Alt+F3 keyboard shortcuts (or with Go -> Next Change/Previous Change menu items).

However, I can confirm that the commands are not available in the command palette. The issue seems to be that the editor loses focus when the command palette is activated, and the commands are enabled only for the currently focused editor. I'll dig into it.

[tsmaeder]

Here's what I do after building the merge of your branch into master (as per "command line instructions") with the git/git base extensions enabled in the electron app.

https://github.com/eclipse-theia/theia/assets/13163770/ce937f02-5140-48e0-8b8e-ba5534256ace

In the last step, I would expect an annotation to appear where the breakpoints are in the editor.

[tsmaeder]

Maybe a Mac/Windows problem?

[tsmaeder]

Doing the same workflow on a mac works. The structure of the html looks very different in both cases: the "dirty-diff-glyph" div is missing from the line where the change is.

[pisv]

Doing the same workflow on a mac works. The structure of the html looks very different in both cases: "margin-view-overlays" seem to be completely absent in the Window case.

I see. Thank you very much for this information. It is clear now that the issue is platform-specific. I'll need some time to set up a Windows VM and debug the issue.

[pisv]

@tsmaeder I hope that the issues found so far are fixed now.

For the sake of convenience, I pushed separate commits, which are intended to be squashed when the review is finished.

[pisv]

Converted to draft, as the dirty diff peek view no longer works when this PR is rebased on the current master. Perhaps not surprisingly, given that #13217 was merged earlier today...

[tsmaeder]

Perhaps not surprisingly, given that https://github.com/eclipse-theia/theia/pull/13217 was merged earlier today...

Sorry about that. Let me know if you have questions about the update.

[pisv]

This PR has been rebased on the current master, updated to the new version of @theia/monaco-editor-core, and re-tested on macOS and Windows. It is ready for review again.

[tsmaeder]

When I hover over the little red triangle that signifies "removed line", I can get the decorations to keep flickering between the red bar and the triangle icon. Maybe there is an overlap of the decorations that gets the "hover" attribute to flip-flop between states?

[pisv]

When I hover over the little red triangle that signifies "removed line", I can get the decorations to keep flickering between the red bar and the triangle icon.

This issue is fixed now in the same way as it was fixed in VS Code about 6 years ago: https://github.com/microsoft/vscode/commit/00260009e8d9f3f6e7326bab3e09b1c932022e59.

Note that the issue can also be reproduced on the current master; it was not introduced by this patch.

[pisv]

Thanks for this very useful PR.

Thanks for reviewing this PR.

The main issue I have is the direct use of monaco API outside of the monaco package: this is making the task of updating to a new VS Code version more tedious. Not sure this is a direction we want to take.

Since this seems to be the main issue, let's try to address it first.

Here are my initial thoughts:

• The debug package already makes heavy use of the Monaco API, so there is already a precedent for the direct use of this API outside of the monaco package. There are other packages in Theia that also use the Monaco API directly outside of the monaco package, although not quite as heavily as debug.

• The use of the Monaco API by the scm package in this PR is fully encapsulated in the non-exported DirtyDiffPeekView class. This is in line with the first requirement set forth in the Using code from VS Code document: Updating Monaco should not impact adopters.

• Empirically, migration of the DirtyDiffPeekView class from @theia/[email protected] to the new 1.83.1 version took less than a couple of hours of work in this PR, although I'm not an expert in the Monaco API in any way. In other words, it seems to satisfy the other requirement in the above mentioned document: Adoption of a new Monaco version should generally be a straightforward, quick process.

• I don't quite understand why the direct use of Monaco API outside of the monaco package would per se make the task of updating to a new version of @theia/monaco-editor-core more tedious.

  Would moving some code from the scm package down to the monaco package make the migration actually easier? It seems that approximately the same amount of work would still be required, as approximately the same Monaco APIs would still need to be used to implement the same functionality.

Can you explain what exactly I'm missing here?

[tsmaeder]

@pisv I'll have time to come back to this one probably early next week.

[rayguncertified1]

Here's what I do after building the merge of your branch into master (as per "command line instructions") with the git/git base extensions enabled in the electron app.

2024-02-09.11-40-14.mp4 In the last step, I would expect an annotation to appear where the breakpoints are in the editor.

@Luzifer

[tsmaeder]

Thanks for this very useful PR.

Thanks for reviewing this PR.

The main issue I have is the direct use of monaco API outside of the monaco package: this is making the task of updating to a new VS Code version more tedious. Not sure this is a direction we want to take.

Since this seems to be the main issue, let's try to address it first.

Here are my initial thoughts:

• The debug package already makes heavy use of the Monaco API, so there is already a precedent for the direct use of this API outside of the monaco package. There are other packages in Theia that also use the Monaco API directly outside of the monaco package, although not quite as heavily as debug.

Yes, but that is not a good thing. we have to look at every usage and determine whether it needs updating in the case of a dependency update. VS Code API is internal API that changes without notice and we have to adapt to without any guidance.

• The use of the Monaco API by the scm package in this PR is fully encapsulated in the non-exported DirtyDiffPeekView class. This is in line with the first requirement set forth in the Using code from VS Code document: Updating Monaco should not impact adopters.

That is true, but you're also not making the `PeekView' functionality available for others clients to use. For example, the comments thread widget is using a monaco editor zone widget. While the API of the zone widget implementation is arguably bad (uses VS Code API in its API), at least other Theia modules could reuse some of that implementation. The "Using code from VS Code" document is aimed at mitigating a bad thing. Using VS Code couples our code to an internal API we don't control and forces us to spend time on tasks which are mostly unproductive. We need to get a lot of value out of the reuse to offset the cost, and we should strive to minimize the coupling as much as we can. There are a number of places where the coupling to monaco API is not necessary. With limited resources, my aim is to clean things up if we touch an area anyway.

• Empirically, migration of the DirtyDiffPeekView class from @theia/[email protected] to the new 1.83.1 version took less than a couple of hours of work in this PR, although I'm not an expert in the Monaco API in any way. In other words, it seems to satisfy the other requirement in the above mentioned document: Adoption of a new Monaco version should generally be a straightforward, quick process.

That is true, and it's also true of the other 27 places that need to be updated in a monaco update. All these "couple of hours" add up. Updating monaco right now is a multi-week endeavor. We should strive to make it easier, not harder.

• I don't quite understand why the direct use of Monaco API outside of the monaco package would per se make the task of updating to a new version of @theia/monaco-editor-core more tedious. Would moving some code from the scm package down to the monaco package make the migration actually easier? It seems that approximately the same amount of work would still be required, as approximately the same Monaco APIs would still need to be used to implement the same functionality.

Keeping all references local to one package and consuming functionality outside that package via well-defined interfaces controllled by ourselves makes it clear what functionality we actually use from the monaco packages. I like to think in terms of "upward" and "downward" interfaces: the "upward" interface is everything VS Code offers internally. The "downward" interfaces is what Theia consumes from this "upward" interface. These interfaces are not the same and sometimes an adaptation layer is needed. Spreading this adaptation layer out makes it hard to understand and control the "downward" interface. It also makes it hard to reuse the adaptation layer, because as in the case of the PeekView it's usually private, because otherwise, it would make sense to make it a general service and move it to the "monaco" package.

[pisv]

@tsmaeder I have addressed a number of smaller items. It would be great if you could review and resolve them, or provide further feedback if necessary. This would make it easier to concentrate on the remaining ones.

[tsmaeder]

@pisv I resolved all the comments that I don't think need further action.

[pisv]

@tsmaeder I think I have addressed all of the remaining items except https://github.com/eclipse-theia/theia/pull/13104#discussion_r1497314715. Could you please review and resolve them, or provide further feedback? Thank you.

[tsmaeder]

Hi @pisv , except https://github.com/eclipse-theia/theia/pull/13104#discussion_r1497314715 I'm fine with this PR.

[pisv]

(In reply to https://github.com/eclipse-theia/theia/pull/13104#issuecomment-1983041968 and https://github.com/eclipse-theia/theia/pull/13104#discussion_r1497314715)

If I understand your comments correctly, you ask me to add the necessary API to the monaco package in Theia and use it instead of using VSCode monaco API directly in dirty-diff-widget.ts.

After careful consideration of what this would require, I don't think I currently have enough experience in Theia and monaco to design such API properly.

I guess that was part of the reason why I decided to take the shortcut of using monaco API directly. I hoped it would not be a problem provided that such usage is well-encapsulated, as described in https://github.com/eclipse-theia/theia/pull/13104#issuecomment-1966190802.

I can understand your concern, but having invested huge amount of time in this PR already, it would probably require even more time and very detailed guidance and prompt review feedback for me to design and implement the necessary API in the desired way.

Not sure this would be an optimal direction.

[pisv]

OK. I'm trying to come up with something in the meantime. Let us see how it goes...

[tsmaeder]

@pisv very cool, thanks. Rest assured that your efforts are much appreciated.

[pisv]

Hi @tsmaeder,

The latest commit gets rid of monaco internal API use in dirty-diff-widget.ts by adding the necessary API to the Theia monaco package and using it instead. Essentially, the new API introduces two things:

• MonacoEditorPeekViewWidget, which is a wrapper around monaco's PeekViewWidget

• Support for embedded editors (i.e. editors that have a parent editor) in MonacoEditor and MonacoDiffEditor

I'm keen to know what you think about it.

[tsmaeder]

Sounds good, I'll have a look.

[pisv]

@tsmaeder I'm really glad to hear that you are fine with this PR now. Thank you for your review and approval.