nativescript-community / ui-collectionview

Allows you to easily add a collection view (grid list view) to your projects. Supports vertical and horizontal modes, templating, and more.
Apache License 2.0
59 stars 18 forks source link

ios crash on back navigation when using visibility-binding inside itemtemplate #58

Open felixkrautschuk opened 1 year ago

felixkrautschuk commented 1 year ago

Make sure to check the demo app(s) for sample usage

Make sure to check the existing issues in this repository

If the demo apps cannot help and there is no issue for your problem, tell us about it

Please, ensure your title is less than 63 characters long and starts with a capital letter.

When navigating forwards and then backwards between multiple instances of a page using a collectionview, the iOS app crashes, when using a visibility binding inside the itemTemplate. I only occurs when navigating at least 2 times to the same page. Hard to explain by words, look at the video:

https://user-images.githubusercontent.com/6443021/228847852-7f1e6533-3602-4067-b05b-a6eeb50509bf.mov

Fatal JavaScript exception - application has been terminated. NativeScript encountered a fatal error: Uncaught Error: Invalid visibility value: undefined. Valid values are: "visible", "hidden", "collapse". at [visibility:setNative](file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view/index.ios.js:598:0) at applyPendingNativeSetters(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/properties/index.js:1117:0) at initNativeView(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/properties/index.js:1076:0) at onResumeNativeUpdates(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:877:22) at _resumeNativeUpdates(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:351:0) at onLoaded(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:305:0) at onLoaded(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view/view-common.js:108:0) at (file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:392:0) at callFunctionWithSuper(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:386:0) at callLoaded(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:392:0) at loadView(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:556:0) at (file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:307:0) at eachChildView(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/layouts/layout-base-common.js:101:0) at eachChild(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view/view-common.js:793:0) at onLoaded(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:306:0) at onLoaded(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view/view-common.js:108:0) at (file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/view-base/index.js:392:0) at callFunctionWithSuper(file:///app/vendor.js<…>

<!-- itemTemplate -->

<StackLayout>
    <Label text="{{ id }}"/>
    <Label text="{{ title }}"/>
    <Label text="text" visibility="{{ isNew ? 'visible' : 'collapse' }}"/>  <!-- causing the issue -->
    <Button text="delete" tap="deleteItem"/>
</StackLayout>
export function onPageLoaded() {
  loadItems();
}

function loadItems() {
  vm.set("isLoading", true);

  Http.request({
      url: "https://pokeapi.co/api/v2/ability/?limit=10000",
      method: "GET"
  }).then((response) => {
     const json = response.content.toJSON();

     vm.get("items").splice(0, vm.get("items").length);
     vm.get("items").push(...json.results.map((data) => {
         return {
            title: data.name,
            id: data.url,
            isNew: false
         }
     }));

     vm.set("isLoading", false);
   }, (e) => {
      vm.set("isLoading", true);
   });
}

export function onItemTap()  {
  Frame.topmost().navigate("collectionview-page");
}

Not sure if this is really related to collectionview or maybe nativescript/core, but the issue only occurs using collectionview for us so far (not with RLV or default ListView), so I decided to report it here.

Which platform(s) does your issue occur on?

Please, provide the following version numbers that your issue occurs with:

Please, tell us how to recreate the issue in as much detail as possible.

Describe the steps to reproduce it.

Just follow the steps from the video provided above

Is there any code involved?

ns-collectionview.zip

felixkrautschuk commented 1 year ago

I just noticed that the isue can be prevented by resetting the whole by re-setting the whole ObservableAeeay instead of clearing and pushing items to it.

Instead

vm.get("items").splice(0);
vm.get("items").push(...newItemsArray);

do

vm.set("items", new ObservableArray(newItemsArray));

But actually I thought it would be a better practice to re-use the existing ObservableARray instead of creating a new one every time I load the list items from our server.

farfromrefug commented 1 year ago

@felixkrautschuk not sure it is directly related to this plugin. The error happens on N side. Would need to figure out why and where from visibility is set to undefined. Could be a reset issue on N side

felixkrautschuk commented 1 year ago

@farfromrefug I am also not sure why this plugin should cause the issue. I just noticed that the crash did not appear when doing the same stuff with ListView or RadListView.

farfromrefug commented 1 year ago

@felixkrautschuk I missed that part ! Will try to look at it when I can

farfromrefug commented 1 year ago

@felixkrautschuk by the way doing the slice then push is not optimized as you do 2 operations whuh means 2 collectionview updates. Instead do a splice removing all and adding all new ones.

felixkrautschuk commented 1 year ago

@farfromrefug I tried to remove the items and add new items in one step with using the splice method, but then I see those layout issues again as described in https://github.com/nativescript-community/ui-collectionview/issues/57 and the crash is also still happening. Both things only happen in our own app, I was not able yet to make them reproducable in the sample app so far.

However I noticed, that there also seems to be a timing issue. When I comment the http request part in the sample app and instead load the items like that:

...
vm.set("isLoading", true);

setTimeout(() => {
      vm.get("items").splice(0, vm.get("items").length);
      vm.get("items").push(...initialItems);

      vm.set("isLoading", false);
}, 200);
...

... then the app is crashing. When I increase the delay to 1000ms, the crash does not occur.

felixkrautschuk commented 1 year ago

Finally, I managed to find out why the layout issue and the crash still orccur in our own app with splicing and pushing items in one step. It's because item count can change as well while navigating forwards and backwards.

When I change the demo app, that it always pushes a random number of items to the list like this:

...
setTimeout(() => {
      vm.get("items").splice(0, vm.get("items").length, ...initialItems.slice(0, randomNumber(1, 20)));
      vm.set("isLoading", false);
}, 300);
...

... then the layout issue and the crash are reproducable in the demo app as well:

https://user-images.githubusercontent.com/6443021/232085148-10e91876-b7cf-40d5-9d34-1be12edbf3c1.mov

Updated demo app: ns-collectionview.zip

farfromrefug commented 1 year ago

@felixkrautschuk and it does not happen with listview and rlv?

felixkrautschuk commented 1 year ago

@farfromrefug with ListView, everything works as expected (no layout issues, no crashes) With RadListView, the layout issues are already visible on forwards navigation (with collectionview only when navigating backwards) and the app is also crashing on iOS with a different error:

NativeScript encountered a fatal error: Uncaught Error: attempt to delete item 18 from section 0 which only contains 1 items before the update at onSourceCollectionChanged(file: app/webpack:/ns-collectionview/node_modules/nativescript-ui-listview/index.ios.js:1658:0) at onSourceCollectionChangedInternal(file: app/webpack:/ns-collectionview/node_modules/nativescript-ui-listview/common.js:598:0) at handler(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/ui/core/weak-event-listener/index.js:31:0) at _handleEvent(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/data/observable/index.js:306:0) at notify(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/data/observable/index.js:287:0) at splice(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/data/observable-array/index.js:187:0) at (file: app/webpack:/ns-collectionview/app/radlistview-page.ts:74:22) at invoke(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/timer/index.ios.js:54:0) at TimerTargetImpl.tick(file: app/webpack:/ns-collectionview/node_modules/@nativescript/core/timer/index.ios.js:18:0)

felixkrautschuk commented 1 year ago

Btw, the crash for RadListView can be prevented bei calling listview.refresh() in the loaded-event of RadListView. I tried that for collectionview as well, but that did not help.

farfromrefug commented 1 year ago

@felixkrautschuk sorry dont have much time to look at they right now

MrSnoozles commented 1 year ago

@felixkrautschuk I've got the exact same error (only on iOS aswell, Android works fine). I think it could be related to bugs in ObservableArray. Somewhere the length of the ObservableArray gets mixed up. I also removed all the .splice calls, as soon as I manually .push items to an existing ObservableArray the app will eventually crash. If you have found any workaround I'd be glad if you could share. I'm trying to find a solution too.

felixkrautschuk commented 1 year ago

@MrSnoozles I only found out that I could prevent the crash and the layout issue by creating a new ObservableArray everytime I want to remove/add items, but actually that should not be necessary.

farfromrefug commented 1 year ago

@felixkrautschuk i tested taht a bit. To test it the right way i had to change your demo to call loadItems in onNavigatingTo and not onLoaded.

So for now it is a wont fix here as the example shows clear issues.

felixkrautschuk commented 1 year ago

@farfromrefug thanks for your investigations and sorry for late reply.

Regarding the time of loading the items: I actually load everything in navigatingTo event in our apps, as it feels smooth. Sometimes the loading process is really fast, so it feels like the data is already there when the page is shown to the user. When doing that in navigatedTo event, it feels really weird as the user gets a blank page or only a placeholder for half a second and then only the loading process begins.

And yes, I have used that "shared" ObservableArray approach for years now with RadListView and ListView as I never had problems with that. But I got your point and I admit, that it is not a good practice. When creating a freshly new Observable (and ObservableArray) for every page, the crash does not happen anymore.

Just want to add one thing: there is one weird effect with iOS 16.4 (I updated Xcode 14.3 last week). Loading the items in navigatingTo event is working for CollectionView on iOS, but for iOS 16.4 it is crashing (even when using loaded event):

Error: Invalid batch updates detected: the number of sections and/or items returned by the data source before and/or after performing the batch updates are inconsistent with the updates. Data source before updates = { 1 section with item counts: [5] } Data source after updates = { 1 section with item counts: [5] } Updates = [ Insert item (0 - 0), Insert item (0 - 1), Insert item (0 - 2), Insert item (0 - 3), Insert item (0 - 4) ] Collection view: <UICollectionView: 0x7fcd33811000; frame = (0 97.6667; 393 720.333); clipsToBounds = YES; hidden = YES; autoresizesSubviews = NO; tag = 117; gestureRecognizers = <NSArray: 0x6000039fcb70>; backgroundColor = UIExtendedGrayColorSpace 0 0; layer = <CALayer: 0x600003701220>; contentOffset: {0, 0}; contentSize: {393, 557}; adjustedContentInset: {0, 0, 0, 0}; layout: <UICollectionViewFlowLayout: 0x7fcd3291aa20>; dataSource: <CollectionViewDataSource: 0x60000356dcf0>>

At first I thought that it is a timing issue and I am forced to use navigatedTo event, but then I noticed that I can prevent the crash by using a setTimeout even with 0 delay:

export function onPageNavigatingTo(args: NavigatedData) {
  vm = createViewModel();

  page = args.object as Page;
  page.bindingContext = vm;

  setTimeout(() => {
    loadItems();
  }, 0);
}

Do you an explanation for that? When using RadListView (or ListView), this issue is not occuring.

Latest sample app: ns-collectionview.zip

darklight365 commented 1 year ago

@farfromrefug - This seems to be an iOS 16.4 bug: https://developer.apple.com/forums/thread/728797

It looks like a possible solution to get around this case would be to switch from UICollectionViewDataSource to UICollectionViewDiffableDataSource as the data source being used within the plugin codebase.