gokcehan / lf

Terminal file manager
MIT License
7.65k stars 325 forks source link

enhancement: LF_CONFIG_HOME to ~/.config/lf #1722

Closed shimeoki closed 4 months ago

shimeoki commented 4 months ago

As I tested, lf tries to use LF_CONFIG_HOME environment variable to find lf folder in it to use other configuration files like lfrc, icons and colors. So the lfrc path is "$LF_CONFIG_HOME/lf/lfrc".

If LF_CONFIG_HOME does not exist, lf will use XDG_CONFIG_HOME instead. So basically XDG_CONFIG_HOME = LF_CONFIG_HOME.

So why not change LF_CONFIG_HOME to navigate not to the usual .config folder, but also to the lf folder? For example, ~/.config/lf, not just ~/.config.

Because of the current situation with these variables, there is no point in using LF_CONFIG_HOME, because XDG_CONFIG_HOME is almost certainly set on every system. Even if it is not, LF_CONFIG_HOME is only used for the lf folder and below, so there I see no reason to "include" the whole .config directory in the configuration.

I suggest this for every environment variable listed here, except for the system or XDG ones. image

joelim-work commented 4 months ago

lf is actually appended to all of these paths when resolving them, so the list of paths is actually:

Which means that the path ~/.config itself is never actually being used.

As for the existence of LF_CONFIG_HOME on Unix systems, I agree it's not very practical since most users will just configure XDG_CONFIG_HOME instead, but this was added to be consistent with Windows (see https://github.com/gokcehan/lf/pull/1253#issuecomment-1555940700).

With this is mind, what is the exact set of changes you are trying to propose?

shimeoki commented 4 months ago

First of all, the story of my use case:

I was rewriting all of my configuration files, and I decided to use environment variables as much as possible to make my config extensible, readable and portable.

As I read in the documentation, lf uses the $LF_CONFIG_HOME environment variable to find the configuration files. And maybe it's just me, but from the documentation I made two assumptions about this part:

Unix
    $LF_CONFIG_HOME
    $XDG_CONFIG_HOME
    ~/.config

    $LF_DATA_HOME
    $XDG_DATA_HOME
    ~/.local/share
  1. $LF_CONFIG_HOME and $LF_DATA_HOME use the same value as $XDG_CONFIG_HOME and $XDG_DATA_HOME.
  2. $LF_CONFIG_HOME and $LF_DATA_HOME are empty by default, and their values should be set manually.

And semantically, I thought that the variable with ...LF_CONFIG... in it should point to the folder containing all the config files of that particular program, like ~/.config/lf, just ~/.config and append /lf to it. I tried setting $LF_CONFIG_HOME to ~/.config/lf and wondered why my config was loading at all.

Because of the current path resolving, lf folder name is hardcoded, and you can't change it whatsoever. One format is great, and I don't think any other program's config should be named lf, but a possibility is a possibility nevertheless.

One scenario I was thinking about is multiple configs in directories like lf-old, lf-preview and so on, if user does not use git or dotfiles manager. In this case, if $LF_CONFIG_HOME serves as full path to folder, containing all of lf config files, user can just change this variable to corresponding directory.

Second, my set of changes I'm trying to propose:

Since I brought up documentation, I think the configuration section should be more explicit for people like me (stupid).

Example:

  1. Use environment variables in the path description of files:
OS       system-wide               user-specific
Unix     /etc/lf/lfrc              $LF_CONFIG_HOME/lfrc
Windows  %ProgramData%\lf\lfrc     %LF_CONFIG_HOME%\lfrc
  1. Write the corresponding path under each variable, instead of combining them. Also, because of my idea, the variables will have different default values (more on this a second later):
Unix
    $LF_CONFIG_HOME
    ~/.config/lf

    $XDG_CONFIG_HOME
    ~/.config

    $LF_DATA_HOME
    ~/.local/share/lf

    $XDG_DATA_HOME
    ~/.local/share

