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

AsyncObserver #4444

Merged
merged 12 commits into from
Feb 16, 2024
Merged

AsyncObserver #4444

merged 12 commits into from
Feb 16, 2024

Conversation

aleks-f
Copy link
Member

@aleks-f aleks-f commented Feb 6, 2024

#4414

Summary:

  • existing functionality is only broken in the (now deprecated) AbstractObserver::accepts()
    This was a "quick and dirty" external path to the goal of filtering notifications by name (in addition to type-filtering)
  • optional _matcher is added to NObserver
    (defaults to null and does not break existing functionality)
  • NotificationCenter::observersTonotify() filters out the non-interested observers
    • this is the only place where dynamic_cast performance hit is taken
    • unnecessary casting is avoided when matcher is present and does not match
    • since observers are filtered, only the ones with matching type are notified, so notify() can cast statically
  • AsyncObserver is child of NObserver and runs its own thread to decouple Notification posting from handling
  • addedAsyncNotificationCenter, a child class of the NotificationCenter; currently, there is a problem with default static version of this observer on windows:
    // TODO: static object thread hangs without this on windows
    // for explanation/solution, see
    // https://stackoverflow.com/questions/10441048/exit-thread-upon-deleting-static-object-during-unload-dll-causes-deadlock
    nc.stop();

@aleks-f aleks-f marked this pull request as draft February 12, 2024 08:19
@aleks-f aleks-f marked this pull request as ready for review February 12, 2024 13:02
Copy link
Contributor

@matejk matejk left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

@aleks-f aleks-f merged commit 88be669 into devel Feb 16, 2024
31 checks passed
@aleks-f aleks-f deleted the 4414-active-notification-center branch February 16, 2024 08:34
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

2 participants