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

feat(cli): make --config optional for enable-autostart #596

Merged

Conversation

amrbashir
Copy link
Contributor

No description provided.

@LGUG2Z
Copy link
Owner

LGUG2Z commented Nov 25, 2023

Can you check if autostarting without using the static config works for you? The last time we looked into autostarting with the dynamic config format it was too unreliable with a high reported failure rate; that's why I kept the autostart command limited to static config in the last release.

@amrbashir
Copy link
Contributor Author

I am not sure what you mean by the dynamic config format but I have komorebi.json and applications.yaml and both seem to be picked up correctly.

@LGUG2Z
Copy link
Owner

LGUG2Z commented Nov 25, 2023

if let Some(config) = arg.config {

If the --config arg isn't passed to the start command, the komorebi.json file isn't loaded by komorebi.exe, but if the user has either komorebi.ps1 or komorebi.ahk (ie. what I refer to as the dynamic config formats) in their KOMOREBI_CONFIG_HOME, these will be loaded; can you double check that it isn't one of these latter ones that is being loaded for you?

From the linked line above I can't see how a static komorebi.json configuration file would be loaded without the --config flag.

@amrbashir
Copy link
Contributor Author

amrbashir commented Nov 25, 2023

That make a lot more sense since I actually wanted to figure out why some of the options I have don't work while others work like workspaces options because I have 5 workspaces defined but I guess that is just the default.

Creating a komorebi.ps1, based on the sample in this repo, in $Env:USERPROFILE seems to get picked up just fine for me. Also running komorebic log quickly before the shortcut in shell:startup is executed logs the correct values from komorebi.ps1

If I were to guess why it failed for some people, they may have set KOMOREBI_CONFIG_HOME in powershell profile but forgot to set it in the Windows environment variables panel so when autostarting from the shortcut in shel:startup, it won't detect the variable and won't load the dynamic configurations. I use KOMOREBI_CONFIG_HOME and ran into this same issue so I had to paste the samples in the default $Env:USERPROFILE

@amrbashir
Copy link
Contributor Author

Now if --config is really needed, why not auto-detected same as other options? I would be happy to open another PR for this.

@LGUG2Z
Copy link
Owner

LGUG2Z commented Nov 26, 2023

I think we can move to auto-detecting komorebi.json as well, or at least attempting to. The --config flag was introduced so that the loading from a static configuration file could be more portable than loading from a ps1 or an ahk file. I guess there is still this culture in Windows of people putting "portable" versions of things onto thumb drives and moving them from machine to machine, which is where the --config flag would come in.

But for the rest of us *nix types, the autoloading too makes sense. I'm a little tired of typing out the path all the time. 😅

If I were to guess why it failed for some people, they may have set KOMOREBI_CONFIG_HOME in powershell profile but forgot to set it in the Windows environment variables panel so when autostarting from the shortcut in shel:startup, it won't detect the variable and won't load the dynamic configurations. I use KOMOREBI_CONFIG_HOME and ran into this same issue so I had to paste the samples in the default $Env:USERPROFILE

I think this sounds right, I can't believe I didn't think of this before. 🤦‍♀

@amrbashir
Copy link
Contributor Author

amrbashir commented Nov 26, 2023

I agree the --config flag is necessary and I also think komorebi should start deprecating dynamic config in favor of static (but that's really your choice, I don't know how many people use them) because it will always be faster than the dynamic ones and it should be autodetected by default (which I can open a PR for it next weekend probably if you haven't gotten to it by then).

@LGUG2Z
Copy link
Owner

LGUG2Z commented Dec 2, 2023

I'll add the attempted automatic lookup for the komorebi.json file over the weekend and then I think we should be good to merge this PR in after that 🤞

LGUG2Z added a commit that referenced this pull request Dec 2, 2023
This commit makes the --config flag on komorebi.exe optional, and
updates the configuration loading logic to try to find a komorebi.json
file in the HOME_DIR, which is either $Env:KOMOREBI_CONFIG_HOME or
$Env:PROFILE.

This unlocks the way for Amr's PR to make the --config flag optional on
the enable-autostart command.

re #596
@LGUG2Z
Copy link
Owner

LGUG2Z commented Dec 3, 2023

@amrbashir I made the changes in komorebi - I can merge this branch once you have time to resolve the conflicts 🚀

@LGUG2Z LGUG2Z merged commit 7078b06 into LGUG2Z:master Dec 3, 2023
2 checks passed
@LGUG2Z
Copy link
Owner

LGUG2Z commented Dec 3, 2023

Actually nevermind I was able to squash this to avoid fixing conflicts with the rebase 🎉

Thanks for all the work you put into these two PRs! 🙏

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.

2 participants