alexrainman / CarouselView

CarouselView control for Xamarin Forms
MIT License
437 stars 177 forks source link

ItemsSource changes from view #37

Closed woellij closed 7 years ago

woellij commented 7 years ago

Hi.

First of all: thanks for your component. I think it's much needed in the Xamarin.Forms world. Much obliged.

Though i ran into some big problems resetting the ItemsSource and adding / removing items from it.

At the moment the View (Renderer) removes and adds items from the collection itself. In my opinion that's a design flaw. At first it seemed to me, that the component supported DataBinding, including reflecting changes to the itemssource (observablecollection / INotifyCollectionChanged), which unfortunately isn't the case.

I think the CarouselViewControl should listen to changes to the ItemsSource and the InsertPage and RemovePage should only remove the displayed Views and not touch the ItemsSource-collection.

I'm not sure, whether there are any platform specific contraints on the component and that's why it's implemented the way it is now - changing it on android wasn't a problem (at least doing it quick and dirty)

I might send a pull request some time in the future, but at the moment i don't have the time.

For now i just wanted to 'hint' that the current design is a bit misleading (providing a bindable ItemsSource, but not reflecting changes to the view and making the Renderer change the ItemsSource-collection)

(probably for a lot of cases the itemssource won't ever change and the component works great in these cases)

greetings

alexrainman commented 7 years ago

@woellij i also think "the CarouselViewControl should listen to changes to the ItemsSource and then InsertPage and RemovePage" but, the platform specific components introduce some limitations. Also, each element in the source is tied to a view in the UI, both of them should be in sync, so if you remove an item from the source you should remove a view from UI no matter the direction you do it. That's way i provided custom methods to add/remove pages and those methods also add/remove from the source to keep UI and source in sync, exactly the same way it will behave if the source is observable. DataBindings works really well in the component less this part. Anyway, i am not giving up on the matter.

alexrainman commented 7 years ago

@woellij i want to make both position and ItemsSource observable for a real MVVM behavior.

alexrainman commented 7 years ago

@woellij i think i found the way to make it work the way you want but only ItemsSource for now. To make the control to move between pages changing position binding i will have to redesign bunch of things.

alexrainman commented 7 years ago

@woellij this is possible but losing a great functionality the plugin has: sliding to previous page when current page is removed which i want to maintain. Maybe creating a copy of the source in the renderer itself? Will try that.

woellij commented 7 years ago

yeah, switching to the previous page when the current one is removed, seems to be a bit buggy with my hacky soluation too :) i didn't look much more into it, since i'm just in the prototyping stage at the moment and needed the "Update View from ItemsSource functionality" at all costs.

For now i basically just hooked up the CollectionChange Listener and removed all modifications of the ItemsSource from the Renderer (it mainly updates the Adapter now)

I'm not sure there is a use-case for "removing an item from the listview should remove it from the itemssource" - i can't remember i've ever used it that way. Even if there was a use-case for that one could just find the associated item from the ItemsSource and remove it there to have it removed from the listview. (i'm not sure, whether you meant that kind of behavior with "each element in the source is tied to a view in the UI, both of them should be in sync, so if you remove an item from the source you should remove a view from UI no matter the direction you do it"

it's great you're actively working on it and investigating the possibilities, since - like i said - it's a much needed component in the Xamarin.Forms world (and if it works super well with MVVM pattern aswell then even better ;) )

Once i have the time i will try to contribute something

alexrainman commented 7 years ago

@woellij from the beginning i tried to make ItemsSource observable but Android ViewPager introduces limitations and part of design was to slide to previous page when current one is removed for a nicer UX. And yes, the number of pages in the carousel is tied to the number of elements in the source because Android ViewPager mentioned limitations.

alexrainman commented 7 years ago

@woellij you will get ilegal exception if you try to remove the current item from source because of the "slide when remove" functionality. If you don't need that then it will work, i tested myself and it's very simple.

alexrainman commented 7 years ago

I can create a different repo with the implementation.

alexrainman commented 7 years ago

If my work is helping you, please help me back: https://xamarinhq.wufoo.com/forms/nominate-a-xamarin-mvp/

alexrainman commented 7 years ago

This is what i have done that is community visible: