savedra1 / clipse

Configurable TUI clipboard manager for Unix
MIT License
272 stars 10 forks source link

Adding configuration capability for clipse. #32

Closed Pranav-Badrinathan closed 4 months ago

Pranav-Badrinathan commented 4 months ago

Closes #26

Adds the following capabilities:

All the paths are relative to the current file. So if a config file in /a/directory/ defines a historyFile in it as hist.json then the resulting absolute path used by clipse would be /a/directory/hist.json. Paths can be relative to home using ~ and can have environment variables that are expanded upon importing from the config file.

Limitations (things this PR can't do/weird behaviours):

Specify a directory and import all files inside it. This does not support any globing patterns, however that should be easy with the filepath.Glob(...) function. I can add it if it is wanted.

There is a bit of weird behaviour with themes. Defining anything in the sources category in the config file, that file needs a sourceType tag in it for the program to know what kind of file it is parsing. So if the theme is at its default location ($HOME/.config/clipse/custom_theme.json) and how it currently stands (without a sourceType), clipse will complain and skip the file. However, since it is in the default location and if no other theme files are loaded, clipse will fallback on its behaviour before this PR and read the file in the default location. So it leads to that warning being ignorable, and the particular entry in sources for the default theme unnecessary (as with the sourceType tag in the default theme).

Idk what to do here. Easiest is obviously to remove it from the default config, but perhaps we could leverage go's forgiving error system and instead try and parse the file as both theme and config, only emitting an error or warning if none of those successfully complete. This would remove the sourceType requirement entirely, but would require a small re-write of the way these files are currently handled (which is not necessarily a bad thing). I would lean towards the latter as it is easier for the user.

savedra1 commented 4 months ago

Hi @Pranav-Badrinathan :wave:

I love what you've done here! Has been super interesting for me to learn the methods you've used to achieve the new functionality. Has helped me to understand some concepts better so thank you for that.

There is one thing I'm not quite clear on though and it may just be that I'm missing something... Given the $HOME/.config/clipse/config.json has to exist as a starting point anyway, what is the actual benefit/value to providing the option for alternate config files, leading to the added complexity involved? For example. Instead of taking an arbitrary number of sources, having a single config file in the default dir that would look like this:

{
    "themePath": "relative/path/to/themeFile.json",
    "maxHistory": 100,
    "historyFile": "relPathTo/clipboard_history.json",
    "tempDir": "tmp_files"

    ...additional config

}

Do you maybe have an example use case for when splitting the config files would provide value? Examples of what the separate config files json would look like in that context would be helpful for me also.

As I said I haven't personally come across a situation where this would be helpful , so I may just be overlooking something.

I feel like this simplification would not sacrifice any flexibility for future developments and due to your excellent work with allowing relative paths etc, additional configurations could simply be added in as new components/references to new files in the config.json file.

Let me know what you think/what I'm missing :smile:

Pranav-Badrinathan commented 4 months ago

The only real use case I can think of for multiple source files (in the near future) is the ability to switch themes without editing the config (if that is even desirable). It's just you had "Custom config paths" or something similar in your future enhancements list, hence why I added that.

Clipse is still a relatively simple tool, so your proposition will likely be enough regardless of what is added in the future.

Pranav-Badrinathan commented 4 months ago

I suppose if not removing the functionality, we can simplify it to take a theme or an array of themes should that use-case be desired.

Either way, looking at your future enhancements list, I feel like I can really easily also add cli-set custom config paths and custom theme switching. When you run clipse --listen-shell or clipse --listen you can provide a config for it to use that is different than the base config. Or when simply running clipse to access the clipboard, pass in a theme path and use that to render clipse.

savedra1 commented 4 months ago

Thanks for clarifying @Pranav-Badrinathan I do remember you mentioning that now and I understand the use case.

I think from a design perspective I prefer the idea of the config file pointing to a single theme file path, as it saves some complexity in having to recursively read an arbitrarily long array.

The future cli arg added could then take a path for a different file still, but replace the value in the config.json file instead. I'm thinking this would be a separate, explicit command, something like clipse --set-theme themes/a_theme_file.json. Potentially also something like clipse --theme pathTo/a_theme.json could alternatively update a theme for the current instance instead of replacing the path value in the config file.

I will definitely be keeping this method in mind for the future though as I love the creativity of it. Just trying to maintain code simplicity here for my own sanity :laughing:

Are you happy to make this update? I really appreciate your hard work so far and would ofc be happy to pick this up where you left off if needed :smile:

Pranav-Badrinathan commented 4 months ago

I think the clipse --theme pathTo/a_theme.json implementation would not be too difficult to implement. I personally do not see the usecase for --set-theme (you can either call --theme each time if in a script of sorts, or set the theme manually if you want to change your theme with --set-theme anyways. I'm going to be moving over the weekend, so I might not get a lot done then. Will definitely get to it next week otherwise though.

Edit: Oh, and I'll also remove the recursive sources list. It's a cool idea, but I agree it is misplaced here :).

savedra1 commented 4 months ago

@Pranav-Badrinathan agreed.

We are nearly there then, if I get a chance during the week I'll let you know and make the updates directly to this PR.

Good luck with your move!

savedra1 commented 4 months ago

@Pranav-Badrinathan I'm getting some permissions issues in adding my commits here (probably missing something) but just gonna close this and merge in #36 instead :partying_face: