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

Use viewDidChangeBackingProperties to remove the use of notifications in RCTImageView #1834

Merged

Conversation

lenaic
Copy link

@lenaic lenaic commented May 30, 2023

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

In some rare cases a react native app on macOS would crash with the following message:

-[RCTImageView windowDidChangeBackingProperties:]: unrecognized selector sent to instance 0x10b0c2c00

The notification center is supposed to check if the object subscribed to the notification still exists before calling the registered selector for the notification. This is supposed to relieve us from having to remove the observer when deallocating the observing object.

In these rare crashes I assume another NSObject got allocated at the same address as what was previously an observing RCTImageView and the notification center may have considered the observer still active and sent the registered selector to it.

To fix the potential bug and also remove a couple differences with React Native this PR removes the need for notification subscriptions by using the NSView viewDidChangeBackingProperties instead.

Changelog

[macOS] [CHANGED] - Use viewDidChangeBackingProperties for image reloads on backing properties change

Test Plan

Tested by running RNTester on macOS with paper and dragging the window from a 1.0 backing scale factor display to a 2.0 backing scale factor display and checking that the image reload happens in the debugger.

Screen.Recording.2023-05-30.at.23.47.47.mov

@lenaic lenaic requested a review from a team as a code owner May 30, 2023 22:03
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

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

Yay less code!

@Saadnajmi Saadnajmi merged commit 74bd8b4 into microsoft:main Jun 1, 2023
15 of 16 checks passed
@lenaic lenaic deleted the image-reload-on-backing-props-change branch June 1, 2023 15:30
lenaic added a commit to lenaic/react-native-macos that referenced this pull request Jun 1, 2023
… in image view. (microsoft#1834)

Co-authored-by: Nick <lefever@meta.com>
Saadnajmi pushed a commit that referenced this pull request Jul 6, 2023
… in image view. (#1834) (#1840)

Co-authored-by: Nick <lefever@meta.com>
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