Skip to content

Fixing unmounting of layers during runtime (v10, iOS)#1843

Merged
mfazekas merged 1 commit intornmapbox:mainfrom
rastapasta:fix-ios-subview-handling
Apr 17, 2022
Merged

Fixing unmounting of layers during runtime (v10, iOS)#1843
mfazekas merged 1 commit intornmapbox:mainfrom
rastapasta:fix-ios-subview-handling

Conversation

@rastapasta
Copy link
Contributor

@rastapasta rastapasta commented Apr 16, 2022

Description

In the new swift implementation, the MapView's removeReactSubview is never called by the RTCManager, resulting in removed React Components not being unmounted from the map.

To solve this, the corresponding super methods are called resulting in the removeReactSubview method being called and layers removed from the map.

Checklist

  • I have tested this on a device/simulator for each compatible OS
  • I formatted JS and TS files with running yarn lint:fix in the root folder
  • I updated the documentation with running yarn generate in the root folder
  • I mentioned this change in CHANGELOG.md
  • I updated the typings files (index.d.ts)
  • I added/ updated a sample (/example)

Example code

const App = () => {
  const [visible, setVisible] = useState(false)

  return (
    <MapView
      onPress={() => setVisible(!visible)}
      style={{flex: 1}}>

      {visible && <RasterSource
        id="source"
        tileUrlTemplates={['https://stamen-tiles.a.ssl.fastly.net/watercolor/{z}/{x}/{y}.jpg']}>

        <RasterLayer
          id="raster"
          style={{rasterOpacity: 0.5}}
        />
      </RasterSource>}
    </MapView>
  )
}

@rastapasta rastapasta force-pushed the fix-ios-subview-handling branch from ca43483 to 11bfcf8 Compare April 16, 2022 17:49
@rastapasta rastapasta changed the title Fixing unmounting of layers during runtime Fixing unmounting of layers during runtime (v10, iOS) Apr 16, 2022
Copy link
Contributor

@mfazekas mfazekas left a comment

Choose a reason for hiding this comment

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

@rastapasta can you add a short sample code, where the issue occurs.
I assume something like this, and when setting visible to false the component still rendered on the map?!

<MapView>
   {visible && <ShapeSource>
       <CircleLayer .../>
     </ShapeSource>
   }
</MapView>

sources.append(source)
}

super.insertReactSubview(subview, at: atIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that we haven't called insertReactSubview in prev implementation but we did overwrote reactSubviews.

Not sure if it worth the trouble to not do the pre v10 implementation, calling super.insertReactSubview sounds simpler.

See

- (void)insertReactSubview:(id<RCTComponent>)subview atIndex:(NSInteger)atIndex {
[self addToMap:subview];
[_reactSubviews insertObject:(UIView *)subview atIndex:(NSUInteger) atIndex];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found that interesting as well when digging through the code to explain its behavior, though found no advantage of re-implementing the reactSubviews as done previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mfazekas your guess was spot on! added an example to the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only difference I can think of is that views were not added, to MapView before.

So this

<MapVIew>
  <View style={{backgroundColor: 'red', width: 100, height: 100}} />
</MapView>

will now add a view into MapView, not sure if that's works on android.

Let's go with this implementation for now, we can revisit later.

@mfazekas mfazekas merged commit 7a38dfa into rnmapbox:main Apr 17, 2022
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