Make WinRT/UWP platform classes more maintainable #652

Merged
merged 5 commits into from Jan 28, 2017

Conversation

Projects
None yet
7 participants
@hartez
Member

hartez commented Dec 16, 2016

Description of Change

The WinRT/UWP shared Platform.cs and NavigationPageRenderer.cs files share the bulk of their code; the many interlaced conditional compilation statements make them difficult to modify and review changes in.

This change is aimed at reducing the number of conditional compilation statements and making it clearer how the platform and navigation pages behave on each platform.

This also allows for the removal of a few suppressed Resharper warnings about methods which are marked async and lack await operators.

No tests, this is just a refactoring.

Bugs Fixed

Countless future ones :)

API Changes

None

Behavioral Changes

None

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
+ if (provider == null) // || (titleProvider != null && !titleProvider.ShowTitle))
+ return null;
+
+ return await provider.GetCommandBarAsync();

This comment has been minimized.

@campersau

campersau Dec 16, 2016

async and await are not needed here.

@campersau

campersau Dec 16, 2016

async and await are not needed here.

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

also, remove the comments

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

also, remove the comments

This comment has been minimized.

@hartez

hartez Dec 27, 2016

Member

Good points. I'll simplify that method.

@hartez

hartez Dec 27, 2016

Member

Good points. I'll simplify that method.

@StephaneDelcroix

this is so beautiful I almost wish all of our platforms were defined in a single Shared Asset Projects.

Or not

+ if (provider == null) // || (titleProvider != null && !titleProvider.ShowTitle))
+ return null;
+
+ return await provider.GetCommandBarAsync();

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

also, remove the comments

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

also, remove the comments

@@ -189,7 +177,11 @@ public void SetElement(VisualElement element)
LookupRelevantParents();
UpdateTitleColor();
UpdateNavigationBarBackground();
- UpdateToolbarPlacement();
+
+#if WINDOWS_UWP

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

this one isn't required. you could have UpdateToolBarPlacement defined to no-op on the platforms not supporting it

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

this one isn't required. you could have UpdateToolBarPlacement defined to no-op on the platforms not supporting it

This comment has been minimized.

@hartez

hartez Dec 27, 2016

Member

Good point. Will update.

@hartez

hartez Dec 27, 2016

Member

Good point. Will update.

@@ -207,8 +199,11 @@ public void SetElement(VisualElement element)
protected void Dispose(bool disposing)
{
- if (!disposing || _disposed)
+ if (_disposed || !disposing)

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

subtle !

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

subtle !

-#else
- object defaultColor = Windows.UI.Xaml.Application.Current.Resources["ApplicationPageBackgroundThemeBrush"];
-#endif
+ object defaultColor = GetDefaultColor();

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

conditional compilation like god intended to

@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

conditional compilation like god intended to

@huangjinshe

This comment has been minimized.

Show comment
Hide comment
@huangjinshe

huangjinshe Dec 21, 2016

I've changed these 2 class before. Can't you change it based on mine?

I've changed these 2 class before. Can't you change it based on mine?

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Jan 12, 2017

Member

@hartez needs to be rebased

Member

rmarinho commented Jan 12, 2017

@hartez needs to be rebased

hartez added some commits Dec 30, 2016

+
+ var toolBarProvider = GetToolbarProvider() as IToolBarForegroundBinder;
+
+ foreach (ToolbarItem item in _toolbarTracker.ToolbarItems.OrderBy(ti => ti.Priority))

This comment has been minimized.

@rmarinho

rmarinho Jan 28, 2017

Member

If toolBarProvider is null we don't need anything that is below

@rmarinho

rmarinho Jan 28, 2017

Member

If toolBarProvider is null we don't need anything that is below

+ if (_showTitle || (render != null && render.ShowTitle))
+ {
+ var platform = Element.Platform as Platform;
+ if (platform != null)

This comment has been minimized.

@rmarinho

rmarinho Jan 28, 2017

Member

await platform?.UpdateToolbarItems();

@rmarinho

rmarinho Jan 28, 2017

Member

await platform?.UpdateToolbarItems();

@rmarinho rmarinho merged commit 11331bc into master Jan 28, 2017

2 checks passed

OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3717, ignored: 10
Details

@hartez hartez deleted the win-platform-cleanup branch May 16, 2017

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

@samhouts samhouts modified the milestone: 3.1.0 Jun 1, 2018

@samhouts samhouts added this to Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts removed this from Done in vNext+1 (master) Jun 26, 2018

@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