fix: readonly permissions with disk FS provider

[kittaakos]

What it does

Sets the permissions of the stat retrieved from the disk filesystem provider to FilePermissions.Readonly if the owner does not have write access.

Closes #199
Closes #12342

12342.mp4

How to test

Remove the owner write permission from one of the files on your disk, and open it in the editor. The editor is readonly. You see the 🔒 in the editor tab, or please reference the attached screencast. 👆

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

[msujew]

I'm wondering whether this is the correct approach to local readonly files. I've implemented the original readonly mode for files coming from readonly file system (such as contributed from a vscode extension). These are file systems for which we know that we can never write content into.

However, for local files, we have a bit more control over these files. VSCode is also handling this a bit differently: Opening a readonly file works like with a normal file, but saving it yields a notification that offers the user to make a file writable or save it as a new file. See screenshot:

I like this approach better, maybe it makes sense to adapt it to Theia as well?

[kittaakos]

I like this approach better, maybe it makes sense to adapt it to Theia as well?

OK. We should do that. Thanks for your input.

[dpetroff]

Is there a setting that can disable this dialog? I do not think this dialog makes sense on unix-like file systems: VSCode's opinion on this matter most likely comes from the "read-only" flag on windows file systems, where it is possible for a user to have write permissions to a file but the file can be marked as read-only independently. I do not think this is a thing in unix file systems at all. Regardless of the file system flavour, if the user does not have permissions to write to the file, this dialog does not make sense to me as the overwrite will certainly fail. There is also the question of how this interacts with the autosave feature.

Setting the above aside, I have a use case where local files on a unix file system are marked read-only (i.e. the user does not have write permission for those files) and it does not make sense for them to be editable in the editor at all. At a minimum, there should be a setting that disables this dialog, and a setting that disables editing of local read-only files, or at least a very easy way for me as a developer of a Theia app to disable that dialog and editing of read-only files in the editor.

[msujew]

@dpetroff The dialog screen shot was done on an Ubuntu machine, not Windows. Note that vscode seems to use different means of making the file writable, see here.

But I agree, having some sort of preference that allows to disable editing of read only files, seems like a good solution. I think the default should still be to allow editing and then use the Save As... command as a fallback in case the file cannot be saved, but devs should be able to change that behavior. @kittaakos What do you think about that?

[dpetroff]

@msujew, I mentioned Windows because it seems to me that the entire concept of "overwriting read-only files" is firmly rooted in VSCode's history as a tool written by Visual Studio developers who are, at their core, Windows developers, so VSCode's behaviour comes with Windows-inspired baggage that you're not required to blindly reimplement.

Besides this, VSCode is targeted at developers/power users, and Theia is a customisable IDE framework, so it does not immediately follow that if VSCode does something, it's inherently suitable for Theia to do the same by default.

In the end, as long as the dialog does not exist without an option to fully disable it as well as the editing of read-only files under the hood, I'm happy. But I'd also like you to consider what would make sense as behaviour when the "auto-save" option is enabled. I don't think it makes sense for me to get a "Would you like to overwrite this" 0.5 seconds after every time I write something in a read-only editor (if that ability is enabled). But also it doesn't make sense to me that editors shouldn't save automatically when the "auto-save" option is enabled.

Either way, this PR resolves the issues it sets out to resolve and the video above shows exactly the behaviour I expect from Theia, so it seems to me that the dialog you're proposing and its implications on auto-save belong to a separate set of issues/PRs and we should not discuss it further in the context of this PR.

I would like to see it merged as soon as possible because it resolves a major issue in my use case for Theia.

[tsmaeder]

I would agree that the case of clearing the read-only flag is Windows-specific. However, even on Unix-like file systems, there is usually the option to save the file somewhere the user does have write access. Therefore disabling editing based on the file being read-only does not really make sense to me. @dpetroff could you elaborate why you would want to prevent editing? For me, the logical thing to do would be to open a "Save as..."-dialog upon save if the file is read-only, with the Windows-specific override of offering to remove the read-only flag if that's what's preventing us from saving over the file.

