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

Remove observers for events #2236

Merged
merged 12 commits into from
Jul 19, 2024
Merged

Conversation

zeitschlag
Copy link
Collaborator

@zeitschlag zeitschlag commented Jul 12, 2024

  • Fix some leftovers from Refactor events #2234
  • Migrate over everything but Progress-observers from closures to methods. This way there's no need to remove the observers manually.
  • Clean up DispatchQueue.Main-handling (at least in ChatViewController) (Caller site decides)

  • As discussed: Observers in ProgressAlertHandler are not part of this PR as I need to come up with an alternative approach to that Protocol-extension and closures. Let me think about it a little longer.

@zeitschlag zeitschlag force-pushed the refactor-notifications-bye-observer branch 2 times, most recently from 21bb337 to 2649919 Compare July 12, 2024 13:56
@zeitschlag zeitschlag changed the title [WIP] Remove observers for notifications [WIP] Remove observers for events Jul 16, 2024
@zeitschlag zeitschlag mentioned this pull request Jul 16, 2024
Base automatically changed from refactor-notifications to main July 16, 2024 14:14
@zeitschlag zeitschlag force-pushed the refactor-notifications-bye-observer branch from 7d58159 to 4268761 Compare July 16, 2024 14:16
@zeitschlag zeitschlag force-pushed the refactor-notifications-bye-observer branch from 4268761 to 3211195 Compare July 16, 2024 15:38
@zeitschlag zeitschlag self-assigned this Jul 16, 2024
@zeitschlag zeitschlag changed the title [WIP] Remove observers for events Remove observers for events Jul 19, 2024
@zeitschlag zeitschlag marked this pull request as ready for review July 19, 2024 11:44
@zeitschlag zeitschlag requested a review from r10s July 19, 2024 11:44
@r10s
Copy link
Member

r10s commented Jul 19, 2024

yeah: Screenshot 2024-07-19 at 14 55 08 is the harvest of the previous PRs 👌

@r10s
Copy link
Member

r10s commented Jul 19, 2024

Clean up DispatchQueue.Main-handling (at least in ChatViewController) (Caller site decides)

we're often calling DispatchQueue.main.async two times now. probably no big deal, however, in the future, we can consider to say that DcEventHandler does not dispatch to main and it is up to the consumer to do that as needed. that step would require careful double checking, of course.

but that is no blocker of this PR with excellent improvements together with the other efforts. this will make maintenance much easier 👌

@zeitschlag zeitschlag merged commit d610fb8 into main Jul 19, 2024
1 check passed
@zeitschlag zeitschlag deleted the refactor-notifications-bye-observer branch July 19, 2024 13:59
@zeitschlag zeitschlag added this to the 1.46.8 milestone Aug 10, 2024
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.

2 participants