-
Notifications
You must be signed in to change notification settings - Fork 326
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 'setlocal' for directory specific options #1381
Conversation
I have never thought of this before. I guess the idea is to allow directories to actually have different settings instead of the hack mentioned in the wiki which changes the global settings whenever a certain directory is visited. I think the biggest aspect to consider is the interaction between global vs local settings. From a quick look at the patch, it looks like you just simply make a clone of the global settings when Regarding the syntax, I guess |
@joelim-work Thank you for the feedback. I wasn't aware of the example in the wiki page. I have tried it today and it seem to work well, though it is worth explicitly mentioning that its behavior is slightly different than this patch. Since it changes the global option, it effects each directory in the ui, which can result in a disorienting flicker. It also does not work if the corresponding directory is in the preview pane. So I still think a separate It is nice to have the For the boolean options, I haven't added the It sounds like a good idea to follow vim convention syntax to reset an option (I think maybe you changed It seems that |
To me, the
TBH I don't think the Regarding the toggling of boolean options, I would expect Despite suggesting it, I'm not 100% convinced on using the Vim syntax for resetting an option since it may not be intuitive to users. Using Anyway I don't have much more to add, the PR in general looks fine to me and I would probably approve it once it's polished (e.g. documentation and unit tests added). But it's also OK to leave it open to see if anyone else has feedback. |
@joelim-work I wasn't aware of |
Hi, author of the original example from the wiki here. Nice to have a native way to apply local options. Also, there is nothing wrong with implementing some options just for yourself (considering you are the author in first place), especially if it doesn't introduce a breaking change. Is it meant to only apply to a given directory, no recursion? Because, in my example recursion is the point - you have some directories that have things you want sorted. Like
and you wouldn't want to write
for every single one of them, right? Because sorting episodes right is the point, not alphabetically sorting movies by name. |
@MahouShoujoMivutilde Thank you for the feedback. To be honest, I haven't even considered recursion so far. Now I can see how the One possible workaround is to recursively set local options at launch with something like:
I think this is much simpler than the current If we want to add a builtin support for this, then I guess the usual way would be to allow glob patterns in
There could be some issues with trailing |
I don't like this workaround because it is static for a session and is IO-intensive (even when you don't plan to visit those directories). I can sort-of convert the on-cd example to make it more dynamic cmd on-cd &{{
case "$PWD" in
/mnt/movies*)
lf -remote "send $id setlocal '$PWD' sortby natural"
lf -remote "send $id setlocal '$PWD' noreverse"
lf -remote "send $id echomsg setlocal: sort = natural"
;;
esac
}} But it has predictable flicker issues. This can be avoided with checks even more complex than original wiki example and more dynamic I am okay with limited globs, even just prefix string matching tbh. |
I had not considered recursive directories either, but I think it makes sense to support this functionality. I agree with @MahouShoujoMivutilde that the Regarding the syntax, I would prefer to have it referred to as a 'glob-like syntax' rather than actually implementing globs. As an alternate suggestion, given the example current directory of
The lookup logic will have to be applied for each option, but I guess this can be extracted out into a separate function which takes a I suppose this also means that multiple options can be applied even if they come from different keys. For example given the following settings:
Then both the |
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 patch in its current state looks fine.
I guess it's up to you whether you want to implement additional changes like recursive directory options here or separately.
@joelim-work Is there an advantage for keeping the trailing I agree this shouldn't be referred as globs. I think maybe we can call it subdirectory matching or something similar. With this in mind, I also tried using the trailing
I'm not yet sure if this is a good idea. It avoids the use of a special character for this purpose (e.g. |
@gokcehan Apologies I missed the fact I don't know whether a trailing |
Maybe But both are fine, as long as it is explicitly stated in the docs that it only works at the end of the path. |
cc #117 #171
This has been on the back of my mind for quite a while as sorting the downloads directory by time automatically is quite common in other file managers, so I decided to give this a try today. I'm pushing this as a PR to benefit from our usual review mechanism, so discussion is welcomed.
You can try this with the following example:
This is still a draft. Some things that are worth mentioning are as follows:
There were two approaches in my mind before I started working on this, keep the local option values in
dir
or in a global map. First approach seemed a little complicated as we want to be able to set local options even if the corresponding directories are not loaded yet. On the other hand, the second approach requires checking the global map each time an option is used, though I don't expect it to create a performance bottleneck.Adding a new keyword (i.e.
setlocal
) seemed inevitable. I also thought about reusing theset
keyword with an optional directory value, butset
already have an optional field for boolean options, sosetlocal
seemed like the safer choice. It is possible to shortensetlocal
tosetl
but I think it is better to be explicit in this case.I have only added
sortby
so far for testing. In general, there are many options that are not applicable as a local option or they don't make much sense even if they are applicable. So I thought it would be better to add options selectively whenever they make sense. Currently, I'm thinking of only adding options related to sorting and maybe also things likeinfo
initially. I expect these options to cover most use-cases that users have in mind.There is currently no way to remove a local option once it is set. If there is a need for it, I'm not sure how it should work yet.
There could be some issues related to
sortType
. If a local option in this struct is set, then I think other options in this struct will not be updated when the corresponding global option value is changed. I'm not yet sure if this is a niche case or it is worth considering. Also, I think we have some inconsistencies about the included options insortType
. For example, I thinkdironly
could have been added tosortOption
as well. I think the reasonignorecase
andignoredia
is excluded is due to them being used in places other than sorting. I'm not sure abouthiddenfiles
but maybe it could have been added as a separate field tosortType
. I don't remember much about why we introducedsortType
but one possibility is to get rid of it altogether to fix the issue withsetlocal
.