-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Readonly status #7740
Readonly status #7740
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
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 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. |
In the future, instead of having a |
b64b1e6
to
de1f25c
Compare
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. |
vim uses |
Using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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. |
Here is the code for vim windows access implementation https://github.com/vim/vim/blob/4c0089d696b8d1d5dc40568f25ea5738fa5bbffb/src/os_win32.c#L7665 |
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 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 |
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
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.
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 |
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? |
The readonly error messages are kind of verbose for |
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 |
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 |
Do I just need to change the docs in |
default config is in the various |
9eab6a8
to
6088449
Compare
Neovim uses It uses |
sounds like we can just use access via rustix on UNIX and std everywhere else (on windows) |
Yeah, but unfortunately it would not report if the client has write privileges for that file. |
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!)