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 #32 - MacOS crash when Desktop becomes the active window #39

Closed
wants to merge 3 commits into from

Conversation

uglow
Copy link
Contributor

@uglow uglow commented Mar 17, 2019

Fixes #32.

The Desktop window does not appear in the window collection because of the excludeDesktopElements flag - which is reasonable as you probably don't want icons and the System menu showing up in the results. However, this causes the crash that was identified in issue #32.

This PR prevents the system from crashing out when the active window is not in the window collection by return a "fake" Desktop result.

@uglow
Copy link
Contributor Author

uglow commented Mar 18, 2019

@sindresorhus Good to merge?

@matthewjumpsoffbuildings

im having this issue too, hoping this fixes it

@uglow
Copy link
Contributor Author

uglow commented Mar 18, 2019

@tiangolo @sindresorhus - are you able to review and merge, please?

"bundleId": app.bundleIdentifier!,
"path": app.bundleURL!.path
]
]
Copy link
Owner

Choose a reason for hiding this comment

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

If we go with this solution, you have to fill out all the fields.

@sindresorhus
Copy link
Owner

I think a better solution would be to admit that there can't always be an active window, and return null in such scenarios.

@uglow
Copy link
Contributor Author

uglow commented Mar 20, 2019

If we return null it would be a (more) breaking change than this PR. But I don’t mind.

@sindresorhus
Copy link
Owner

I'm ok with that.

@matthewjumpsoffbuildings
Copy link

matthewjumpsoffbuildings commented Mar 20, 2019

i also think returning null is fine, its easy enough to modify code to check if null

watever happens, as long as it doesnt break if you are lookin at the desktop on macOS im happy ;)

…which is what is happening inside lib/macos.js, but is not directly testable.
@uglow
Copy link
Contributor Author

uglow commented Mar 20, 2019

Updated to return null. Works locally when calling sleep 5 && ./main. Added test to demonstrate that JSON.parse('null') returns null (ie. retuning 'null' doesn't break the JSON parser).

@uglow
Copy link
Contributor Author

uglow commented Mar 21, 2019

@sindresorhus Good to merge?

@uglow
Copy link
Contributor Author

uglow commented Mar 23, 2019

Btw, I've just created a tool called active-win-log (link) which uses this package to create a usage log. I'm using it to help me keep track of time spent on projects when working remotely. Thanks for writing this package! 👍🏻

@sindresorhus
Copy link
Owner

@uglow Cool use-case. You should do a PR to add it to the readm here. Under a new "Users" section or something ;)

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.

None yet

3 participants