Android fastrenderers #845

Merged
merged 34 commits into from Apr 6, 2017

Conversation

Projects
None yet
6 participants
@rmarinho
Member

rmarinho commented Mar 29, 2017

Description of Change

Avoid using our generic wrapper for a couple of renderers, some refactoring was also done in the process

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

hartez and others added some commits Feb 27, 2017

+ if (isOnParentRenderer &&
+ PlatformViewType != PlatformViews.BoxView &&
+ PlatformViewType != PlatformViews.Frame
+#if __ANDROID__

This comment has been minimized.

@rmarinho

rmarinho Mar 30, 2017

Member

@hartez is this correct or did i screw the rebase ?

@rmarinho

rmarinho Mar 30, 2017

Member

@hartez is this correct or did i screw the rebase ?

This comment has been minimized.

@hartez

hartez Mar 30, 2017

Member

That looks correct. The UI tests will catch it if not.

@hartez

hartez Mar 30, 2017

Member

That looks correct. The UI tests will catch it if not.

RegisterHandlerForDefaultRenderer(typeof(Switch), typeof(AppCompat.SwitchRenderer), typeof(SwitchRenderer));
RegisterHandlerForDefaultRenderer(typeof(Picker), typeof(AppCompat.PickerRenderer), typeof(PickerRenderer));
- RegisterHandlerForDefaultRenderer(typeof(Frame), typeof(AppCompat.FrameRenderer), typeof(FrameRenderer));
+ RegisterHandlerForDefaultRenderer(typeof(Frame), typeof(FastRenderers.FrameRenderer), typeof(FrameRenderer));
RegisterHandlerForDefaultRenderer(typeof(CarouselPage), typeof(AppCompat.CarouselPageRenderer), typeof(CarouselPageRenderer));

This comment has been minimized.

@rmarinho

rmarinho Mar 30, 2017

Member

No ImageRenderer for AppCompat so we don't see any change here.

@rmarinho

rmarinho Mar 30, 2017

Member

No ImageRenderer for AppCompat so we don't see any change here.

+
+namespace Xamarin.Forms.Platform.Android.FastRenderers
+{
+ internal class Accessibilitizer : IDisposable // TODO Think of better name

This comment has been minimized.

@rmarinho

rmarinho Mar 30, 2017

Member

Do we want to think on this now ?

@rmarinho

rmarinho Mar 30, 2017

Member

Do we want to think on this now ?

+ void SetContentDescription(string contentDescription = GetFromElement)
+ {
+ if (Element == null || Control == null)
+ {

This comment has been minimized.

@rmarinho

rmarinho Mar 30, 2017

Member

we could drop the curly braces for 1 line ifs and save some loc

@rmarinho

rmarinho Mar 30, 2017

Member

we could drop the curly braces for 1 line ifs and save some loc

+ void SetLabeledBy()
+ {
+ if (Element == null || Control == null)
+ return;

This comment has been minimized.

@rmarinho

rmarinho Mar 30, 2017

Member

hehe like i said above i prefer this sintax

@rmarinho

rmarinho Mar 30, 2017

Member

hehe like i said above i prefer this sintax

+ {
+ var id = Control.Id;
+ if (id == -1)
+ id = Control.Id = FormsAppCompatActivity.GetUniqueId();

This comment has been minimized.

@rmarinho

rmarinho Mar 30, 2017

Member

Here we should have curly braces as we are inside other if

@rmarinho

rmarinho Mar 30, 2017

Member

Here we should have curly braces as we are inside other if

using Android.Support.V7.Widget;
using Android.Views;
using AColor = Android.Graphics.Color;
using AView = Android.Views.View;
-namespace Xamarin.Forms.Platform.Android.AppCompat
+namespace Xamarin.Forms.Platform.Android.FastRenderers

This comment has been minimized.

@rmarinho

rmarinho Mar 30, 2017

Member

Should we keep the old one for back compact ?

@rmarinho

rmarinho Mar 30, 2017

Member

Should we keep the old one for back compact ?

This comment has been minimized.

@hartez

hartez Mar 30, 2017

Member

Do you mean keep the old namespace? I thought we had to put them in a different namespace so people could still use the old renderers.

@hartez

hartez Mar 30, 2017

Member

Do you mean keep the old namespace? I thought we had to put them in a different namespace so people could still use the old renderers.

This comment has been minimized.

@samhouts

samhouts Mar 30, 2017

Member

But in this case, FrameRenderer is the old renderer.

@samhouts

samhouts Mar 30, 2017

Member

But in this case, FrameRenderer is the old renderer.

This comment has been minimized.

@hartez

hartez Mar 30, 2017

Member

Oh, I see. I guess for the moment we should move it back to the old namespace, then. Otherwise, someone will have to change their code.

@hartez

hartez Mar 30, 2017

Member

Oh, I see. I guess for the moment we should move it back to the old namespace, then. Otherwise, someone will have to change their code.

+ public event EventHandler<VisualElementChangedEventArgs> ElementChanged;
+ public event EventHandler<PropertyChangedEventArgs> ElementPropertyChanged;
+#pragma warning disable 414

This comment has been minimized.

@rmarinho

rmarinho Mar 30, 2017

Member

@hartez do we need this here ? i added the #praga so we can make it build .. but maybe we should just remove it..

@rmarinho

rmarinho Mar 30, 2017

Member

@hartez do we need this here ? i added the #praga so we can make it build .. but maybe we should just remove it..

This comment has been minimized.

@hartez

hartez Mar 30, 2017

Member

It's probably just left over from the rebase.

@hartez

hartez Mar 30, 2017

Member

It's probably just left over from the rebase.

hartez added some commits Apr 3, 2017

@rmarinho rmarinho merged commit 425fafb into master Apr 6, 2017

2 of 3 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests faile…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3755, ignored: 10
Details
@assemhakmeh

This comment has been minimized.

Show comment
Hide comment
@assemhakmeh

assemhakmeh Apr 26, 2017

I noticed the VisualElementRenderer adds listeners on the Element for property changes in the OnElementChanged handler but the renderers also propagate the property change events. This seems to result in property changes on being processed twice. Seems like a potential issue.

assemhakmeh commented Apr 26, 2017

I noticed the VisualElementRenderer adds listeners on the Element for property changes in the OnElementChanged handler but the renderers also propagate the property change events. This seems to result in property changes on being processed twice. Seems like a potential issue.

@rmarinho rmarinho deleted the android-fastrenderers branch Apr 26, 2017

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 May 9, 2017

Contributor

@rmarinho @hartez I just rebased master as of #908 and am getting the following when Android is starting up:

System.MissingMethodException: Method 'Xamarin.Forms.Platform.Android.FormsViewGroup.SendViewBatchUpdate' not found.

I believe this is due to the new fast renderers. Have you guys seen this issue before?

Contributor

adrianknight89 commented May 9, 2017

@rmarinho @hartez I just rebased master as of #908 and am getting the following when Android is starting up:

System.MissingMethodException: Method 'Xamarin.Forms.Platform.Android.FormsViewGroup.SendViewBatchUpdate' not found.

I believe this is due to the new fast renderers. Have you guys seen this issue before?

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez May 9, 2017

Member

@adrianknight89 that sounds like the Xamarin.Forms.Platform.Android.FormsViewGroup project isn't being rebuilt with the latest version of formsviewgroup.jar. If you rebuild that project, does the problem persist?

Member

hartez commented May 9, 2017

@adrianknight89 that sounds like the Xamarin.Forms.Platform.Android.FormsViewGroup project isn't being rebuilt with the latest version of formsviewgroup.jar. If you rebuild that project, does the problem persist?

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 May 9, 2017

Contributor

@hartez Unfortunately, that didn't fix anything. :( I compiled FormsViewGroup, Xamarin.Forms.Platform.Android, and Xamarin.Forms.Platform.Android (Forwarders) in that order.

Also, I noticed FormsViewGroup appears as a reference in Xamarin.Forms.Platform.Android and Xamarin.Forms.Platform.Android (Forwarders). Not sure if this is causing the issue. Since it's part of Android, is there a reason why it should be part of Forwarders also? Forwarders has other assemblies as well that appear in Android. Maybe they should all be cleaned up?

Contributor

adrianknight89 commented May 9, 2017

@hartez Unfortunately, that didn't fix anything. :( I compiled FormsViewGroup, Xamarin.Forms.Platform.Android, and Xamarin.Forms.Platform.Android (Forwarders) in that order.

Also, I noticed FormsViewGroup appears as a reference in Xamarin.Forms.Platform.Android and Xamarin.Forms.Platform.Android (Forwarders). Not sure if this is causing the issue. Since it's part of Android, is there a reason why it should be part of Forwarders also? Forwarders has other assemblies as well that appear in Android. Maybe they should all be cleaned up?

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 May 9, 2017

Contributor

@hartez You were right. FormsViewGroup was out of sync in the client project. I was looking at XForms files only. Thanks for your help! :)

Contributor

adrianknight89 commented May 9, 2017

@hartez You were right. FormsViewGroup was out of sync in the client project. I was looking at XForms files only. Thanks for your help! :)

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez May 10, 2017

Member

@adrianknight89 Awesome, glad that fixed it!

Member

hartez commented May 10, 2017

@adrianknight89 Awesome, glad that fixed it!

@samhouts samhouts added D-15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts modified the milestones: 2.3.0, 2.3.5 Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment