Merge pull request #116 from bamlab/fix-error-when-recreating-node

fix(core): fix error when recreating node

Author: Pierre Poupin

packages/example/App.tsx

@@ -17,6 +17,7 @@ import { NonVirtualizedGridPage } from './src/pages/NonVirtualizedGridPage';
 import { GridWithLongNodesPage } from './src/pages/GridWithLongNodesPage';
 import { useTVPanEvent } from './src/components/PanEvent/useTVPanEvent';
 import { SpatialNavigationDeviceTypeProvider } from '../lib/src/spatial-navigation/context/DeviceContext';
+import { ListWithVariableSize } from './src/pages/ListWithVariableSize';
 
 const Stack = createNativeStackNavigator<RootStackParamList>();
 
@@ -27,6 +28,7 @@ export type RootTabParamList = {
   ProgramGridPage: undefined;
   NonVirtualizedGridPage: undefined;
   GridWithLongNodesPage: undefined;
+  ListWithVariableSize: undefined;
 };
 
 export type RootStackParamList = {
@@ -54,6 +56,7 @@ const TabNavigator = () => {
         <Tab.Screen name="ProgramGridPage" component={ProgramGridPage} />
         <Tab.Screen name="NonVirtualizedGridPage" component={NonVirtualizedGridPage} />
         <Tab.Screen name="GridWithLongNodesPage" component={GridWithLongNodesPage} />
+        <Tab.Screen name="ListWithVariableSize" component={ListWithVariableSize} />
       </Tab.Navigator>
     </MenuProvider>
   );

packages/example/src/components/Menu/Menu.tsx

@@ -58,6 +58,7 @@ const menuItems: Record<keyof RootTabParamList, MenuItems> = {
     label: 'Grid with long nodes',
     icon: 'LayoutDashboard',
   },
+  ListWithVariableSize: { label: 'List with variable size', icon: 'LayoutDashboard' },
 };
 
 export const Menu = ({ state, navigation }: BottomTabBarProps) => {

packages/example/src/modules/program/view/ProgramList.tsx

@@ -25,10 +25,12 @@ export const ProgramList = ({
   orientation,
   containerStyle,
   listRef,
+  data,
 }: {
   orientation?: 'vertical' | 'horizontal';
   containerStyle?: object;
   listRef: MutableRefObject<SpatialNavigationVirtualizedListRef>;
+  data?: ProgramInfo[];
 }) => {
   const navigation = useNavigation<NativeStackNavigationProp<RootStackParamList>>();
 
@@ -52,7 +54,7 @@ export const ProgramList = ({
         <Container isActive={isActive} style={containerStyle}>
           <SpatialNavigationVirtualizedList
             orientation={orientation}
-            data={programInfos}
+            data={data ?? programInfos}
             renderItem={renderItem}
             itemSize={theme.sizes.program.portrait.width + 30}
             numberOfRenderedItems={WINDOW_SIZE}

packages/example/src/pages/ListWithVariableSize.test.tsx

@@ -0,0 +1,57 @@
+import { ThemeProvider } from '@emotion/react';
+import { render } from '@testing-library/react-native';
+import { theme } from '../design-system/theme/theme';
+import { ListWithVariableSize } from './ListWithVariableSize';
+import { NavigationContainer } from '@react-navigation/native';
+import '../components/tests/helpers/configureTestRemoteControl';
+import testRemoteControlManager from '../components/tests/helpers/testRemoteControlManager';
+
+jest.mock('../modules/program/infra/programInfos', () => ({
+  getPrograms: () => {
+    return jest.requireActual('../modules/program/infra/programInfos').programInfos;
+  },
+}));
+
+const renderWithProviders = (page: JSX.Element) => {
+  return render(
+    <NavigationContainer>
+      <ThemeProvider theme={theme}>{page}</ThemeProvider>
+    </NavigationContainer>,
+  );
+};
+describe('ListWithVariableSize', () => {
+  it('node is still focusable after being removed', () => {
+    const screen = renderWithProviders(<ListWithVariableSize />);
+
+    // Go to last programd and focus it
+    testRemoteControlManager.handleRight();
+    testRemoteControlManager.handleRight();
+    testRemoteControlManager.handleRight();
+    expect(screen.getByLabelText('Program 4')).toBeSelected();
+
+    // Go back to first position
+    testRemoteControlManager.handleLeft();
+    testRemoteControlManager.handleLeft();
+    testRemoteControlManager.handleLeft();
+
+    // Remove last item
+    testRemoteControlManager.handleDown();
+    testRemoteControlManager.handleDown();
+    testRemoteControlManager.handleEnter();
+
+    expect(screen.queryByLabelText('Program 4')).not.toBeOnTheScreen();
+
+    // Add back last item
+    testRemoteControlManager.handleUp();
+    testRemoteControlManager.handleEnter();
+    expect(screen.getByLabelText('Program 4')).toBeOnTheScreen();
+
+    // Focus last item
+    testRemoteControlManager.handleUp();
+    testRemoteControlManager.handleRight();
+    testRemoteControlManager.handleRight();
+    testRemoteControlManager.handleRight();
+
+    expect(screen.getByLabelText('Program 4')).toBeSelected();
+  });
+});

packages/example/src/pages/ListWithVariableSize.tsx

@@ -0,0 +1,79 @@
+import styled from '@emotion/native';
+import { Page } from '../components/Page';
+import { getPrograms } from '../modules/program/infra/programInfos';
+import { SpatialNavigationView } from '../../../lib/src/spatial-navigation/components/View';
+import { scaledPixels } from '../design-system/helpers/scaledPixels';
+import { DefaultFocus } from '../../../lib/src/spatial-navigation/context/DefaultFocusContext';
+import { SpatialNavigationNode } from '../../../lib/src/spatial-navigation/components/Node';
+import { Spacer } from '../design-system/components/Spacer';
+import { Button } from '../design-system/components/Button';
+import { useState } from 'react';
+import { ProgramList } from '../modules/program/view/ProgramList';
+import { useTheme } from '@emotion/react';
+
+const ROW_PADDING = scaledPixels(70);
+
+const MAX = 1000;
+
+export const ListWithVariableSize = () => {
+  const theme = useTheme();
+  const [programsBase, setProgramsBase] = useState(getPrograms(MAX));
+
+  const [numberOfPrograms, setNumberOfPrograms] = useState(4);
+
+  const addItem = () => {
+    setNumberOfPrograms((prev) => {
+      if (prev === MAX) return prev;
+
+      return prev + 1;
+    });
+  };
+
+  const removeItem = () => {
+    setNumberOfPrograms((prev) => {
+      if (prev === 0) return prev;
+      return prev - 1;
+    });
+  };
+
+  const shuffleItems = () => {
+    setProgramsBase((prev) => [...prev].sort(() => Math.random() - 0.5));
+  };
+
+  const programs = programsBase.slice(0, numberOfPrograms);
+
+  return (
+    <Page>
+      <DefaultFocus>
+        <Container>
+          <SpatialNavigationNode orientation="horizontal">
+            <ListContainer>
+              <ProgramList
+                data={programs}
+                listRef={null}
+                containerStyle={{ height: theme.sizes.program.portrait.height + ROW_PADDING }}
+              />
+            </ListContainer>
+          </SpatialNavigationNode>
+          <Spacer gap="$6" />
+          <SpatialNavigationView direction="vertical">
+            <Button label="Add item" onSelect={addItem} />
+            <Button label="Remove item" onSelect={removeItem} />
+            <Button label="Shuffle items" onSelect={shuffleItems} />
+          </SpatialNavigationView>
+        </Container>
+      </DefaultFocus>
+    </Page>
+  );
+};
+
+const Container = styled.View({
+  flex: 1,
+  padding: scaledPixels(30),
+});
+
+const ListContainer = styled.View(({ theme }) => ({
+  flexDirection: 'row',
+  gap: theme.spacings.$4,
+  padding: theme.spacings.$4,
+}));

packages/lib/src/spatial-navigation/components/virtualizedList/SpatialNavigationVirtualizedListWithVirtualNodes.tsx

@@ -48,9 +48,11 @@ const useRegisterInitialAndUnregisterFinalVirtualNodes = <T,>({
 const useUpdateRegistration = <T,>({
   allItems,
   registerNthVirtualNode,
+  unregisterNthVirtualNode,
 }: {
   allItems: Array<T>;
   registerNthVirtualNode: (index: number) => void;
+  unregisterNthVirtualNode: (index: number) => void;
 }) => {
   const previousAllItems = useRef<Array<T>>();
 
@@ -63,6 +65,7 @@ const useUpdateRegistration = <T,>({
         currentItems: allItems,
         previousItems: previousAllItemsList,
         addVirtualNode: registerNthVirtualNode,
+        removeVirtualNode: unregisterNthVirtualNode,
       });
     }
     previousAllItems.current = allItems;
@@ -109,7 +112,7 @@ const useRegisterVirtualNodes = <T extends ItemWithIndex>({
     unregisterNthVirtualNode,
   });
 
-  useUpdateRegistration({ allItems, registerNthVirtualNode });
+  useUpdateRegistration({ allItems, registerNthVirtualNode, unregisterNthVirtualNode });
 
   return { getNthVirtualNodeID };
 };

packages/lib/src/spatial-navigation/components/virtualizedList/helpers/updateVirtualNodeRegistration.test.ts

@@ -11,6 +11,7 @@ describe('updateVirtualNodeRegistration', () => {
       previousItems,
       currentItems,
       addVirtualNode: mockAddNode,
+      removeVirtualNode: jest.fn(),
     });
 
     expect(mockAddNode).toHaveBeenCalledTimes(2);
@@ -24,6 +25,7 @@ describe('updateVirtualNodeRegistration', () => {
       previousItems,
       currentItems,
       addVirtualNode: mockAddNode,
+      removeVirtualNode: jest.fn(),
     });
 
     expect(mockAddNode).not.toHaveBeenCalled();

packages/lib/src/spatial-navigation/components/virtualizedList/helpers/updateVirtualNodeRegistration.ts

@@ -15,6 +15,20 @@ const registerNewNode = <T>({
   });
 };
 
+const unregisterOldNode = <T>({
+  currentItems,
+  previousItems,
+  removeVirtualNode,
+}: {
+  currentItems: Array<T>;
+  previousItems: Array<T>;
+  removeVirtualNode: (index: number) => void;
+}) => {
+  for (let index = previousItems.length - 1; index > currentItems.length - 1; index--) {
+    removeVirtualNode(index);
+  }
+};
+
 /**
  * This function aims to compare 2 arrays of items : currentItems and previousItems and :
  * - addVirtualNode for every item from currentItems that weren't in previousItems
@@ -26,17 +40,16 @@ export const updateVirtualNodeRegistration = <T>({
   currentItems,
   previousItems,
   addVirtualNode,
+  removeVirtualNode,
 }: {
   currentItems: Array<T>;
   previousItems: Array<T>;
   addVirtualNode: (index: number) => void;
+  removeVirtualNode: (index: number) => void;
 }) => {
   // Step 1 : addVirtualNode for every item from currentItems that weren't in previousItems
   registerNewNode({ currentItems, previousItems, addVirtualNode });
 
   // Step 2 : removeVirtualNode for every from previousItems that aren't there anymore in currentItems
-  // TODO
-
-  // Step 3 : re-order all the items
-  // TODO
+  unregisterOldNode({ currentItems, previousItems, removeVirtualNode });
 };