Windows
    %ProgramData%
    C:\ProgramData

    %LF_CONFIG_HOME%
    C:\Users\<user>\AppData\Local\lf

    %LOCALAPPDATA%
    C:\Users\<user>\AppData\Local

And the main point of the change: environment variables with ...LF... in them should point to the exact folder of the configuration (or data) directory, instead of appending lf to it. Reasons are explained in my use case.

Disadvantages of this solution:

  1. Change for the change's sake. It just works, and is not that important for the functionality of the program.
  2. If someone has configured these variables in their setup with the current value, they need to change it. This will cause a lot of trouble, I think, because the reason for "config is not loading" is not so clear.

To fix the second drawback, lf can show a message like lfrc is not found on launch to inform the user. Maybe this would be good even without changing environment variables.

joelim-work commented 4 months ago

You raise a number of points, and below are my responses:

  1. $LF_CONFIG_HOME and $LF_DATA_HOME use the same value as $XDG_CONFIG_HOME and $XDG_DATA_HOME.
  2. $LF_CONFIG_HOME and $LF_DATA_HOME are empty by default, and their values should be set manually.

And semantically, I thought that the variable with ...LF_CONFIG... in it should point to the folder containing all the config files of that particular program, like ~/.config/lf, just ~/.config and append /lf to it. I tried setting $LF_CONFIG_HOME to ~/.config/lf and wondered why my config was loading at all.

Just to be clear, the actual implementation is somewhat more simpler:

https://github.com/gokcehan/lf/blob/12e99fdb641565e3122ab62dce0b77e836aa69a4/os.go#L87-L98

The base path is the first of LF_CONFIG_HOME, XDG_CONFIG_HOME and ~/.config that is not unset/empty. From there /lf/lfrc is appended and that becomes the final resolved path for the config file. The same process is used for resolving the data directory (base path uses the list LF_DATA_HOME, XDG_DATA_HOME and ~/.local/share).

I think by now you are aware of this.

Because of the current path resolving, lf folder name is hardcoded, and you can't change it whatsoever. One format is great, and I don't think any other program's config should be named lf, but a possibility is a possibility nevertheless.

From a theoretical perspective, it is certainly possible for someone else to create another application called lf which uses the same config file paths, thereby causing a conflict. But the same problem exists for just about every other application out there, and in practice people will generally choose a name that hasn't been used yet when developing a new application. Consequently, I don't see this as much of a concern.

One scenario I was thinking about is multiple configs in directories like lf-old, lf-preview and so on, if user does not use git or dotfiles manager. In this case, if $LF_CONFIG_HOME serves as full path to folder, containing all of lf config files, user can just change this variable to corresponding directory.

Disadvantages of this solution:

  1. Change for the change's sake. It just works, and is not that important for the functionality of the program.
  2. If someone has configured these variables in their setup with the current value, they need to change it. This will cause a lot of trouble, I think, because the reason for "config is not loading" is not so clear.

So it seems like this is the main reason why you raised this issue in the first place, to allow for multiple sets of config files? I think I understand your use case here, but it does feel somewhat niche to me. For a long time the default config path used by lf was based off XDG_CONFIG_HOME, which is a standard for users to set the location used by all programs, not just lf. This works fine for most users, because they typically only need to manage one set of config files.

LF_CONFIG_HOME was added last year in #1253, and that was just because of a Windows user who wanted to avoid LOCALDAPPDATA and use a more Unix-like location instead. This would also not have considered the use case of having multiple sets of config paths like lf-old and lf-preview and being able to switch between them.

Your suggestion of modifying LF_CONFIG_HOME to include the /lf path component changes its semantic meaning from "override of XDG_CONFIG_HOME" to "directory containing the lfrc file". Such a change may end up being confusing as to whether the /lf path component is included or not, and in any case is a breaking change that will impact other users that already use LF_CONFIG_HOME. At this point I'm not really convinced that this kind of change is worth making, although it just my personal opinion.

I wonder if it's possible for you to be able to manage multiple sets of config files using other means. If it's just using a different lfrc file, you can specify it using lf -config <path>. Otherwise you could try using a dotfiles manager, or symlink ~/.config/lf to point to a custom location.