[dpetroff]

There are plenty of reasons why editable read-only files with an automatic save dialog might not make sense.

One reason, which also happens to be why this would be a problem in my use case, is that it encourages code duplication: "Oh, you're not allowed to overwrite this library source. Why don't you keep on editing it, and here's a button that lets you easily duplicate the whole thing after." No, thanks!

By making read-only files editable and save-as-able by default, you're assuming that all Theia users are power-users that are fully cognisant of the implications of taking the seemingly simple easy option presented to them. But this is not a valid assumption. Remember that Theia is "An Open, Flexible and Extensible Cloud & Desktop IDE Platform" with an emphasis on the Could and Flexible IDE Platform parts, and VSCode is a Historically-Closed-Turned-Open, Inflexible and Extensible-Where-Permitted Only-Recently-Cloud & Primarily-Desktop Actual-IDE-Not-Platform with an emphasis on the Inflexible and Desktop parts, so many of the things that seem logical to you as a developer and to VSCode users would not necessarily make sense for many Theia-based product users.

I do not think this discussion is moving in a productive direction at this point. If anyone feels strongly about the need for editable read-only files with an automatic save dialog, I would like you to consider some reasons why it is inappropriate to continue the discussion here instead of a separate Issue related to that behaviour specifically:

• This PR implements an acceptable solution to Handle readonly files #199 and Lock icon is not visible in the editor for readonly resources, saving the dirty editor throws an error #12342 and someone very interested in Handle readonly files #199 (me) confirms that the solution as demonstrated above in this PR is exactly what they would expect
• The proposed feature is only tangentially related to the issues that this PR addresses
• The proposed feature is a new feature that nobody has requested by opening a related Issue
• An actual user (me) has told you that the proposed feature would break their use case

I'm happy to continue the discussion if you would like to open an Issue and tag me there but from my perspective this PR is done and there is no need to derail it by discussing a tangent any further.

[msujew]

I do not think this discussion is moving in a productive direction at this point.

@dpetroff I agree. As maintainers of an open source framework we do not serve any one interested party but many. While the change in question might benefit you specifically, it might break other workflows in unpredictable ways, especially once we start moving away from the UX inspiration (vscode). When people see a vscode-like IDE with vscode extension capabilities, they assume vscode UX.

This PR implements an acceptable solution to #199

Funnily enough, the last relevant comment on the linked issue is exactly about asking how vscode handles this case and whether we should follow their implementation. I don't think the acceptable solution is clearly about locking read-only file editors.

As mentioned earlier, I'm open to the discussion on this and I agree to have a read-only editor mode that can be controlled through a preference. I'm just not willing to approve this PR as it is.

Aside from that, @kittaakos are you still planning to continue with this feature? You've been quiet about this topic for a while.

[kittaakos]

Aside from that, @kittaakos are you still planning to continue with this feature? You've been quiet about this topic for a while.

Thanks for the ping.

While the change in question might benefit you specifically, it might break other workflows in unpredictable ways, especially once we start moving away from the UX inspiration (vscode). When people see a vscode-like IDE with vscode extension capabilities, they assume vscode UX.

