Update RelativeLayout to make it respond to constraint changes #425

Merged
merged 7 commits into from Feb 2, 2017

Conversation

Projects
None yet
8 participants
@activa
Contributor

activa commented Oct 6, 2016

Description of Change

Constraints of a RelativeLayout are bindable properties but the layout does not update when the constraints are updated.

This change will invalidate the layout whenever XConstraint, YConstraint, WidthConstraint or HeightConstraint is changed (either in code or through a change in the bound property)

API Changes

Methods added:

  • RelativeLayout.SetXConstraint(BindableObject,Constraint)
  • RelativeLayout.SetYConstraint(BindableObject,Constraint)
  • RelativeLayout.SetWidthConstraint(BindableObject,Constraint)
  • RelativeLayout.SetHeightConstraint(BindableObject,Constraint)

Behavioral Changes

RelativeLayout will now invalidate itself when any of the constraint expressions on children change. Since constraints are bindable properties, this is the expected behavior.

In the exisiting implementation, it was impossible to update the constraints after the layout was created, even from code. The only way to change the layout was to update the BoundsConstraint property and call InvalidateLayout. Changing XConstraint, YConstraint, WidthConstraint or HeightConstraint had no effect.

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
@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Oct 6, 2016

Hi @activa, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

dnfclas commented Oct 6, 2016

Hi @activa, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

Xamarin.Forms.Core/RelativeLayout.cs
- public static readonly BindableProperty HeightConstraintProperty = BindableProperty.CreateAttached("HeightConstraint", typeof(Constraint), typeof(RelativeLayout), null);
+ public static readonly BindableProperty HeightConstraintProperty = BindableProperty.CreateAttached("HeightConstraint", typeof(Constraint), typeof(RelativeLayout), null, BindingMode.OneWay, null, ConstraintChanged);

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 6, 2016

Contributor

If you don't need to set a custom binding mode and validation values, use propertyChanged: ConstraintChanged and remove the defaults that precede it.

@adrianknight89

adrianknight89 Oct 6, 2016

Contributor

If you don't need to set a custom binding mode and validation values, use propertyChanged: ConstraintChanged and remove the defaults that precede it.

@StephaneDelcroix

This looks ok, even if it's probably expensive, but it requires a few unit tests

if (boundsConstraint == null)
{
throw new Exception("BoundsConstraint should not be null at this point");
}
- result = boundsConstraint.Compute();
+
+ var result = boundsConstraint.Compute();
return result;

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Oct 9, 2016

Member

while you're at it, you can avoid defining and assigning the result variable

@StephaneDelcroix

StephaneDelcroix Oct 9, 2016

Member

while you're at it, you can avoid defining and assigning the result variable

This comment has been minimized.

@activa

activa Oct 10, 2016

Contributor

I saw that too, but I didn't want to fix 2 unrelated things in one pull request.

@activa

activa Oct 10, 2016

Contributor

I saw that too, but I didn't want to fix 2 unrelated things in one pull request.

@activa

This comment has been minimized.

Show comment
Hide comment
@activa

activa Oct 10, 2016

Contributor

The change is expensive when the extra functionality is being used. If the constraints are not updated at runtime, it has no performance impact.

I'll try to add unit tests.

Contributor

activa commented Oct 10, 2016

The change is expensive when the extra functionality is being used. If the constraints are not updated at runtime, it has no performance impact.

I'll try to add unit tests.

@activa

This comment has been minimized.

Show comment
Hide comment
@activa

activa Nov 28, 2016

Contributor

Any update on this PR? It has been "under-review" for a long time. @rmarinho

Contributor

activa commented Nov 28, 2016

Any update on this PR? It has been "under-review" for a long time. @rmarinho

@StephaneDelcroix

Thanks for the heads up, @activa.

I added some extra comments, could you please look at it ?

Also, the documentation needs to be updated, and the changes added to this PR. There are scripts to help you doing that at the root of the repository.

When this is done, don't hesitate to ping me, I'll get a second pair of eyes on this, and hopefully we'll get this merged.

@@ -75,6 +75,47 @@ public void SimpleLayout ()
}
+ [Test]
+ public void LayoutChangesAtRuntime()

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Nov 28, 2016

Member

rename to something like 'LayoutISUpdatedWhenConstraintChanges'

@StephaneDelcroix

StephaneDelcroix Nov 28, 2016

Member

rename to something like 'LayoutISUpdatedWhenConstraintChanges'

+ {
+ bindable.SetValue(YConstraintProperty, value);
+ }
+

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Nov 28, 2016

Member

you should add getters as well.

@StephaneDelcroix

StephaneDelcroix Nov 28, 2016

Member

you should add getters as well.

This comment has been minimized.

@activa

activa Nov 28, 2016

Contributor

Getters were already there. I just added the setters.

@activa

activa Nov 28, 2016

Contributor

Getters were already there. I just added the setters.

