-
Notifications
You must be signed in to change notification settings - Fork 329
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 support for displaying lf options in the ruler #1257
Conversation
@joelim-work Thank you very much for investigating various ways to implement this feature. I really appreciate the simplicity of this approach. I have tested it briefly and it seems to work as intended. I was slightly worried that we now need to export all variables while drawing, but I have tried profiling a simple session and exporting does not seem to show up at all, so I guess it is not really a big deal. Leaving the job to external scripts running with
The above currently leaves the process behind after quitting on my machine but I think it could also be handled somehow (either using I'm ok with either keeping or removing the builtin diskfree, so I will leave the decision to the contributors (cc @rrveex ). |
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.
Overall, this seems like a great idea.
Thinking out loud, I am also wondering if there's a better solution than suggesting cron
.
The most obvious solution is to have an on-reload
command that lf runs on reload. One problem with that is that it might slow down lf
a lot if a user configures this variable unwisely. However, we can forcefully suggest people use &
commands in on-reload. Also, if we don't introduce on-reload, people might use on-select
for this, which would be even worse.
Another issue is what the on-reload
command should do if the previous on-reload
is still running.
Perhaps a better on-reload
command would send something to a pipe, and we could suggest a script that runs something when it sees something in the pipe. This approach is inspired by how lf
works; we could even build a little simple client that runs a command when the lf server sends something to its named pipe into lf
.
Another good solution might be to suggest some tool that is simpler than cron
and would only run while lf
runs. Something like entr/watchexec
, but ideally simpler and without the file watching. I don't have one in mind right now and don't know if it exists, but I might look.
@gokcehan @ilyagr thanks for the review and feedback. To answer your questions.
I agree this is a possible concern, this is being discussed in #1257 (comment).
I did not consider spawning a new process like this, thanks for the idea! I think it's possible to just have the while loop check if the parent &{{
# prevent more than one instance if lfrc is sourced again
lockfile=/tmp/lf_ruler.$PPID.lock
if [ -f "$lockfile" ]; then
exit
fi
touch "$lockfile"
# quit if parent lf process no longer exists (alternatively use ps here)
while [ -d "/proc/$PPID" ]; do
diskfree=$(df -h --out=avail . | awk 'NR == 2 { print $1 }')
lf -remote "send $id set user_df $diskfree"
lf -remote "send $id set user_time \"$(date +%r)\""
sleep 1
done
rm -f "$lockfile"
}}
set ruler lf_user_df:lf_user_time
Although I lean slightly towards removing
The intent of this PR is to simply provide a basic mechanism for allowing the user to display options in the ruler, and it is up to users to decide how to make use of them. I only mentioned
I thought about providing such a hook too, but I didn't really investigate it because I had some concerns:
Again I haven't investigated this in detail, but I think this is also a question that can be addressed outside of this PR. |
That's totally fine. I think the PR will be quite useful independently of solving that problem. I think that maybe we shouldn't "officially" recommend |
Agreed.
I don't think this is a problem: the command will update the
This is a very good point! I think this could be addressed, maybe, but we should certainly worry about it.
Certainly. |
Regarding |
I believe that removing the These changes are fantastic and align better with the app's extensibility, focusing on its core functionality rather than trying to handle every possible use case internally. |
Thanks @DusanLesan, I will take your opinion into consideration. I think this PR won't get merged this weekend anyway (maybe there's still some wrinkles to iron out), so in the meantime you're welcome to try the config in #1257 (comment) and see if it works for you. Although I think the script does work nicely, there are probably some limitations. For instance I have found that the background process doesn't change it's CWD even after you Maybe we do need a separate timer, or some kind of |
@joelim-work We can merge the patch if it is ready. I wasn't sure because there is still an ongoing discussion but I guess it is mostly about future changes. Regarding the builtin diskfree, I'm still not sure if it is better to remove it. I'm not personally using it myself so I don't mind it either way, but still I think I'm in favor of keeping it for the following reasons:
On the other hand, it also has some disadvantages:
A quick note about I feel like the discussion can benefit from additional use cases about the use of user options other than diskfree. I have been trying a few examples myself with the
Another example is to show the sum of sizes in the current directory:
Do we have other use cases to stimulate the discussion? The way I see this patch, the primary purpose is its capability to show the values of options in the ruler. The secondary purpose is acting as a barrier to prevent requests about niche use cases, though I'm not sure diskfree is one of them especially if it requires making use of |
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.
The code looks great to me.
In retrospect, it would have been good to mark the diskfree functionality as experimental and subject to change when we merged it. Perhaps we could do it now.
At the moment, I don't have an opinion on whether the diskfree stuff should stay as is, be changed, or be removed entirely in favor of the scripts. I'll need to play with the scripts a bit to decide, and perhaps other people will try that out as well.
eval.go
Outdated
app.ui.echoerr("ruler: should consist of 'df', 'acc', 'progress', 'selection', 'filter' or 'ind' separated with colon") | ||
return | ||
if !strings.HasPrefix(s, "lf_") { | ||
app.ui.echoerr("ruler: should consist of 'df', 'acc', 'progress', 'selection', 'filter', 'ind' or start with 'lf_' separated with colon") |
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.
Minor and optional: I was thinking about how to better word this. Here's one idea.
"ruler: invalid value '%s'. Must be a colon-separated list consisting of a subset of 'df', 'acc', 'progress', 'selection', 'filter', 'ind', or 'lf_OPTION_NAME'."
I'm also worried that this is usually too long to fit one the screen. Perhaps we should point people to the docs instead of listing the options? Or print the error to stderr? I don't think we have to resolve that for the purposes of this PR, though. For now, if people really want to read it, they can maximize their window, make the font tiny, and try again.
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.
For now I just simply changed start with 'lf_'
to lf_<option_name>
, hopefully that makes the grammar a bit more clear to understand.
I don't really have a strong opinion on how to format these error messages, apart from the desire to keep things fairly consistent among the different user options. Both ruler: should consist of ...
and ruler: invalid value, please see the documentation for more information
sound fine to me. One other thing worth mentioning is that if all the different fields/possibilties are listed, it's possible someone in the future may forget to update the error message when adding a new one. That being said, I haven't seen any issues raised about unclear error messages, and I (would like to) think that users are generally smart enough to turn to the documentation anyway if they need help on configuration, so I'm not really inclined to change anything right now.
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.
Feel free to go with whatever makes sense to you. This is a very optional suggestion, and I agree this doesn't matter very much.
I'll still elaborate: I was mainly thinking of 1) including the offending value and 2) saying "colon-separated list" first, in the part of the message that would probably fit on the screen.
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.
And by the way, I think the change you made definitely helps. In general, I think the PR is in a good shape. Thanks for creating it in the first place and and fixing all the nits!
@gokcehan I think this a good interpretation - the idea is to store all the options as a Regarding the diskfree code, I'm actually starting to think it should be kept, at least for the following reasons:
I can't really think of many other use cases myself, TBH the idea of setting user information externally and then displaying it in the statusbar is already quite niche. @ilyagr Hopefully I have addressed all of your comments, if not then let me know. Thanks once again for your review. |
I have found another use of user-defined variables, which is to alter the way a builtin setting is displayed (i.e. display something based on the value of builtin settings). For example, the following config shows set ruler acc:lf_user_hidden:ind
cmd draw_hidden %{{
if [ "$lf_hidden" = true ]; then
lf -remote "send $id set user_hidden hidden"
else
lf -remote "send $id set user_hidden"
fi
}}
map zh :set hidden!; draw_hidden
# start with hidden enabled (optional)
set hidden; draw_hidden |
@joelim-work Thanks again for the patch. |
Summary
This change enhances the
ruler
option to allow fields starting withlf_
, which will resolve to the corresponding environment variables exported bylf
. Built-in options are displayed in the format<name>=<value>
, for examplehidden=true
, while user-defined options (added in #865) are displayed as is.Examples
Built-in options
Display the current value of the
selmode
option (requested in #1174):User-defined options
User-defined options can be used to store arbitrary data in
lf
to be displayed in the ruler. This can be done by creating a script that calls something likelf -remote "send set user_test 'hello world'"
and then running that script periodically (e.g. usingcron
).Display the amount of free disk space remaining (requested in #1103):
Display the time
with a blue background(styling is no longer supported, see #1257 (comment)):Replacing the builtin
df
functionalityBecause this change can now handle calculating free disk space externally, it leaves the question of whether it's worth keeping the
df
functionality added in #1168. On one hand, it does provide convenience for users, and removing it would be a breaking change, but on the other hand it adds more maintenance burden to the project (all thedf_<platform>.go
files), and contradicts with the minimalistic design oflf
. I am leaning towards removing thedf
functionality since it is still relatively new, but I can understand if others prefer to keep it.