Since I brought up documentation, I think the configuration section should be more explicit for people like me (stupid).

Example:

  1. Use environment variables in the path description of files:
OS       system-wide               user-specific
Unix     /etc/lf/lfrc              $LF_CONFIG_HOME/lfrc
Windows  %ProgramData%\lf\lfrc     %LF_CONFIG_HOME%\lfrc

Regarding the documentation, if you find anything that is unclear, you are more than welcome to submit a PR to address it.

However the documentation should list all the paths involved. For instance the above example doesn't mention the fallback paths of XDG_CONFIG_HOME and ~/.config at all, and may also suggest to users that it is mandatory to set LF_CONFIG_HOME when it is not.

To fix the second drawback, lf can show a message like lfrc is not found on launch to inform the user. Maybe this would be good even without changing environment variables.

The current implementation simply processes config files if possible but doesn't report any issues otherwise:

https://github.com/gokcehan/lf/blob/12e99fdb641565e3122ab62dce0b77e836aa69a4/app.go#L266-L270

Note that gConfigPaths contains the system config path (/etc/lf/lfrc) in addition to the user config path. I think most users don't have any system-wide configuration, and on top of that some users may not even have a user config file as they are satisfied with the defaults, so I'm not sure if adding a warning will end up being more helpful than annoying. Normally it should be very easy to see if a config file doesn't get processed (e.g. some visual aspect like promptfmt doesn't get applied).

shimeoki commented 4 months ago

Thank you for responding in such detail.


Your suggestion of modifying LF_CONFIG_HOME to include the /lf path component changes its semantic meaning from "override of XDG_CONFIG_HOME" to "directory containing the lfrc file".

I was so slow to make this correlation, I am sorry. I was too focused on the LF_CONFIG part of the variable, not on the CONFIG_HOME. This variable was supposed to act like XDG_CONFIG_HOME, and I have only just got it now. Due to the circumstances with Windows it's perfectly logical to name this variable that way, and I wasn't considerate of that.

With that in mind, I fully agree with you and take back this proposed change.

But, maybe something like $LFDOTDIR is viable, as$ZDOTDIR in zsh?

It would serve the purpose of the change (adding an environment variable with the exact directory of all of the configuration files), and wouldn't interfere with the current set of environment variables.

But, maybe this is overkill? As you mentioned, the use case of multiple sets of configuration folders for the same program is quite niche. Personally, I will not be doing this (there are dotfiles managers). It just came to mind as an example of why having the ability to do so can be helpful. A much more realistic use case is something like structuring configurations in patterns like ~/.config/file-managers/lf/... for whatever reason.

Also, this variable will complicate the code significantly. That's my version:

// i don't really like the naming of this slice,
// but i haven't called it like "userEnvs",
// because these "envs" are not "processed" yet to
// get the actual values of them
userEnvNames := []string{
    "LFDOTDIR",
    "LF_CONFIG_HOME",
    "XDG_CONFIG_HOME"
}

// maybe pass slice as argument,
// to not access it from global scope
func getUserConfigPath() string {
    path string

    for _, name := range userEnvNames {
        path = os.Getenv(name)

        // this loop should result in setting blank "path",
        // if all variables are empty
        if path == "" {
            continue
        }

        // in terms of extensibility,
        // this statement should be changed to
        // strings.HasSuffix(name, "_CONFIG_HOME")
        if name != "LFDOTDIR" {
            path = filepath.Join(path, "lf")
        }

        // breaking the loop on the first found non-empty path
        break
    }

    // fallback
    if path == "" {
        return filepath.Join(gUser.HomeDir, ".config", "lf")
    }

    return path
}

/// ...

config := getUserConfigPath()

gConfigPaths = []string{ 
    filepath.Join("/etc", "lf", "lfrc"), 
    filepath.Join(config, "lfrc"), 
}

I think it's better to create several functions to distunguish between getting the path and using it afterwards, because right now it's just if statements in the init() function.