Xamarin.Forms.Core/RelativeLayout.cs
+ RelativeLayout.SetXConstraint(view, xConstraint);
+ RelativeLayout.SetYConstraint(view, yConstraint);
+ RelativeLayout.SetWidthConstraint(view, widthConstraint);
+ RelativeLayout.SetHeightConstraint(view, heightConstraint);

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Nov 28, 2016

Member

these 4 calls should be wrapped between a BatchBegin() and a BatchCommit() calls.

@StephaneDelcroix

StephaneDelcroix Nov 28, 2016

Member

these 4 calls should be wrapped between a BatchBegin() and a BatchCommit() calls.

@activa

This comment has been minimized.

Show comment
Hide comment
@activa

activa Nov 28, 2016

Contributor

Requested changes were committed @StephaneDelcroix

Contributor

activa commented Nov 28, 2016

Requested changes were committed @StephaneDelcroix

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Nov 28, 2016

Member

@activa still missing the documentation

Member

StephaneDelcroix commented Nov 28, 2016

@activa still missing the documentation

@activa

This comment has been minimized.

Show comment
Hide comment
@activa

activa Nov 28, 2016

Contributor

I guess only the attached property setters need documentation? That's the only public API that has changed.

Stupid question: how do I update the documentation? Surely I'm not supposed to edit the XML doc files manually, right? What tool do you guys use?

Contributor

activa commented Nov 28, 2016

I guess only the attached property setters need documentation? That's the only public API that has changed.

Stupid question: how do I update the documentation? Surely I'm not supposed to edit the XML doc files manually, right? What tool do you guys use?

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Nov 28, 2016

Member

yes, only the setters.

There are scripts to help you doing that at the root of the repository.

if you're using a mac, make docs, on windows it's probably update-docs.ps1. Use the interactive git add so you don't change anything but the 4 new static methods.

Member

StephaneDelcroix commented Nov 28, 2016

yes, only the setters.

There are scripts to help you doing that at the root of the repository.

if you're using a mac, make docs, on windows it's probably update-docs.ps1. Use the interactive git add so you don't change anything but the 4 new static methods.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
Member

StephaneDelcroix commented Nov 28, 2016

👍

@activa

This comment has been minimized.

Show comment
Hide comment
@activa

activa Nov 28, 2016

Contributor

@StephaneDelcroix done

Would be useful if this is mentioned somewhere in the "How to contribute" page

Contributor

activa commented Nov 28, 2016

@StephaneDelcroix done

Would be useful if this is mentioned somewhere in the "How to contribute" page

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Nov 29, 2016

Contributor

@StephaneDelcroix Nothing seems to be happening when I run update-docs.ps1. As suggested above, we should see this in a How-To.

Contributor

adrianknight89 commented Nov 29, 2016

@StephaneDelcroix Nothing seems to be happening when I run update-docs.ps1. As suggested above, we should see this in a How-To.

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Nov 29, 2016

Member

@adrianknight89 Do you get any output messages at all with update-docs.ps1? Any files changed under the docs folder? Any error messages?

Member

hartez commented Nov 29, 2016

@adrianknight89 Do you get any output messages at all with update-docs.ps1? Any files changed under the docs folder? Any error messages?

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Nov 30, 2016

Contributor

@hartez I tried it again, and it worked this time. My execution policy wasn't set up properly. I also tried update-docs-windows.bat which seems to be working as well.

Contributor

adrianknight89 commented Nov 30, 2016

@hartez I tried it again, and it worked this time. My execution policy wasn't set up properly. I also tried update-docs-windows.bat which seems to be working as well.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Jan 3, 2017

Member

please rebase

Member

rmarinho commented Jan 3, 2017

please rebase

@rmarinho rmarinho requested a review from jassmith Jan 5, 2017

activa added some commits Oct 6, 2016

Update RelativeLayout to make it respond to constraint changes
Constraints of a RelativeLayout are bindable properties but the layout
does not update when the constraints are updated.

This change will invalidate the layout whenever  XConstraint,
YConstraint, WidthConstraint or HeightConstraint is changed (either in
code or through a change in the bound property)
Adding attached property accessors for layout properties
Since the constraint attached properties can now be updated at runtime,
setters are required for those properties. Also, when adding a child
view at runtime using the Add() method with x/y/w/h constraints,
generating the bounds constraints is deferred to the layout phase.
Rename unit test method
Rename LayoutChangesAtRuntim() to LayoutIsUpdatedWhenConstraintsChange()
Update documentation of RelativeLayout
Added SetXConstraint(), SetYConstraint(), SetWidthConstraint() and
SetHeightConstraint()
@activa

This comment has been minimized.

Show comment
Hide comment
@activa

activa Feb 2, 2017

Contributor

@rmarinho rebase done

Contributor

activa commented Feb 2, 2017

@rmarinho rebase done

@jassmith jassmith merged commit 01a56f9 into xamarin:master Feb 2, 2017

1 check was pending

Windows-Debug-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug
Details

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

@samhouts samhouts added this to the 2.3.5 milestone Jun 27, 2018

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