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

Update macOS window title logic #49

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

ErikBjare
Copy link
Member

@ErikBjare ErikBjare commented Feb 1, 2021

Continuing on #40

Issues

ianobermiller and others added 9 commits February 14, 2018 23:07
- Switch from the opaque scpt file to a normal JS file using JavaScript for Automation.
- Grab the first window (which is the active one) and get its name as the title. This worked across all apps I tried, including Sublime, Atom, Chrome, Safari, and iTerm
- Clean up the api to the macos.py file (have it return the dict directly)
- Fix a typo in README
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 1, 2021

This pull request introduces 2 alerts and fixes 1 when merging 8c33a3e into ff08c9c - view on LGTM.com

new alerts:

  • 2 for Unused import

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@ErikBjare ErikBjare requested a review from xylix February 1, 2021 13:46
@ErikBjare
Copy link
Member Author

Hey @xylix I made some progress here and seem to have tracked down the issue as to why window titles aren't being returned (screen recording permissions needed...)

Would you be interested in (and have the time) to continue work on this in the near future? I don't think it should be that much work now that I found the core issue.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 1, 2021

This pull request introduces 6 alerts and fixes 1 when merging 9af9993 into ff08c9c - view on LGTM.com

new alerts:

  • 4 for Unused import
  • 1 for Unused local variable
  • 1 for Unreachable code

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@xylix
Copy link
Contributor

xylix commented Feb 1, 2021

I might have time in a couple weeks... can't really promise I will, though. It is probably pretty doable in a few hours of work at this point.

On the fork / subprocess stuff: I've coded some other python multiprocessing and came across fork crashing things on macOS. Only being able to use fork with pyinstaller seems like a quite difficult limitation to work around 🤔

@ErikBjare
Copy link
Member Author

ErikBjare commented Feb 2, 2021

Understandable, just checking :)

tbh I think it might sense to make a release supporting both the new and old method (with the old as default, for the time being), so we can get something basic merged and go from there.

Re fork crashing when run by PyInstaller: I'm hoping that the Python docs are wrong/outdated here and will work fine, but it's a gamble.

@iloveitaly
Copy link
Contributor

@ErikBjare what's left to do here? What's breaking/needs more testing?

@ErikBjare
Copy link
Member Author

@iloveitaly Basically everything mentioned in the PR description.

I've abandoned work on this for now, as you provided an alternative method that wasn't as cumbersome to implement (but may still suffer from some of the same bugs).

@ErikBjare
Copy link
Member Author

Ah, I saw now that this would improve CPU and memory from frequently spawning a process.

I guess this needs to be revisited, but I'm personally prioritizing signing the app bundle, but it might be easier to do if we had this...

I'll think about it, unless you're willing to give it a shot @iloveitaly? :)

@iloveitaly
Copy link
Contributor

@ErikBjare I may have time to take a look! Do you remember why this didn't get merged / what was left to do here?

@iloveitaly
Copy link
Contributor

@ErikBjare any notes around what's left here to do?

@ErikBjare
Copy link
Member Author

ErikBjare commented Jan 19, 2022

@iloveitaly Only what's in the PR description I think. Basically testing it and making sure that permissions etc work.

@iloveitaly
Copy link
Contributor

@ErikBjare we should close this out too—the swift-based method solves all of these problems!

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.

4 participants