dotter icon indicating copy to clipboard operation
dotter copied to clipboard

Add config command

Open SIMULATAN opened this issue 2 years ago • 24 comments

This command can be used to configure the packages in the local.toml file. This is particularly useful when bootstrapping new devices where you quickly want to select what packages to install.

I also plan on creating a more feature rich TUI for the local config including changing variables, but I wanted to get some feedback first before making deeper changes.

Please review this change and tell me what to improve as some things I changed probably should be done in another way (most notably, making a few of the config structs + fields public).

Notes

  • This introduces a new dependency, dialoguer, which adds around 100 KB to the release binary (which is 16 MB at the moment)
  • I had to make a few of the structs in config.rs public to access them in the new file (config_local.rs). I wasn't sure if I should add the command to the existing config.rs file as that one was already quite long even before my changes

I'd be happy to hear feedback and improve this pr based on it. Thanks for your work on this project!

SIMULATAN avatar Jan 09 '23 09:01 SIMULATAN

Sounds like a good idea. I'll check this out over the next couple of days and give feedback :)

SuperCuber avatar Jan 09 '23 11:01 SuperCuber

Couple thoughts:

  • dotter init can be deleted as it's just a worse version of this
  • A simple list of checkboxes doesn't capture the depends relations between the packages. For example I have bash and zsh packages depending on a shell package. I think ideally if I check bash or zsh, the shell package should get checked, and if I uncheck shell then bash and zsh should get unchecked. Maybe to make it more obvious there should be a note near them on hover, saying what the consequences of enabling/disabling this checkbox are. At the same time, I feel like the packages that get depended on shouldn't be written into the final config. Maybe dialoguer doesn't have the features to do this though :(

I'm curious what your ideas are for editing variables.

SuperCuber avatar Jan 14 '23 12:01 SuperCuber

Hi, thanks for the review!

  • init does have the advantage of automatically detecting files and adding them to a default package which can help new users start out with configuring, but I totally agree that it's not too useful. Maybe we should re-make the command as a interactive TUI that walks you through a basic setup with variables or something? If wanted, I can look into that (in a separate PR though)
  • Looking at the docs, there indeed doesn't seem to be a way to programmatically set the checked state of items while rendering menus or even listen for item tick events. My proposal would be to render a "tree-view" of the items which would allow us to show relations whilst also making it easier to code since we don't have to remove the dependencies from the final elements written to the config.

I'm currently trying to get a PoC for a tree display working (that shows the dependends of a package), I'll send an example screenshot in a second

EDIT: PoC of the tree view, the issue right now is that when a package that depends on another package is enabled the package dependent on obviously is also enabled and therefore checked in the tree screenshot of the poc (third depends on test, other has no relations)

SIMULATAN avatar Jan 14 '23 13:01 SIMULATAN

This implementation is rather terrible as it doesn't even permit more than one layer of depth for the dependencies, it's just to demonstrate how the tree view could look like. An additional issue is that there's no way to determine if a package is enabled because it's explicitly mentioned in the local.toml or because it's the dependencies of another package.

SIMULATAN avatar Jan 14 '23 15:01 SIMULATAN

there's no way to determine if a package is enabled because it's explicitly mentioned in the local.toml or because it's the dependencies of another package.

Pretty sure that would be local_config.packages - those are only the explicitly enabled ones.

Also, please merge/rebase on master since I fixed some CI failures

SuperCuber avatar Jan 14 '23 16:01 SuperCuber

Indeed, local_config.packages exists... Why didn't I use that previously??

I also rebased onto master (interesting commits btw :joy:). The only thing left now from my side would be to generate the tree with recursion so that the depth can be greater than 1 package.

SIMULATAN avatar Jan 14 '23 17:01 SIMULATAN

Make sure you test dependency cycles :)

SuperCuber avatar Jan 14 '23 17:01 SuperCuber

New PoC :partying_face:

I changed it to display the root packages at the first layer and print the dependencies of it in a tree, but without items (the text is just added to the root package name but with newlines)

PoC with a simple config

As requested, it also checks for dependency cycles, and now, it actually only shows packages as ticked if they're enabled in the local config (excluding packages enabled due to dependents) PoC with a complex config + cyclic dependencies

Please let me know what you think of this version and what to improve on, thanks :heart:

SIMULATAN avatar Jan 19 '23 11:01 SIMULATAN

I'm back from the underworld! I think this is going in the right direction but it's not what I imagined. If I think about package managers I know, they have packages either "installed", "not installed", or "installed as dependency" which is a weaker version of installed. If nothing depends on a package and it is "installed as dependency" then it is cleaned up.

I think the UI should reflect it in terms of interactivity. Simulating using an editor but I imagine something like: and then checking it turns into and

What do you think?

