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

🍎 Fix wrong app name resolution in pulsar.sh on Mac #252

Merged
merged 2 commits into from Dec 25, 2022
Merged

🍎 Fix wrong app name resolution in pulsar.sh on Mac #252

merged 2 commits into from Dec 25, 2022

Conversation

Soupertonic
Copy link
Contributor

@Soupertonic Soupertonic commented Dec 20, 2022

Aims to fix #251. On the attempt to open Pulsar using the script it would error out with:

The application //Applications cannot be opened for an unexpected reason,
error=Error Domain=NSOSStatusErrorDomain Code=-10827 "kLSNoExecutableErr: 
The executable is missing" UserInfo={_LSLine=4101, _LSFunction=_LSOpenStuffCallLocal}

This error is reported by open.

The script was changed to only perform 3 instead of 4 dirname calls which enables the script to resolve the correct app name. It is unknown whether or not the issue is exclusive to Ventura because it seems like it has worked before. We may have to include MacOS version checks to maintain backwards compatibility.

Additionally, the open call now includes the -g flag. Not using the -g flag would cause MacOS (maybe exclusive to Ventura too?) to select a random app and (re-)focus it before Pulsar is even visible. It is now starting in the background without claiming focus.

For the time being a temporary fix. It is unknown whether or not the 
issue (#251) is exclusive to Ventura because it seems like it has worked 
before. I left a note in the source for that reason. Additionally, the 
open call now includes the -g flag. Not using the -g flag would cause 
MacOS (maybe exclusive to Ventura too?) to select a random app and 
(re-)focus it before Pulsar is even visible. So, for the time being it 
is going to start in the background without claiming focus.
@Soupertonic Soupertonic changed the title 🍎 Fix wrong app name resolution in pulsar.sh on Mac (#251) 🍎 Fix wrong app name resolution in pulsar.sh on Mac Dec 20, 2022
@confused-Techie
Copy link
Member

This overall looks great to me, but since our tests don't cover this part of code, and I'm not personally on mac, I'll reach out to others in the community to see if we can test this.

Thanks a ton for the contribution!

@Soupertonic
Copy link
Contributor Author

I have UTM (vm software) installed and could test it in previous versions of Mac if I can get hold of older Mac versions.

@confused-Techie
Copy link
Member

I have UTM (vm software) installed and could test it in previous versions of Mac if I can get hold of older Mac versions.

Here ya go, use this link to grab older versions of MacOS, but fair warning, the more recent 'older' versions require a download through the app store, so you can test the much older versions though

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Wow, I'm impressed with this. Good catch with the -g flag.

details (click to expand):

The focus-hopping weirdness was an old issue I think that still affects the old Atom code. Without the fix, Pulsar seems to try to grab focus right away (due to the invocation of open, I suppose), then my Terminal.app grabs focus again, then Pulsar uses (I assume) the Electron APIs to grab focus when it's actually ready.

Now my Terminal.app keeps the focus it organically/expectedly had (from me manually invoking the shell script using the Terminal), and Pulsar only grabs focus once, when it's properly ready.

And the main fix, the path adjustment, appears to be correct for finding/launching Pulsar successfully.

Testing notes: I initially tested this by just editing the pulsar.sh in place to match your changes. But I also confirmed by running the pulsar.sh in the binary our Cirrus CI produced for this PR. So, looks good to me.

Related issue?: I bet the "permission error" when trying to install the shell script to the user's PATH... is because the shell script "doesn't exist" at the path we're trying to copy. It's one folder up. So, we can probably fix the related "install bins to PATH" issue by adjusting the path there as well. But I think that's for another PR. EDIT: See #263 for this semi-related fix.

EDIT to add: Yeah, I tested this on macOS 10.15 "Catalina", and it worked for me. 👍

@DeeDeeG
Copy link
Member

DeeDeeG commented Dec 25, 2022

Thanks for this, I'm going to go ahead and merge this!

Appreciate the contribution!

@Soupertonic Soupertonic deleted the fix-macos-app-name-resolution branch December 25, 2022 11:14
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.

Shell integration on MacOS (Ventura) Apple Silicone M1
3 participants