I did not change my opinion (#12354 (comment)); Theia should do what VS Code does. The default behavior could be adjusted with a preference, but it's a pipe dream.
I am still interested in implementing it, but it's not the highest priority.

[dpetroff]

@msujew, just to be clear, the value I see in this PR is that I won't have to roll out and maintain my own half-baked fixes to disk-file-system-provider so all the resource, filestat, etc. properties do not lie to me about the read-only status. That it also happens to result in my desired UX is a very welcome added bonus.

I see the UX as an entirely separate issue from this PR. The PR propagates the read-only flags correctly. The flags being propagated then triggers the existing UX for read-only files in Theia that was previously not triggered because the flags were not being propagated.

It doesn't make sense to me that you're blocking this PR that solves the API problems just because the already existing UX does not match your VSCode-based UX expectations. Had I defined my own file system provider or provided my own resource with the read-only flag set somewhere else along the way to the editor I would get exactly the UX I want without the changes in this PR and without any further UX-related changes on my part, so I don't see why you insist that a UX change is somehow integral to fixing the broken flag propagation.

By the way, the current UX in Theia is what VSCode does for read-only/virtual file systems defined by extensions anyway, and there is a setting that changes the overall behaviour to something similar too:

Regarding the rest of it, I think the comment you referenced was about the VSCode APIs and not the UX. I don't think the UX was part of the discussion there at all. Either way, I still disagree that Theia should preserve all possible VSCode UX by default. If I wanted my IDE to act exactly like VSCode, I'd just go clone VSCode and toss Theia in the bin. But even the VSCode "Readonly From Permissions" setting tries to circumvent the file system flags as you can see in the message above that allows the user to override it anyway. This override is something that might lead to a lot of confusion to users of my Theia app, so I would need to disable for my use case.

From my perspective, Theia is an IDE platform, not an IDE. I think this distinction is very important to everyone using Theia. It would make sense to me to say that Theia Blueprint should copy the VSCode UX because the goal of Theia Blueprint is to be an example of using Theia as a platform to implement a VSCode-like UX. I think the strengths and appeal of Theia are in the parts of it that aren't VSCode because VSCode is now open-source too. The Theia extension APIs are genuinely more flexible and nicer to work with than anything VSCode has to offer. If there's an important VSCode extension that absolutely must run in my Theia app, as a Theia app developer, I can fairly easily go and adjust my Theia app then maybe I'll come back and contribute the fix to the Theia project. But instead if Theia breaks my Theia app every release by introducing some VSCode UX with broad implications that I did not anticipate, I might end up just biting the bullet and switching to VSCode entirely.

I agree that as maintainers of an open source framework you do not serve any one interested party but many. Where we disagree is that you're proposing to adopt a VSCode UX with very broad implications that previously did not exist in Theia and will break all Theia apps that rely on the current read-only editor behaviour. I think it's worth reiterating that this behaviour already exists in Theia without this PR and is likely to already be relied upon by many other Theia apps besides the one I maintain.

If I understand your argument correctly, you wish to maintain maximum compatibility with VSCode extensions but I'm not convinced that the VSCode extension APIs permit extensions that would be broken by lacking this proposed UX in the first place.

More importantly, the particular UX from VSCode is very intent on making truly read-only editors that respect the file system impossible as you can see in those screenshots. Why? I could not tell you. But it is a VSCode design decisson with broad implications that following your logic must be thrust upon all Theia app developers. I don't think this is reasonable. It needs to be considered carefully and discussed in detail separately from this PR, which is about making the API propagate the read-only property correctly from the file system.

Please consider merging this PR as is and continuing the UX discussion separately.

[tsmaeder]

@dpetroff @msujew in the interest of moving forward, I think we should separate the "what's the right behavior of the UX" from "should we merge this PR". I quickly checked the UX for both cases (on Windows): right now, you can edit a read-only file, but saving the edited file silently fails. With the change, the editor is read-only and you get a hover indicating that you cannot edit a read-only file. You can, however, save the file to a different place.
T.b.h, I prefer the UX with the change (even though I still think it's the wrong), because at least, it's transparent to the user what's going on. At the very least, it's no worse than what we have now.

IMO, the question we should ask before merging a PR is not: "could this PR be better?", but "does this improve the code base vs. the current state?". Since this one makes the API more correct and the UX no worse (IMO), I think we should merge it.

[tsmaeder]

@kittaakos why is this PR a draft?

[kittaakos]

@kittaakos why is this PR a draft?

See #12354 (review)

I made it ready for review, and you can decide if it's OK to merge. I can rebase if needed.

[dpetroff]

@tsmaeder 100% agree with everything you said. That is exactly what I want.

[msujew]

Looks good to me 👍