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

[Optimization] UIManager cannot re-use elements #1707

Closed
uber5001 opened this issue Jun 22, 2015 · 2 comments
Closed

[Optimization] UIManager cannot re-use elements #1707

uber5001 opened this issue Jun 22, 2015 · 2 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@uber5001
Copy link

It would be useful if the UIManager allowed for re-use of native elements, so that elements don't need to be destroyed every time they should be removed from the application tree. Instead of destroying elements upon removal from their parent, there could be a UIManager.destroyView instead.

Currently, this is what happens:

// create some text and attach it...
RCTUIManager.createView(2, "RCTText", {});
RCTUIManager.createView(3, "RCTRawText", {"text":"foobar"});
RCTUIManager.manageChildren(2, null, null, [3], [0], null);
RCTUIManager.manageChildren(1, null, null, [2], [0], null);
// If we stop here, the text "foobar" is rendered, as expected

// detach the text...
RCTUIManager.manageChildren(1, null, null, null, null, [0]);
// If we stop here, the text is no longer visible, as expected

// re-attach the text...
RCTUIManager.manageChildren(1, null, null, [2], [0], null);
// If we stop here, the text is still not visible, since element 2 and 3 have been destroyed
// Oddly, there is no error in attaching a non-existent element.

// Update the text...
RCTUIManager.updateView(3, "RCTRawText", {"text":"new text"});
// App crashes with error:
//    Error setting property 'text' of RCTRawText with tag #(null): (null) does not have setter for `text` property

Ideally, when re-attaching the text, it uses the same element that was detached earlier.

@ide ide changed the title UIManager cannot re-use elements [Optimization] UIManager cannot re-use elements Jun 22, 2015
@brentvatne
Copy link
Collaborator

@uber5001:

RCTUIManager.manageChildren(1, null, null, null, null, [0]);

For container 1, move from null, to null, add children null, add at indices null, remove at indices [0]

RCT_EXPORT_METHOD(manageChildren:(NSNumber *)containerReactTag
                  moveFromIndices:(NSArray *)moveFromIndices
                  moveToIndices:(NSArray *)moveToIndices
                  addChildReactTags:(NSArray *)addChildReactTags
                  addAtIndices:(NSArray *)addAtIndices
                  removeAtIndices:(NSArray *)removeAtIndices)

If you look at the definition of _manageChildren:

  // Removes (both permanent and temporary moves) are using "before" indices
  NSArray *permanentlyRemovedChildren = [self _childrenToRemoveFromContainer:container atIndices:removeAtIndices];
  NSArray *temporarilyRemovedChildren = [self _childrenToRemoveFromContainer:container atIndices:moveFromIndices];

Notice that what you indicated as detaching is actually permanently removing.

And immediately after that:

[self _purgeChildren:permanentlyRemovedChildren fromRegistry:registry];

So I think the point about updateView re-using an existing view is moot here because we actually explicitly destroy it. It looks like right now there is only support for detaching/re-attaching atomically via specifying moveFromIndices and moveToIndices. cc @tadeuzagallo

Also, for context it appears to be related to this: angular/react-native-renderer#1

@brentvatne
Copy link
Collaborator

Closing due to inactivity

@facebook facebook locked as resolved and limited conversation to collaborators Jul 22, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

4 participants