Skip to content
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 config command #112

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from
Draft

Conversation

SIMULATAN
Copy link

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!

@SuperCuber
Copy link
Owner

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

@SuperCuber
Copy link
Owner

SuperCuber commented Jan 14, 2023

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.

@SIMULATAN
Copy link
Author

SIMULATAN commented Jan 14, 2023

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
Copy link
Author

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.

@SuperCuber
Copy link
Owner

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

@SIMULATAN
Copy link
Author

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

I also rebased onto master (interesting commits btw 😂).
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.

@SuperCuber
Copy link
Owner

Make sure you test dependency cycles :)

@SIMULATAN
Copy link
Author

New PoC 🥳

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 ❤️

@SuperCuber
Copy link
Owner

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?

@SIMULATAN
Copy link
Author

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.

@SuperCuber
Copy link
Owner

That would be great :D

@SIMULATAN
Copy link
Author

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

@JaydenElliott
Copy link

Any update on this @SIMULATAN ?

@SIMULATAN
Copy link
Author

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
Copy link
Author

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.

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.
This involves switching to SIMULATAN's fork of dialoguer.rs, which was extended with advanced features required in dotter.
@SIMULATAN
Copy link
Author

SIMULATAN commented Sep 18, 2023

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:

  • colored output
  • 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.
  • 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
Copy link
Author

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 😅

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

@SuperCuber
Copy link
Owner

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 :)

@SIMULATAN
Copy link
Author

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.

@SuperCuber
Copy link
Owner

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

@SIMULATAN
Copy link
Author

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...

Copy link
Owner

@SuperCuber SuperCuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

back to life.gif

I am back :D

I have cleaned up some stuff and made some changes to the implementation, I haven't tested it enough though, hopefully I haven't broken anything 😬

I added some TODOs, and also we should write a test or two for the dependencies function.

This looks really good :)

Cargo.toml Outdated
@@ -26,6 +26,7 @@ shellexpand = "2.*"
simplelog = "0.12.*"
tokio = "1.*"
toml = "0.4.*"
dialoguer = { git = "https://github.com/SIMULATAN/dialoguer", features = [] }
Copy link
Owner

@SuperCuber SuperCuber Feb 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crates.io does not allow git dependencies :(
We might have to get your changes merged into dialoguer, is there a reason you haven't opened a PR?

Copy link
Author

@SIMULATAN SIMULATAN Feb 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason is that I kinda just haven't gotten around to it, especially since it wasn't fully approved yet.

However, that isn't the only problem:
My changes are vastly different from what the library usually stands for. While the other components aren't exactly configurable, this one very much is and thus seems a bit out of place (and therefore possibly out of scope).
Additionally, the code is kinda messy and the change is pretty large (~500 lines, although the majority of that is because I created a separate multi_select_plus component rather than editing the existing one).
Let's just say, I'm not exactly confident in my rust skills (I mean, just looking at your cleanup, it appears I made quite a few mistakes 😅)
To get an idea of the changes, see the diff

Considering that reasoning, I'll try to see if I can get something going that would have a chance at getting merged upstream, both in terms of code quality and "genericness". As I said a while back, right now, it's very opinionated, having only what's necessary for this feature to work (in particular, the callback feature).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'm sure dialoguer's maintainer will have comments to steer you in the right direction :)

Please link the PR here when you open one

@SIMULATAN
Copy link
Author

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?

@SuperCuber
Copy link
Owner

I've done the TODOs :)

@SIMULATAN
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants