CarouselView #853
CarouselView #853
Conversation
alexrainman
changed the title
|
Can you explain why you'd want to do this ? |
|
You can talk with Dave Ortinau and Samantha Houts about it. |
|
Dave contacted me about their wish to pull my CarouselView into the core. He asked me to follow a Pull Request channel for it. Thanks |
|
Great. Add that info to the PR description. thanks |
|
So I take it this is not the CarouselView Xamarin was working on? What went wrong with the other one? |
|
No this is not Xamarin's one. I have been maintaining this control for a while and its a really stable piece of code with a big community behind it. |
There was a problem hiding this comment.
I've only reviewed the Core part so far, and if this control is considered for inclusion, it should go through the evolution forum so the public API can be discussed.
I've glanced at the renderers implementation and a few things looked weird:
- I don't think the control reacts at every BP changes, and BP are mutable by nature. e.g. if you change the ItemTemplate, the current views doesn't refresh,
- the renderers are adding items to the user ItemSource collection
| } | ||
|
|
||
| // Android and iOS only | ||
| public static readonly BindableProperty InterPageSpacingProperty = BindableProperty.Create("InterPageSpacing", typeof(int), typeof(CarouselView), 0); |
There was a problem hiding this comment.
Platform specifics are usually set with attached BPs in PlatfromSpecifics
There was a problem hiding this comment.
Basically, move these platform specific properties out of the Core implementation and put them in Xamarin.Forms.Core\PlatformConfiguration\AndroidSpecific\CaourselView.cs (for example) and follow the established pattern for Platform Specifics there. Then in your renderer, you will do something like var interPageSpacing = e.NewElement.OnThisPlatform().GetInterPageSpacing() * metrics.Density; to access the values.
| set { SetValue(PageIndicatorTintColorProperty, value); } | ||
| } | ||
|
|
||
| public static readonly BindableProperty CurrentPageIndicatorTintColorProperty = BindableProperty.Create("CurrentPageIndicatorTintColor", typeof(Color), typeof(CarouselView), Color.Gray); |
There was a problem hiding this comment.
default is usually Color.Default, and the platform implementation is free to pick gray for default color, or anything else
There was a problem hiding this comment.
Why indicators default color cannot be gray?
| set { SetValue(ItemTemplateProperty, value); } | ||
| } | ||
|
|
||
| public static readonly BindableProperty PositionProperty = BindableProperty.Create("Position", typeof(int), typeof(CarouselView), 0, BindingMode.TwoWay); |
There was a problem hiding this comment.
can you do negative values ?
There was a problem hiding this comment.
shouldn't the value be validated, or at least coerced ?
There was a problem hiding this comment.
I do validate position property in platform specific code. Not negative values (collections index start in 0, don't?)
|
|
||
| public EventHandler PositionSelected; | ||
|
|
||
| public Func<object, int, Task> InsertAction; |
There was a problem hiding this comment.
that's not an easy public API to understand
There was a problem hiding this comment.
InsertAction is not supposed to be public but, how i implement it in platform specific code then? If i can find a way to use ObservableCollection then this will go away.
There was a problem hiding this comment.
Members that are only public because they need to be accessed by the renderer are typically put into ICarouselViewController interface and then implemented explicitly on the element so as to hide them from isense for app developers. We are considering a simpler solution whereby you need only add [EditorBrowsable(EditorBrowsableState.Never)] but that PR is limbo at the moment...
There was a problem hiding this comment.
Got your idea but maybe i don't need those methods at all :)
| { | ||
| public class CarouselViewException : Exception | ||
| { | ||
| public CarouselViewException() |
There was a problem hiding this comment.
not sure CarouselView needs its own exception subclass
| { | ||
| get { return (DataTemplate)GetValue(ItemTemplateProperty); } | ||
| set { SetValue(ItemTemplateProperty, value); } | ||
| } |
There was a problem hiding this comment.
DataTemplate inflation code should be part of the Core, not delegated to renderers
There was a problem hiding this comment.
Not possible as Xamarin.Forms Views need to be added to native controls in their native representation.
There was a problem hiding this comment.
the templates are inflated into a View, then that view is rendered
There was a problem hiding this comment.
It doesn't work like that. I do that exactly but, xamarin.forms view cannot be added to platform specific controls. I need to use native views. Also, the inflation/convertion is done on demand as each control use platform specific virtualization (except for UWP).
| /// <summary> | ||
| /// CarouselView Interface | ||
| /// </summary> | ||
| public class CarouselView : View //Layout<View> |
There was a problem hiding this comment.
should inherit from ItemsView<>
There was a problem hiding this comment.
Why? Platform specific controls are Views not collections.
There was a problem hiding this comment.
to share the templating logic with ListView
There was a problem hiding this comment.
But why? ListView is a collection, CarouselView is not.
There was a problem hiding this comment.
Not requiring derivation of ListView certainly allows for a simpler implementation.
There was a problem hiding this comment.
nobody said ListView. I said ItemsView<>. There's nothing in there you do not want.
|
I will answer all your questions. |
|
Stephane, this control is the result of the community collaboration and it's really stable.
Android: Android.View.Views
I have an idea how i can solve this but, it will mean two things:
Believe me, this the best way to do this. I has been working with each platform specific control for a while. By the way, UWP has a lot of limitations comparing to WP. Thanks |
| { | ||
| public static FrameworkElement ConvertFormsToNative(Xamarin.Forms.View view, Rectangle size) | ||
| { | ||
| //var vRenderer = RendererFactory.GetRenderer (view); |
There was a problem hiding this comment.
Are these commented out intentionally? If so, they should just be removed.
There was a problem hiding this comment.
Anything commented is intentional and there's a reason for that. The platform has some limitations. What is commented is the way it supposed to be :)
There was a problem hiding this comment.
Could you add some brief explanation of the limitations and why the code is different for this platform? Something like "normally, we'd do X but on Windows we have to do Y because...". That way future maintainers won't waste time trying to do 'X'.
There was a problem hiding this comment.
The biggest limitation i have in UWP is that DataTemplate doesn't support parameters like Xamarin.Forms DataTemplate does.
If UWP DataTemplate can support Func<object> as parameter, i wont need to render all the pages ahead of time. I will use a DataTemplateSelector instead and render the views on demand and use ItemsSource as FlipView source directly.
|
|
||
| view.Layout(new Rectangle(0, 0, size.Width, size.Height)); | ||
|
|
||
| //vRenderer.ContainerElement.Arrange(new Rect(0, 0, size.Width, size.Height)); |
There was a problem hiding this comment.
This one, too - is this intentional?
| { | ||
| public static UIView ConvertFormsToNative(Xamarin.Forms.View view, CGRect size) | ||
| { | ||
| //var vRenderer = RendererFactory.GetRenderer (view); |
There was a problem hiding this comment.
Is this intentionally commented out?
There was a problem hiding this comment.
Yes. I also have a pending UWP reported issue to solve.
| @@ -182,6 +182,12 @@ | |||
| <Compile Include="..\Xamarin.Forms.Platform.WinRT\LayoutExtensions.cs"> | |||
| <Link>LayoutExtensions.cs</Link> | |||
| </Compile> | |||
| <Compile Include="CarouselView\FlipViewControl.xaml.cs"> | |||
| <DependentUpon>FlipViewControl.xaml</DependentUpon> | |||
There was a problem hiding this comment.
The new .xaml files will need entries for their .xbf files in the UWP section of Xamarin.Forms.nuspec.
There was a problem hiding this comment.
I do that in my own .nuspec but i didn't touch Xamarin.Forms.nuspec
| else | ||
| viewPager.Adapter.NotifyDataSetChanged(); | ||
|
|
||
| await Task.Delay(100); |
|
|
||
| viewPager.SetCurrentItem(1, Element.AnimateTransition); | ||
|
|
||
| await Task.Delay(100); |
|
|
||
| viewPager.SetCurrentItem(newPos, Element.AnimateTransition); | ||
|
|
||
| await Task.Delay(100); |
There was a problem hiding this comment.
I will add comments to all my code. It may seems weird in Android but it's the only way to make it work.
There was a problem hiding this comment.
@alexrainman: In my testing of your control, these Delays were unnecessary and made the whole app hang for many seconds under load testing (i.e., adding thousands of Views to the CarouselView). I removed them and noticed no side effects.
|
Not requiring CarouselView be abstracted as a IObservableCollection (and instead be a linked list) vastly simplifies the public surface area and the test matrix. Not requiring the Android/iOS renders use RecylcerView/UICollectionView (and instead used ViewPager/UIPageViewController) vastly simplifies the implementation. |
|
Agree. Anyway, i am testing some code that will allow me to use IObservableCollection as ItemsSource but it will require lot of regression testing in Android. |
|
Hm. I'd forgo IObservableCollection support. |
|
|
||
| namespace Xamarin.Forms.Controls | ||
| { | ||
| public partial class CarouselViewGalleryPage : ContentPage |
There was a problem hiding this comment.
It's customary to automate the UITests. Please add labels which expose all the bindable properties instead of dumping to the debug log (e.g. position, ItemSource.Length, etc). This will allow you to add assertions in the UITest that the values are as you expect them to be. E.g. that when an item is inserted to the left of the current item that the Position BP is bumped (leaving the current item unchanged) and when to the right is not. In general, UITests should be added for every button given various states of the CarouselView (number of elements/position).
|
@kingces95 to support IObservableCollection (interface that doesn't exists) i have to change ItemsSource BP to |
|
@alexrainman ListView supports observables. the ItemsSource is |
|
Got it. |
|
Can you point me to that implementation? |
|
I'd vote against supporting ObservableCollection (at least at this moment). I'm sure @alexrainman could support it and it may not even be that hard (given the use of ViewPager over RecyclerView). However, just because we can do something doesn't mean we should. Or just because we've been doing something we should continue to do it that way. Customers don't seem to miss ObservableCollection support as they're using this implementation now. Jason and Sam (and I) don't seem to care about ObservableCollection support as they've already approved the PR. And if even if we ultimately decide to support ObservableCollection that doesn't mean it has to be done now. The implementation works as it stands. I'd vote to accept the public API for purposes of this PR and then bolt on additional features as they are requested by customers. If we ask @alexrainman to do anything more I'd rather he add UITests for the existing surface area than add more features to it. |
| ObservableCollection<int> _Source; | ||
| /// <summary> | ||
| /// Images | ||
| /// </summary> |
There was a problem hiding this comment.
@alexrainman Are these summaries needed? There are other examples in different files as well. Maybe we need to make them better or completely remove them?
|
Does this need to be rebased with the updates included? |
|
Yes, i will include them before end of week. |
|
@alexrainman i have created a branch with fixes and updates do you mind we work on that branch ? i will open a new PR with your comments, or you can even open yourself using that branch. This is already rebased on master https://github.com/xamarin/Xamarin.Forms/tree/feature-carouselview |
|
@rmarinho by fixes and updates you mean thr latest i need to include in the PR? |
|
@alexrainman i rebased your branch on top of master, i m now looking at the code, there's a lot of stuff here that we need to clean up to be able to merge this. i recommend we work in that new branch and we will help improving the code and maybe adding more UITests etc. |
|
@rmarinho first i will update with the latest release of the control. We can start from that. |
|
@alexrainman you have another repo with the control ? |
|
@rmarinho of course. This is just a PR based on my control that has been out for couple of years. @davidortinau asked me to do the PR. You can find it here: https://github.com/alexrainman/CarouselView. Some new issues has been reported which i need to confirm. |
|
@alexrainman Can you update this PR (or the new branch Rui mentions) to the latest release for now and re-route the issues to XF repository once the control is merged? This way, the Xamarin team can fix the bugs themselves. My concern is that as you fix bugs listed on your repro, more will appear over time and you'll have to fix them too, and the cycle continues. |
|
@hartez @alexrainman @davidortinau @jassmith Why was this PR closed? I am confused. What does it mean, is CarouselView not getting implemented in XF or? There's no comment, no follow up, this was just closed. For someone who needs CarouselView and wants to understand what's happening, this isn't helpful... Can someone please comment? |
|
@andreinitescu this has been moved to a branch. I has been sick and out for a while so that branch is not updated yet. |
|
@rmarinho i wonder why i cannot find that branch when i try to switch from master using sourcetree |
|
@rmarinho found it |
|
The new branch will allow more flexibility to XF team and others, so we need to get that branch update with latest changes @alexrainman , instead of PR to master, you PR to that branch, that will also allow us to run the CI etc.. @andreinitescu if you can help we will need to review a lot of code on this pr, remove dead code and comments, and test it. Thanks |
|
@rmarinho i will be done incorporating latest code in the new branch later today. We can start from that |
|
@alexrainman is there a way to get the curent item View? It would be sweet to have something like a |
|
@andreinitescu that is not available but i think it is doable. |
|
@alexrainman Thanks. Any way to do it now? I can't see it |
|
@rmarinho i am trying to merge latest code into the branch named "feature-carouselview" and i get this error: Any ideas? |
|
@alexrainman make a PR that targets the feature Carouselview branch |
|
When is this control going to be available? Is there an ETA? It would be awesome! |
|
any news on this control? Its not mergedt into XF 3.0 right ? I'm eagerly waiting for some enhancements |
|
Any update on this? |
|
It seems Xamarin.Forms team has decided not to include my control and build their own from scratch so, i am working on the next release. |
|
I faced DivideByZeroException in android devices. This issue was described here https://bugzilla.xamarin.com/show_bug.cgi?id=53382 To fix it i had to write a component where I checked if the CarouselView is visible and items source is not null. Also i had to set an ItemTemplate and an ItemsSource in the main thread. I hope, you will fix this issue in the future. |
Description
Pulling CarouselView control into Xamarin.Forms.Core (As requested by Dave Ortinau)
File list
Xamarin.Forms.Core
Xamarin.Forms.Platform.Android
Xamarin.Forms.Platform.iOS
Xamarin.Forms.Platform.UAP
ControlGallery/Xamarin.Forms.Controls (portable)
PR Checklist