-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Support 'login-items' as a first-class citizen of the CLI #19744
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
base: main
Are you sure you want to change the base?
Conversation
c97b261 to
eaba8a8
Compare
|
Could someone please help? I am not sure if I am going in the right direction or not. Would love to get some feedback. This PR is trying to solve the #19333 ticket. |
7db01f2 to
ca5e83a
Compare
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vraravam Looks good so far! Anything specific you need help with?
Thanks @MikeMcQuaid - I am first trying to cover all scenarios/commands from the CLI perspective, and finally will proceed to the actual implementation of the osascript calls. Hope that's fine with you? Also, I'm trying to ensure that all code that I write have tests - if I miss something (since this is my first time contributing to this codebase, i don't know the contributing guidelines/rules), please do point me in that direction as well. Based on the CLI commands, do you recon that I am missing any? I have tried with |
|
@vraravam Sounds good to me! When CI is 🟢 let us know and we can give this a more thorough review. CC @homebrew/cask folks for any thoughts before that, too. |
| homepage "https://brew.sh/" | ||
|
|
||
| app "Caffeine.app" | ||
| login_items "Caffeine.app" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the dsl here should be login_item, and repeated if there are multiple.
This will keep it consistent with the other artifacts, that aren't pluralised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean like how multiple language blocks are supported in the DSL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referring to other artifacts such as app, binary, artifact, font, qlplugin, etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we look at the everything.rb DSL, there are other stanzas that are pluralized as well maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the artifact stanzas (which I think a login item will most closely relate to) are not pluralised.
suite
app
pkg
installer
binary
manpage
colorpicker
dictionary
font
input_method
internet_plugin
keyboard_layout
prefpane
qlplugin
mdimporter
screen_saver
service
audio_unit_plugin
vst_plugin
vst3_plugin
For context, where multiple items doc the same type are allowed and provided the stanza is repeated rather than passing an array to the stanza.
| ==> Downloading file:.*caffeine.zip | ||
| Already downloaded: .*--caffeine.zip | ||
| ==> Uninstalling Cask local-caffeine | ||
| ==> Skipping processing of login_items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a concern that there may be side effects of skipping the uninstallation of login_items.
I understand having the option to automatically register a login_item, but is the idea here to be able to skip uninstalling them? The reason we generally aim to remove as much as possible with brew uninstall (which also runs with brew upgrade) is to not leave things behind that aren't connected to the application anymore.
What happens if the login_item changes between versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree - I was also facing the same conundrum. This is predominantly for the usecase where the user has chosen to not uninstall by using --no-login-items CLI switch. By default, the registered login items will be uninstalled (I think that's how I have coded it?)
af24c14 to
53af5c6
Compare
53af5c6 to
b5ec545
Compare
73b1bbd to
48bb325
Compare
85caefe to
1999094
Compare
16d8afc to
76e97c0
Compare
fd7ca76 to
047676a
Compare
966b724 to
1a182f5
Compare
962c8f1 to
f0386a4
Compare
…the top-level Fix issues found when running brew cli commands (upgrade, update, install) Incorporate 'login_items' into 'info' command output (not yet verified) Incorporate 'login_items' option for 'reinstall' command WIP: Adding placeholder for using osascript to register/unregister login items Mark for deprecation Trying to fix broken unit test Fix ruby version
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?login_itemfrom under theuninstallblock to the top-levellogin_itemsinto info command output (not yet verified)login_itemsoption forreinstallcommand