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

[macos] Improve handling of missing application attributes #105

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

Conversation

amsam0
Copy link

@amsam0 amsam0 commented Oct 25, 2024

This PR tries to fix at least 1 crash in the macOS swift implementation.

As described in ActivityWatch/aw-qt#103, a custom GTK application I wrote was previously ignored since it didn't have a bundle ID. It is now correctly recognized, and a few other possible crash points have been fixed. However, I have been unable to replicate the crashing I used to experience (it may have already been fixed).

Related: #101, #74, #100


Important

Fixes crash in macOS Swift by handling missing bundle identifiers and improving error handling for Chrome and Safari in macos.swift.

  • Behavior:
    • Fixes crash by using localizedName or bundleIdentifier for application name in MainThing.windowTitleChanged().
    • Adds error handling for missing bundleIdentifier in Chrome and Safari sections in MainThing.windowTitleChanged().
    • Improves error logging for failed window/tab data extraction in Chrome and Safari.
  • Misc:
    • Minor formatting change in debug() function condition.

This description was created by Ellipsis for 58dcb87. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 58dcb87 in 19 seconds

More details
  • Looked at 118 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. aw_watcher_window/macos.swift:332
  • Draft comment:
    Avoid force unwrapping with SBApplication.init(bundleIdentifier: bundleIdentifier)!. Consider using optional binding to safely handle potential nil values.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. aw_watcher_window/macos.swift:367
  • Draft comment:
    Avoid force unwrapping with SBApplication.init(bundleIdentifier: bundleIdentifier)!. Consider using optional binding to safely handle potential nil values.
  • Reason this comment was not posted:
    Marked as duplicate.
3. aw_watcher_window/macos.swift:355
  • Draft comment:
    Consider using debug or log instead of error for logging tab title differences, as this might not be an error condition.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error logging for tab title differences is using error level, which might not be appropriate for this kind of information. It could be more suitable to use debug or log level instead.
4. aw_watcher_window/macos.swift:385
  • Draft comment:
    Consider using debug or log instead of error for logging tab title differences, as this might not be an error condition.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error logging for tab title differences is using error level, which might not be appropriate for this kind of information. It could be more suitable to use debug or log level instead.

Workflow ID: wflow_qHWQnQn7IDXBftuy


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@amsam0 amsam0 changed the title [macos] Attempt to fix some crashes [macos] Improve handling of missing application attributes Oct 25, 2024
@amsam0
Copy link
Author

amsam0 commented Nov 9, 2024

@ErikBjare any chance of this getting merged?

@amsam0
Copy link
Author

amsam0 commented Dec 12, 2024

@ErikBjare sorry to bother you, are there any changes you'd like me to make before it can be merged?

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.

1 participant