The downside is that this solution reduces performance. Probably negligible, but I think I should have pointed it out.


About the documentation.

However the documentation should list all the paths involved. For instance the above example doesn't mention the fallback paths of XDG_CONFIG_HOME and ~/.config at all, and may also suggest to users that it is mandatory to set LF_CONFIG_HOME when it is not.

Haven't I done that in second point?

Unix
    $LF_CONFIG_HOME
    ~/.config/lf

    $XDG_CONFIG_HOME
    ~/.config

...

In this case, the part with the variables can be placed at the beginning of the configuration section, and these variables can then be used. This may be easier to read.

And to be even more explicit, we can specify that these values are defaults:

These variables are used to set configuration locations. The first non-empty variable is used.

Unix
    1. $LFDOTDIR
    default: ~/.config/lf

    2. $LF_CONFIG_HOME
    default: ~/.config

    3. $XDG_CONFIG_HOME
    default: ~/.config

...

Configuration files:

lfrc // style as heading, probably

system-wide
    Unix
        ...

user-specific
    Unix
        1. $LFDOTDIR/lfrc
        2. $LF_CONFIG_HOME/lf/lfrc
        3. $XDG_CONFIG_HOME/lf/lfrc
        fallback: ~/.config/lf/lfrc

And once again, this is probably overkill. Also sorry for the bad formatting, I was using tabs.


About the config error message.

Okay. Adding a configuration parameter like silence-config-warning is probably not worth it, so I agree.

joelim-work commented 4 months ago

Regarding LFDOTDIR, this is essentially a feature request.

I don't think there's any issue with the performance (I doubt it would be noticeable to users), but my main concern is whether lf really needs such a feature. I have no interest in using it myself, and you have also said that you could use a dotfiles manager instead. There's no point in adding a feature just because it might be useful - in fact it makes the experience for new users worse, as they will now waste time reading about LFDOTDIR and probably end up deciding that XDG_CONFIG_HOME is all they need. Granted, LFDOTDIR by itself is a relatively small feature, but features accumulate very quickly to the point of being overwhelming.

I'm not particularly interested in discussing LFDOTDIR much further, but if you insist on its usefulness, you can leave the issue open and see if anyone else is interested.


As for the documentation, the problem with the following is that it can be a bit misleading:

Unix
    $LF_CONFIG_HOME
    ~/.config/lf

    $XDG_CONFIG_HOME
    ~/.config

Here, you imply that LF_CONFIG_HOME and XDG_CONFIG_HOME are resolved separately and have their own defaults, when that is not the case. Something like this might be more clear:

Config file should be located at <config_dir>/lf/lfrc, where <config_dir> is determined from the following list (in order of decreasing priority):

  • $LF_CONFIG_HOME
  • $XDG_CONFIG_HOME
  • ~/.config

This is just an example of course, I wrote it in a rush to keep it simple - hopefully you get the idea. Anyway, you are of course welcome to submit a PR to clarify the documentation.

shimeoki commented 4 months ago

About LFDOTDIR:

... but if you insist on its usefulness, you can leave the issue open and see if anyone else is interested.

No, I will close this issue. I have basically convinced myself with several mentions of "overkill" regarding this feature, that it is not needed, and you have also agreed on this. I just wanted to share the idea, as someone might find this useful. Closing this issue will make it a little more difficult to find, but it is still easy with the keyword "config".

Also, as you mentioned,

... this is essentially a feature request.

so a separate issue or discussion should be created for further analysis. But I am not particularly interested either - I got an answer to my original question. Thank you.


As for the documentation, the problem with the following is that it can be a bit misleading: ...

... you imply that LF_CONFIG_HOME and XDG_CONFIG_HOME are resolved separately and have their own defaults ...

Yes, I just copied what I wrote in the second comment (as a quote), so there LF_CONFIG_HOME is different from XDG_CONFIG_HOME. In practice, both should be ~/.config, so the defaults are the same.

And your variant is great! I really like it, maybe I will submit a PR with this kind of style sometime.