SuperCuber avatar Mar 22 '23 21:03 SuperCuber

That looks great!

However, that would probably require either forking the dialoguer library and adding support for these features or just using another library altogether. If you want to, I can look into that, but it'll probably take me at least a week or more.

Looking at the attached screenshots, I completely agree that your proposal looks way better and is also more user friendly.

SIMULATAN avatar Mar 23 '23 06:03 SIMULATAN

That would be great :D

SuperCuber avatar Mar 23 '23 16:03 SuperCuber

Guess I know what'll keep me occupied for the next weeks :sweat_smile: I'll keep you updated!

SIMULATAN avatar Mar 23 '23 20:03 SIMULATAN

Any update on this @SIMULATAN ?

JaydenElliott avatar Apr 27 '23 03:04 JaydenElliott

Life's stressful ATM so I didn't manage to find any time to work on this unfortunately. However, if this is a feature multiple people actually want, I'll try to get some work done soon.

SIMULATAN avatar Apr 27 '23 04:04 SIMULATAN

After literally 3 hours of programming, I managed to migrate the view to be a list rather than tree. No clue why it took me so long 😅 Anyway, this is quite a work-in-progress version and the maximum that can be done without forking which is what I'll tackle next.

SIMULATAN avatar Apr 27 '23 09:04 SIMULATAN

guess who's back, back again

I'm back!!!! After a "short" break of... 5 months.... I finally got around to working on this again. I went ahead and updated the branch, did some cleanups and refactorings (I genuinely wonder what I was thinking when I wrote the code, it's so easy to shorten it down and improve it).

Now, time for the fancy stuff. The required fork is now live, so far, I added support for custom summary names, which avoids showing the dependencies of the selected packages in said summary. Things I'll do next:

  • [x] colored output
  • [x] custom mark types / names
    • since I want to do it as generic as possible to ultimately maybe contribute the module upstream, I might use traits here to allow consumers to programmatically set the current state. This system will replace the checked field.
  • [x] item selected callback / event handlers
    • this allows consumers to react to changes programmatically
    • required to set the ticked status of dependencies
    • it's unclear if the PR will be accepted considering the heavy modifications needed for this to work

SIMULATAN avatar Sep 18 '23 13:09 SIMULATAN

I did a thing... FINALLY, after 9 months of programming, I think I got it down. The TUI now works as @SuperCuber requested. demo of the TUI

For my part, I'm done with the code changes now. If anyone has the time, I'd appreciate a PR review, as this is like my first time working with rust :sweat_smile:

Once the changes are approved by @SuperCuber, I'll create a new version in my dialoguer fork to make the CI pass.

SIMULATAN avatar Sep 28 '23 08:09 SIMULATAN

Looks amazing! I'll have a look through the code once I have some free time, in the meantime please run cargo fmt and fix cargo clippy's warnings :)

SuperCuber avatar Sep 30 '23 12:09 SuperCuber

Damnit, I ran those commands at least 40 times while developing. Before committing, my clippy somehow broke and showed an old state with invalid issues so I figured it was fine... it was not Anyway, apart from the publish CI, which as said before I'll do once you give the OK.

SIMULATAN avatar Oct 02 '23 08:10 SIMULATAN

Sorry for the lack of activity, there's a lot of life stuff happening (especially the whole situation in Israel, where I live)

SuperCuber avatar Oct 29 '23 13:10 SuperCuber

Sorry for the lack of activity, there's a lot of life stuff happening (especially the whole situation in Israel, where I live)

Zero obligation to say sorry here. In a situation like this, your "well being" easily takes 1st priority (or 0 since you're a programmer :P). We have to thank you for your past and (hopefully) future work and for doing this all for realistically nothing in return.

Stay safe out there! From the bottom of my heart, and I think I speak for the whole FOSS community here; the best of luck to you and your loved ones 🙏 May the war and the cruelty in this world be over soon...

SIMULATAN avatar Oct 29 '23 21:10 SIMULATAN

Welcome back! Good to see you're alive and well :)

Thanks for the updates. I saw you added a good few TODOs - what's your plan regarding those, will you work on that yourself or do you want me to take a look at it? I haven't worked with rust in a while (and even back then, frankly, I sucked pretty bad lol), so it'll take a bit of time to get back into it, but I can absolutely do that if it helps.

The tests I can absolutely write, are you planning on making more breaking changes soon or do I just go for it?

SIMULATAN avatar Feb 17 '24 17:02 SIMULATAN

I've done the TODOs :)

SuperCuber avatar Feb 19 '24 12:02 SuperCuber

@SuperCuber created https://github.com/console-rs/dialoguer/pull/303. Let's hope it'll get merged soon :)

SIMULATAN avatar Feb 20 '24 17:02 SIMULATAN