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

Handle deleted files in open editors widget #10361

Conversation

kenneth-marut-work
Copy link
Contributor

image

What it does

Fixes #10322 by adding support for deleted editors in the Open Editors widget. Adds a strikethrough decoration when a deleted editor is detected.

How to test

  • Build this branch and open a workspace with some files
  • Ensure the Close On File Delete preference is set to false
  • Open several files as editors (both regular and preview editor(s))
  • Ensure the open editors widget is shown in the navigator
  • In an external terminal delete the files you have opened using, observe that the deleted files are decorated with a strike-through
  • Drag the deleted editors around to different tabbars and observe that the decorations persist
  • Observe there are no errors in the terminal, and that you can continue to open/add new editors and the Open Editors tree is properly updated
  • Restore the deleted files (with git restore) and observe the strikethrough is removed

Review checklist

Reminder for reviewers

@kenneth-marut-work kenneth-marut-work changed the title Draft: Handle deleted files in open editors widget Handle deleted files in open editors widget Nov 1, 2021
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed the following behavior 👍

  • confirmed that the strike-through decoration is applied when the resource is deleted
  • confirmed that the decoration persists if the deleted editor is moved (different groups)
  • confirmed that restoring the file removes the decoration (from outside the application)
  • confirmed that restoring the file removes the decoration through git
  • confirmed when deleted that the decoration is removed if the file is re-created manually in the app

@vince-fugnitto vince-fugnitto added navigator issues related to the navigator/explorer scm issues related to the source control manager labels Nov 2, 2021
@colin-grant-work
Copy link
Contributor

An earlier version of this PR implemented the strikethrough decoration in the editor-preview package; the current version piggy-backs off of the git decorator's work. The downside of the current approach is that it doesn't work for untracked files or in workspaces that aren't source controlled.

The previous approach required a fair bit of code to track which editors corresponded to deleted files; the current approach is a fair bit simpler. It might be possible to achieve almost the same simplicity by implementing a decorator (perhaps a decorator service, rather than a tree decorator) in the monaco package, which knows whether its models are valid or not.

@vince-fugnitto
Copy link
Member

Along with what @colin-grant-work mentioned, I also thought about not having the deleted decoration as part of scm as well (due to the same concerns about being under version control), and instead having it under filesystem (which knows about deleted files).

At the moment @theia/filesystem applies a (deleted) suffix to widgets that have their resources deleted such as an editor:

protected readonly deletedSuffix = ' (deleted)';

Perhaps we can generalize the behavior to decorate from that extension.

@kenneth-marut-work
Copy link
Contributor Author

Along with what @colin-grant-work mentioned, I also thought about not having the deleted decoration as part of scm as well (due to the same concerns about being under version control), and instead having it under filesystem (which knows about deleted files).

At the moment @theia/filesystem applies a (deleted) suffix to widgets that have their resources deleted such as an editor:

protected readonly deletedSuffix = ' (deleted)';

Perhaps we can generalize the behavior to decorate from that extension.

That sounds like a good approach. I'll give that a shot 👍

@kenneth-marut-work
Copy link
Contributor Author

@vince-fugnitto @colin-grant-work
I've pushed a change that hooks into the filesystem's mechanism that adds the (deleted) "decorations" to the editor tabs. Since Navigator has a dependency on Filesystem, I've implemented the decorator inside the navigator package. Let me know if this is what you had in mind, thanks!

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the functionality works as intended 👍

  • deleting a file outside the app causes the line-through for open editors
  • restoring the file removes the decoration
  • the decoration persists when the file is moved to different editor groups
  • the decoration now works with workspaces outside of version control 👍
  • restoring the file in-app (by recreating it manually) removes the decoration

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work well for me, I'll let @colin-grant-work take a look if he wants 👍

@kenneth-marut-work
Copy link
Contributor Author

The changes work well for me, I'll let @colin-grant-work take a look if he wants 👍

Thanks Vince, he's out today but should be back tomorrow

@colin-grant-work colin-grant-work self-requested a review November 16, 2021 16:46
@colin-grant-work colin-grant-work self-requested a review November 18, 2021 19:17
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

This is working well for me. 👍 Thanks for moving to a URI-tracking system rather than an editor-tracking system. I think it should be more robust and more (potentially) useful elsewhere.

@kenneth-marut-work
Copy link
Contributor Author

Thanks for the review @colin-grant-work and @vince-fugnitto. Merging now 👍

@kenneth-marut-work kenneth-marut-work merged commit 82c847d into eclipse-theia:master Nov 19, 2021
@vince-fugnitto vince-fugnitto added this to the 1.20.0 milestone Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
navigator issues related to the navigator/explorer scm issues related to the source control manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open Editors causes error on deleted file
3 participants