Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MacOS] make OnElementChanged and OnElementPropertyChanged protected virtual #1187

Merged
merged 2 commits into from Oct 17, 2017

Conversation

@MichaelRumpler
Copy link
Contributor

@MichaelRumpler MichaelRumpler commented Oct 6, 2017

Description of Change

In Xamarin.Forms.Platform.MacOS the PageRenderer and ScrollViewRenderer did not have the usual protected virtual methods OnElementChanged and OnElementPropertyChanged. They were private and partly wrong named. Therefore users could not inherit from those renderers and use them as every other renderer on every other platform.

API Changes

Changed:

  • PageRenderer: void OnElementChanged => protected virtual void OnElementChanged
  • PageRenderer: void OnHandlePropertyChanged => protected virtual void OnElementPropertyChanged
  • ScrollViewRenderer: void OnElementChanged => protected virtual void OnElementChanged
  • ScrollViewRenderer: void HandlePropertyChanged => protected virtual void OnElementPropertyChanged

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
Copy link
Member

@StephaneDelcroix StephaneDelcroix left a comment

fine for me, minus the comment

@@ -125,7 +125,7 @@ protected override void Dispose(bool disposing)
base.Dispose(disposing);
}

void OnElementChanged(VisualElementChangedEventArgs e)
protected virtual void OnElementChanged(VisualElementChangedEventArgs e)
Copy link
Member

@StephaneDelcroix StephaneDelcroix Oct 9, 2017

we try to not trigger events from virtual methods.

@MichaelRumpler
Copy link
Contributor Author

@MichaelRumpler MichaelRumpler commented Oct 9, 2017

Good call, but you do raise ElementChanged yourself from CarouselPageRenderer.OnElementChanged, MasterDetailPageRenderer.OnElementChanged, NavigationPageRenderer.OnElementChanged, TabbedPageRenderer.OnElementChanged and even VisualElementRenderer.OnElementChanged.

I changed all the *PageRenderers and ScrollViewRenderer now. The event is now raised in the private Raise(or Handle-)ElementChanged method and the virtual OnElementChanged method is just an empty template method. Thus the ElementChanged event is still raised even if a derived class does not call base.OnElementChanged.

I did not change the VisualElementRenderer as this is used everywhere (also shared with iOS).

@StephaneDelcroix
Copy link
Member

@StephaneDelcroix StephaneDelcroix commented Oct 9, 2017

@MichaelRumpler

but you do raise ElementChanged yourself from CarouselPageRenderer.OnElementChanged, MasterDetailPageRenderer.OnElementChanged, NavigationPageRenderer.OnElementChanged, TabbedPageRenderer.OnElementChanged and even VisualElementRenderer.OnElementChanged.

and on BindableObject.PropertyChanged too... and you can't start to imagine how we regret it

@StephaneDelcroix StephaneDelcroix merged commit 347c340 into xamarin:master Oct 17, 2017
9 of 13 checks passed
@samhouts samhouts added this to the 3.0.0 milestone May 5, 2018
@samhouts samhouts removed this from the 3.0.0 milestone Aug 23, 2019
@samhouts samhouts added this to the 2.5.0 milestone Aug 23, 2019
@samhouts samhouts removed this from the 2.5.0 milestone Aug 23, 2019
@samhouts samhouts added this to the 3.0.0 milestone Aug 23, 2019
PureWeen added a commit that referenced this issue Oct 24, 2019
…virtual (#1187)

* make OnElementChanged and OnElementPropertyChanged protected virtual like all other renderers

* raise ElementChanged outside virtual OnElementChanged method
@samhouts samhouts removed this from the 3.0.0 milestone Oct 29, 2019
@samhouts samhouts added this to the 2.5.0 milestone Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants