-
Notifications
You must be signed in to change notification settings - Fork 310
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 many, many plugins and settings #102
base: master
Are you sure you want to change the base?
Conversation
And so that it always wipes out all items.
The previous `show_delay` was incorrect. It was changing the speed of the transition, not the delay before the transition started. I left `show_delay` in there and mapped it to the (incorrect) `auto_hide_speed` for backwards compatibility but removed reference to it from the README.
There's quite a bit of duplication among many of these functions. I'm going to add a few helper functions to clean some of it up. It will make adding new plugins easier as well as make the user experience more consistent. Not to mention cutting down on possible bugs.
This was much too prone to manual error. For now I just went through the commands that were predominantly 'defaults write' commands as that what I'm currently the most focused on. I also removed the duplication of the restarting of processes and made it happen after any command (which has been given a value) has been run.
@rgcr thanks. :) Unfortunately I've spent way too much time on this already. I ran most of the commands against a fresh OS install of Sierra and it was about a 90% success rate. Which may mean that some of the settings need version checks (100% of them worked in 10.10). I wish I could contribute further, but someone else is going to need to run it over the finish line. |
Additionally, some review will be needed to decide which of these commands need which process to be killed. I run this as a one time thing after a fresh OS install and so all I do is restart. I've never taken the time to try to make it work after each individual command is run. |
Don't worry, I'll work on it as soon as I can |
This PR is 9 months old, time to give birth :) |
I'm going to work on this, but I think we should keep it as simple as we can, I mean every 'plugin' should be independent of each other, so I will implement all plugins of this PR but in a simplest way, I think there are a lot of helper functions that are not necessary. |
@rgcr I could help you with separating those into PRs if you can provide a base those plugins should use, e.g. necessary helpers etc. |
@shpoont, That would be awesome and very helpful, I would like to remove just the dependency of |
@rgcr sounds good, let me know when its ready? I'll then pick one of the plugins and make a test PR |
@rgcr I really don't understand why you would want to remove the "dependency" of those files. You have (in the current codebase) many multiples of places where those exact things are happening. Which means you have many places where you have the possibility of a bug. In fact, when I went through and refactored, I found many bugs where code had been copy/pasted wrong or a bug had been fixed in one place, but not fixed in the other half dozen places where that "same" code was being used. Using those Not to mention, when you remove all the parsing complexity, it makes the actual plugins themselves a lot easier to read and understand. |
Just as an example: BeforeLines 27 to 57 in ba3af98
Afterhidden_files(){
value="$(_mcli_defaults_yes_no_to_boolean "com.apple.finder" \
"AppleShowAllFiles" \
"$1")"
echo "${command} ${subcommand}: ${value}"
} |
@rgcr This is really awesome. What kind of work would you like to see on it to get it into a merge-able state? |
I'd like to pitch in with getting this stuff in. I agree with @thegranddesign that this would remove a lot of my need to refer to various dotfiles. Just need a little guidance on direction from @rgcr. |
@bensleveritt go ahead, at this moment I don't have enough time work on it, just resolve the conflicts and be sure everything works as expected even the completions. |
Looking at all this, I'm tempted to break this into lots of little PRs. If you're happy to accept the utilities lib as the first PR @rgcr, it'll enable me to go through each plugin and supply any changes that @thegranddesign did. This'll make reviewing a lot easier, and technically safer. Are you happy with that approach? My biggest reservation is losing credit for @thegranddesign for all the work they did. |
@bensleveritt, Yes I think that is the best approach. And don't worry I'll update the README to give credit to him and you. 😉 |
@bensleveritt trying to break this up into multiple PRs is going to be a nightmare. There a ton of extraction to create the helper functions. The commits are already in "story order". I don't see how creating a bunch of PRs makes it any more manageable. Just look at the commits in order and approve them. |
@thegranddesign Haa.. You're not wrong about it being a lot of work, but the fact this PR has been stuck for over two years is a good indication that it's asking too much of folks to review. You yourself recognize it's bad form, and yet still expect others to do the work. Since this PR, there have been other submissions for the same or similar functionality, and there are conceptual conflicts, as well as the obvious technical conflicts. My intention is to reduce all of your changes into plugin-specific PRs, so if any parts aren't accepted, the others still can be. It's important that discussion happens in the context of the changes. To be clear, the work you've committed is laudable, I'm just breaking it into more acceptable chunks. |
@bensleveritt To be clear, I don't expect anyone else to do the work, I simply said I had already spent multiple weeks on the code as well as writing commits to make it easy to review. And that I wasn't going to take more time to split it up. This could have been merged as it was with no additional work on anyone's part (other than review) 2.5 years ago. This PR caused almost zero breaking API changes at the time. It could have easily been merged and then tweaked post-merge. For my part I put it up here to give back, and it's been disappointing not seeing it get merged, but my fork works great for everything I use it for and that's good enough for me, and anyone else who wants to use that instead. I definitely wish you good luck. |
@thegranddesign as you wrote, that was a lot of work, unfortunately as you I didn't have too much time to review the PR at that time , I appreciate your work and the time you spent on this. @bensleveritt thank you for being doing the hard work here. Once all changes are reviewed and merged I'll close this PR |
@rgcr Any update on this? |
Ah, it's been a while since I've used |
To this end, I've got a board of progress started https://github.com/bensleveritt/m-cli/projects/1 which I'll update as I move the plugins across. |
This is a fairly massive PR, which isn't exactly good form, but:
My main reason for wanting to get these into
m-cli
is simply because I wanted them out in the community so that fixes could be done in one place rather than in 90,000 dotfiles strewn across Github.These changes focus on
defaults
style commands. I've spend dozens (maybe hundreds) of hours collecting these over the years such that I can now boot up a fresh OS install, run a script, reboot and it's set up exactly as I'd like. Hopefully now that these are all out in the community, we can stop doing these in our own silos and instead combine our resources.There are things about this PR that could be improved (such as the way the "current" value of some of these commands are output) however (not counting the untold hours I spent actually finding which plist files changed when a setting changed), I've already spend three full days converting my old scripts to
m-cli
and I can't devote any more resources to this.I definitely think it's mergable. I cause no (that I know of) breaking changes so it should be a minor Semver release.