Skip to content

Comments

Update MacOS title logic#29

Closed
ianobermiller wants to merge 2 commits intoActivityWatch:masterfrom
ianobermiller:macos-window-title
Closed

Update MacOS title logic#29
ianobermiller wants to merge 2 commits intoActivityWatch:masterfrom
ianobermiller:macos-window-title

Conversation

@ianobermiller
Copy link

@ianobermiller ianobermiller commented Feb 11, 2018

  • Switch from invoking osascript to pure Python
  • Clean up the api to the macos.py file (have it return the dict directly)
  • Fix a typo in README

Fixes #28

Test Plan:

  • source .././venv/bin/activate
  • aw-watcher-window --testing --verbose
  • verify the app name and title being printed are correct in a few different applications

To log current window title the teminal needs access to macOS accessibility API.
To log current window title the terminal needs access to macOS accessibility API.
This can be enabled in System Preferences > Security & Privacy > Accessibility, then add the Terminal to this list. If this is not enabled the watcher can only log current application, and not window title.

Copy link
Author

Choose a reason for hiding this comment

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

Oops, the extra changes here were due to prettier, I believe. Let me know if you want them reverted. Not sure why it would remove the trailing line...

@ianobermiller
Copy link
Author

Ok, that took entirely too long. The issue was that I had moved the app around, and ended up with the accessibility permissions disabled.

@ErikBjare ErikBjare reopened this Feb 12, 2018
@ghost ghost assigned ErikBjare Feb 12, 2018
@ghost ghost added the review label Feb 12, 2018
@ErikBjare
Copy link
Member

ErikBjare commented Feb 12, 2018

Hey, this looks great. Does it work well?

I'd rather do it like this than with the printAppTitle.scpt (which you can remove in this PR).

@ianobermiller
Copy link
Author

ianobermiller commented Feb 14, 2018 via email

@ErikBjare
Copy link
Member

The Yosemite limitation is fine.

However, I'm curious if running it as JS would have a significant performance impact since you'll be spinning up a JS VM on every call. Would be interesting to see how it compares to the .scpt.

@exoji2e
Copy link
Member

exoji2e commented Feb 14, 2018

One way to reduce the load might be to keep the VM alive? I think we need to benchmark this. But yes the .scpt-solution we did not choose out of beauty, but rather because this was the only solution I found after some days of searching the web.

@johan-bjareholt
Copy link
Member

I am trying to understand how osascript works but then started wondering, wouldn't it be possible to find the underlying OS API call which these applescript/javascript files by using dtruss? (the mac alternative to strace)
It's not a request for this PR, but maybe in the future if we have some spare time :)

- 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
@ianobermiller
Copy link
Author

Did some digging, it wasn't too bad to do with Python only :D

@exoji2e exoji2e mentioned this pull request Feb 15, 2018
Copy link
Member

@exoji2e exoji2e left a comment

Choose a reason for hiding this comment

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

Looks great! Just some minor nitpicks :)

cmd = ["osascript", os.path.join(os.path.dirname(os.path.realpath(__file__)), "printAppTitle.scpt")]
p = subprocess.run(cmd, stdout=PIPE)
return str(p.stdout, "utf8").strip()
app = NSWorkspace.sharedWorkspace().activeApplication()
Copy link
Member

Choose a reason for hiding this comment

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


def getTitle(info) -> str:
return info.split('","')[1][:-1]
return {"appname": app_name, "title": title}
Copy link
Member

@exoji2e exoji2e Feb 15, 2018

Choose a reason for hiding this comment

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

Not sure if it ever happens that the pid isn't found in window_list, but maybe it can occur because of realtime problems. We should probably move the return to the break, so we don't get a name-error here.

@ErikBjare
Copy link
Member

The Travis build is failing because the aw-watcher-window.spec file used by PyInstaller references the now removed .scpt.

Just remove the relevant tuple from the datas argument of Analysis and it should work.

@exoji2e
Copy link
Member

exoji2e commented Feb 15, 2018

Some more thoughts:

I think AppKit inside python is included if you install Xcode. We might want to check for it in the makefile, and install pyobjc otherwise.

However I'm guessing the most problems will occur with the binary release for macOS-users, since those might not have Xcode, not really sure what to do about that.

@ErikBjare
Copy link
Member

ErikBjare commented Feb 16, 2018

@exoji2e It should be possible to include the right pyobjc dependency in the PyInstaller build.

I remember we used something like this before, can't remember why we switched but probably worth investigating. Or maybe I just played with it when I had a macOS machine at work long ago? Can't recall.

@ianobermiller Don't feel discouraged by the above. It might take a while to merge your commits, but we will do it sooner or later. If you don't have the time to look into it: just let us know and we'll build on your commits when the time comes.

@ianobermiller
Copy link
Author

ianobermiller commented Feb 17, 2018 via email

@ErikBjare
Copy link
Member

Closing, work continued in #40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No titles on MacOS

4 participants