Refactor FXIOS-12995 [Swift 6 Migration] Update NotificationCenter usage to use Notifiable pattern (part 1) #30250
Conversation
|
This pull request has conflicts when rebasing. Could you fix it @ih-codes? 🙏 |
|
Note: The BVC has a really big method that needs to be migrated, I will do this in a 2nd PR. |
ih-codes
force-pushed
the
ih/FXIOS-12995-update-notificationcenter-usage-to-use-notifiable
branch
from
ba84376 to
3ff295e
Compare
🥇 Perfect PR sizeSmaller PRs are easier to review. Thanks for making life easy for reviewers! ✨ 💬 Description craftsmanGreat PR description! Reviewers salute you 🫡 ❌ Per-file test coverage gateThe following changed file(s) are below 35.0% coverage:
Client.app: Coverage: 37.09
Generated by 🚫 Danger Swift against ff1bf66 |
| import Shared | ||
|
|
||
| class SyncNowSetting: WithAccountSetting { | ||
| class SyncNowSetting: WithAccountSetting, |
There was a problem hiding this comment.
WithAccountSetting is a very annoying class name lololol
There was a problem hiding this comment.
The Settings inheritance chains are scary 🙈
| func startRotateSyncIcon() { | ||
| DispatchQueue.main.async { | ||
| self.imageView.layer.add(self.continuousRotateAnimation, forKey: "rotateKey") | ||
| } | ||
| self.imageView.layer.add(self.continuousRotateAnimation, forKey: "rotateKey") | ||
| } | ||
|
|
||
| @objc | ||
| func stopRotateSyncIcon() { | ||
| DispatchQueue.main.async { | ||
| self.imageView.layer.removeAllAnimations() | ||
| self.imageView.layer.removeAllAnimations() | ||
| } |
There was a problem hiding this comment.
Should we isolate these functions to the MainActor?
There was a problem hiding this comment.
There's a super long scary inheritance chain of this class down to Setting which is @MainActor, so these are all isolated already!
There was a problem hiding this comment.
Screenshot from main.
ih-codes
deleted the
ih/FXIOS-12995-update-notificationcenter-usage-to-use-notifiable
branch
|
🚀 PR merged to |
📜 Tickets
Jira ticket
Github issue
💡 Description
This PR updates several lingering direct usages of NotificationCenter to instead use our Notifiable protocol to enforce safety with implicitly nonisolated
@objcselector method.Wiki documentation here: https://github.com/mozilla-mobile/firefox-ios/wiki/Notifable-Protocol-and-the-NotificationCenter
NOTE: The BVC has a really big method that needs to be migrated, I will do this in a 2nd PR.
cc @Cramsden @lmarceau @dataports | Swift 6 Migration
📝 Checklist