Readonly status

[connortsui20]

These changes address issue #2627.

(This is my first PR ever so apologies if I messed anything up...)

Reading through the issues and the previous PRs that reference a read-only flag on the statusline, it seems like this was supposed to be implemented, but other things (related to the statusline) took priority and this ended up getting dropped.

I am well aware that this is probably not the best way to do this (as this does not let users choose what color they want the text to be), but as a quick solution this works well enough. I feel like people would rather be able to see "[readonly]" than not at all, especially newcomers to helix who are used to vim that might make a bunch of changes not realizing they were editing a read only file (this was me pretty recently).

(As a side note I don't know how to undo that first gitignore commit, so sorry about that!)

[pascalkuthe]

Your PR queries the FS on every single frame which is unacceptable. The FS access must be cached (when opening, reloading and writing the file) at a minimum.

[connortsui20]

Would storing it in the Document struct be okay? If we read the metadata once the permissions are unlikely to change while someone has the file open.

[pascalkuthe]

#7128 (comment)

yeah that's the idea conderidering we are storing mtime too it might make sense to have a dedicated struct for metada (essentially our own reduced std::fs::Metadata) that's how a lot of c code usually handles that. But for now adding an extra field would probably do.

Ideally, we would have more extensive read-only handling, see #7128 (comment) although a status line indicator that indicates the file is not writable might be a reasonable first step.

CC @the-mikedavis

[connortsui20]

In the future, instead of having a readonly: Option<bool> field, it could be a metadata: Option<helix::Metadata> where the struct contains the readonly field, and it might also be beneficial to have other fields like time of access, time of last modification, or even length of file and file type. These are all things that std::fs::Metadata gives out (documentation here), and I can imagine it being nice to have on the statusline as an user-configured option.

[sigmaSd]

Vim also shows readonly flag for files that the user can't modify, like root files, which I think is actually the more common case then editing an actual readonly file.

[sigmaSd]

vim uses access syscall on linux for this (not exposed by rust std), don't have windows to test but maybe it uses https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-accesscheck

[pascalkuthe]

Using access would indeed be nicer, we could use rustix to execute that function (without unsafe code) altough it would only work on [cfg(Unix). On windows we can use the windows-sys crate altoufh that may require unsafe and a utf16 conversion? We already depend on both transitively and these are extenmenly widely used projects with significant institutional backing so it's reaosnabke to pull them in here IMO

[pascalkuthe]

Read-only status should be checked when sabing and reloading. Th disk can change at any time. Also the caching scheme is not necessary (see comments).

I also think that going the access route is likely nicer (see above)

[the-mikedavis]

I think a read-only indicator based on the fs metadata is a good idea but it should be its own statusline element rather than shared with the modification indicator. I think there should be two concepts for read-only: read-only files (fs-metadata-wise) which could just be a statusline element like this and a read-only way of opening files where you aren't allowed to modify them. Even if a file is read-only, you still might want to make changes and save those changes to a different writable path.

So for this PR I think a new statusline element is a nice change but the read-only way of interacting with files will need much larger changes.

[sigmaSd]

Here is the code for vim windows access implementation https://github.com/vim/vim/blob/4c0089d696b8d1d5dc40568f25ea5738fa5bbffb/src/os_win32.c#L7665
It seems to boil down to CreateFileW + some tricks to handle directories + network shares

[connortsui20]

I'm not going to lie I have no clue what is going on with what vim is doing here. However the unix access syscall seems to work well.

One other thing that I don't know how to approach is what the behavior should be when write-all is called. In vim it writes everything that can be written, but then shows a red error message 'readonly' option is set (add ! to overwrite).
The error message doesn't specify which file is readonly, only that there is one among the current buffers.

It would be nice if it showed which specific file or files are readonly, as the above message is kind of vague. Also, while testing this in helix sometimes the error message would get covered by the "file_name" written ..., but I can't consistently reproduce that.

[pascalkuthe]

Here is the code for vim windows access implementation vim/vim@4c0089d/src/os_win32.c#L7665 It seems to boil down to CreateFileW + some tricks to handle directories + network shares

yeah I don't think its fair to require that for merging. If you feel interested/up to it @connortsui20 then go for it but honestly just using access from rustix on UNIX and using the standard metadata.readonly field on Windows is enough for me (leave a todo note for the future with a link tough). The access syscall is a very standard part of POSIX and widely used/avialable (and easy to use). The windows variant seems to me like POSIX emulation and not exactly standard (and pretty complex to implement). It could be nice to have in the future but it doesn't seem to be standard practice on Windows (I am not even sure if this kind of low level thing should live in helix or a standalone crate).

One other thing that I don't know how to approach is what the behavior should be when write-all is called. In vim it writes everything that can be written, but then shows a red error message 'readonly' option is set (add ! to overwrite).
The error message doesn't specify which file is readonly, only that there is one among the current buffers.

Hmm I guess the issue with that is that there could be many readonly files but there is only so much space in the statusline. It's probably fine to just print a list of butters here (that would be truncated if there is not enough space). Usually there are only few readonly files.

It would be nice if it showed which specific file or files are readonly, as the above message is kind of vague. Also, while testing this in helix sometimes the error message would get covered by the "file_name" written ..., but I can't consistently reproduce that.

Yeah that's probably expected since writing happens asycn so that message could come trough later or earlier (non-deteministically). That's a problem in general, we should likely improve the way we display status messages. You don't need to worry about that here tough, that can be left to future work since il likely needs a systematic change

[pascalkuthe]

one thing that could also be nice is to show whether a file is readonly in the buffer picker. What do you think about that @the-mikedavis?

[connortsui20]

The readonly error messages are kind of verbose for write-all, but it sort of doesn't matter because you can't see it anyways if there is even a single writeable file among the buffers

[connortsui20]

Also I'm just going to mention #3709 because I don't really know what happened over there but it seems like similar changes were made then dropped

[pascalkuthe]

Also I'm just going to mention #3709 because I don't really know what happened over there but it seems like similar changes were made then dropped

we don't know why that author closed they PR we gave them feedback in a timely manner and weren't opposed to the changes. Some things will always remain a mystery I guess

[pascalkuthe]

I think the readonly statusline element should be added to the docs too and I haven't checked but IMO it should also be part of the default statusline so if its not part of the default yet you can add it

[connortsui20]

Do I just need to change the docs in configuration.md? I'm not actually sure how the configuration in the config.toml pulls settings in, I'm assuming the logic is here but I'm not sure if I need to add something to translate a read-only-indicator to the enum datatype StatusLineElement::ReadOnlyIndicator ... or is that being handled by serde?

[pascalkuthe]

Do I just need to change the docs in configuration.md? I'm not actually sure how the configuration in the config.toml pulls settings in, I'm assuming the logic is here but I'm not sure if I need to add something to translate a read-only-indicator to the enum datatype StatusLineElement::ReadOnlyIndicator ... or is that being handled by serde?

default config is in the various Default implementations for the config structs

[kirawi]

Here is the code for vim windows access implementation vim/vim@4c0089d/src/os_win32.c#L7665 It seems to boil down to CreateFileW + some tricks to handle directories + network shares

yeah I don't think its fair to require that for merging. If you feel interested/up to it @connortsui20 then go for it but honestly just using access from rustix on UNIX and using the standard metadata.readonly field on Windows is enough for me (leave a todo note for the future with a link tough). The access syscall is a very standard part of POSIX and widely used/avialable (and easy to use). The windows variant seems to me like POSIX emulation and not exactly standard (and pretty complex to implement). It could be nice to have in the future but it doesn't seem to be standard practice on Windows (I am not even sure if this kind of low level thing should live in helix or a standalone crate).

One other thing that I don't know how to approach is what the behavior should be when write-all is called. In vim it writes everything that can be written, but then shows a red error message 'readonly' option is set (add ! to overwrite).
The error message doesn't specify which file is readonly, only that there is one among the current buffers.

Hmm I guess the issue with that is that there could be many readonly files but there is only so much space in the statusline. It's probably fine to just print a list of butters here (that would be truncated if there is not enough space). Usually there are only few readonly files.

It would be nice if it showed which specific file or files are readonly, as the above message is kind of vague. Also, while testing this in helix sometimes the error message would get covered by the "file_name" written ..., but I can't consistently reproduce that.

Yeah that's probably expected since writing happens asycn so that message could come trough later or earlier (non-deteministically). That's a problem in general, we should likely improve the way we display status messages. You don't need to worry about that here tough, that can be left to future work since il likely needs a systematic change

Neovim uses libuv's uv_fs_access: https://github.com/neovim/neovim/blob/8df37423781493f58de060e1c9219cd1c3768130/src/nvim/os/fs.c#L933-L939

It uses GetFileAttributesW which is exposed in Rust via std::os::windows::fs::MetadataExt::file_attributes() which is consequently exposed by Permissions::readonly(). I think we could either copy vim or try using AccessCheck. See also https://web.archive.org/web/20230129045429/https://blog.aaronballman.com/2011/08/how-to-check-access-rights/

[pascalkuthe]

sounds like we can just use access via rustix on UNIX and std everywhere else (on windows)

[kirawi]

Yeah, but unfortunately it would not report if the client has write privileges for that file.