Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Dirty Diff Peek View #13104

Merged
merged 14 commits into from
Apr 23, 2024
Merged

Add Dirty Diff Peek View #13104

merged 14 commits into from
Apr 23, 2024

Conversation

pisv
Copy link
Contributor

@pisv pisv commented Nov 27, 2023

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:

Screenshot

and vscode.git:

Screenshot

How to test

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

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

  2. 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.

  3. 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.

  4. 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.

  5. 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

Reminder for reviewers

@msujew msujew added monaco issues related to monaco scm issues related to the source control manager labels Nov 27, 2023
@pisv pisv force-pushed the GH-4544 branch 2 times, most recently from 36cfcf1 to f024b80 Compare November 28, 2023 19:19
@pisv pisv force-pushed the GH-4544 branch 2 times, most recently from aed2a71 to 8b551e9 Compare December 22, 2023 19:58
@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 7, 2024

@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 Theia is not seeing repository for workspace. #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
Copy link
Contributor Author

pisv commented Feb 7, 2024

@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:

  1. Check out the PR branch.

  2. 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".

  3. Clean-build and run Theia Electron Example:

    yarn clean
    yarn
    yarn download:plugins
    yarn electron build
    yarn electron start
    
  4. Open the theia workspace.

  5. 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
Copy link
Contributor

tsmaeder commented Feb 9, 2024

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.

@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 9, 2024

Maybe a Mac/Windows problem?

@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 9, 2024

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
Copy link
Contributor Author

pisv commented Feb 9, 2024

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
Copy link
Contributor Author

pisv commented Feb 14, 2024

@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 pisv marked this pull request as draft February 15, 2024 14:51
@pisv
Copy link
Contributor Author

pisv commented Feb 15, 2024

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
Copy link
Contributor

Perhaps not surprisingly, given that #13217 was merged earlier today...

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

@pisv pisv marked this pull request as ready for review February 16, 2024 16:52
@pisv
Copy link
Contributor Author

pisv commented Feb 16, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

pisv commented Feb 21, 2024

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: microsoft/vscode@0026000.

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

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this very useful 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.

packages/scm/src/browser/dirty-diff/diff-computer.ts Outdated Show resolved Hide resolved
packages/scm/src/browser/dirty-diff/dirty-diff-widget.ts Outdated Show resolved Hide resolved
packages/scm/src/browser/dirty-diff/diff-computer.ts Outdated Show resolved Hide resolved
return result.join('');
}

class DirtyDiffPeekView extends PeekViewWidget {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problematic as it adds more direct dependencies on VS Code internals outside of the "monaco" package. This leads to more work each time we want to update to a new version of the monaco editor. I think the right thing to do would be to implement a Theia PeekView on a Theia editor and to implement the diff, etc. using the Theia editor package, not monaco interfaces. This way, other clients could reuse the view in a way that respects layering.

packages/scm/src/browser/scm-contribution.ts Show resolved Hide resolved
packages/scm/src/browser/scm-tree-widget.tsx Show resolved Hide resolved
@pisv
Copy link
Contributor Author

pisv commented Feb 27, 2024

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
Copy link
Contributor

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

@rayguncertified1
Copy link

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
Copy link
Contributor

tsmaeder commented Mar 7, 2024

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.

@tsmaeder
Copy link
Contributor

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

@pisv
Copy link
Contributor Author

pisv commented Apr 8, 2024

@tsmaeder I think I have addressed all of the remaining items except #13104 (comment). Could you please review and resolve them, or provide further feedback? Thank you.

@tsmaeder
Copy link
Contributor

tsmaeder commented Apr 9, 2024

Hi @pisv , except #13104 (comment) I'm fine with this PR.

@pisv
Copy link
Contributor Author

pisv commented Apr 10, 2024

(In reply to #13104 (comment) and #13104 (comment))

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 #13104 (comment).

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
Copy link
Contributor Author

pisv commented Apr 12, 2024

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

@tsmaeder
Copy link
Contributor

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

@pisv
Copy link
Contributor Author

pisv commented Apr 16, 2024

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
Copy link
Contributor

Sounds good, I'll have a look.

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now.

@pisv
Copy link
Contributor Author

pisv commented Apr 22, 2024

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

@tsmaeder tsmaeder merged commit 746f8ff into eclipse-theia:master Apr 23, 2024
14 checks passed
@pisv pisv deleted the GH-4544 branch April 23, 2024 12:09
@jfaltermeier jfaltermeier added this to the 1.49.0 milestone Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco scm issues related to the source control manager
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support change details in the editor
5 participants