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

Implemented MaxLength property on Entry and Editor #1880

Merged

Conversation

@jfversluis
Copy link
Member

commented Feb 14, 2018

Description of Change

Implementation of MaxLength on both the Entry as well as the Editor (by implementing it on the InputView) for all platforms supported today. Fixes #1663.

No unit tests have been added since I would just be testing the getter/setter of the property.

Bugs Fixed

  • N/A

API Changes

Added:

  • static BindableProperty InputView.MaxLengthProperty;
  • int InputView.MaxLength { get; set; } //Bindable Property, default value int.MaxValue

Behavioral Changes

There shouldn't be any behavioral changes by just upgrading. But they now have the ability to limit the input length of the Editor and Entry controls.

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
jfversluis added 16 commits Feb 3, 2018
Started implementation of #1663
Added MaxLength property on InputView and implemented iOS and Android
Entry and Android Editor
Improved Android MaxLength
First check to see if there is a LengthFilter in there already and
remove that first
Fixed whitespaces -> tabs
Except GTK, seems everything is spaces there, so kept it for consistency

@samhouts samhouts added this to Ready in v3.1.0 via automation Feb 14, 2018

@samhouts samhouts requested review from jassmith and PureWeen Feb 14, 2018

@samhouts samhouts added this to In Progress in Enhancements Feb 14, 2018

@samhouts samhouts moved this from Ready to In Review in v3.1.0 Feb 14, 2018

@@ -5,6 +5,14 @@ public class InputView : View
public static readonly BindableProperty KeyboardProperty = BindableProperty.Create("Keyboard", typeof(Keyboard), typeof(InputView), Keyboard.Default,
coerceValue: (o, v) => (Keyboard)v ?? Keyboard.Default);

public static readonly BindableProperty MaxLengthProperty = BindableProperty.Create("MaxLength", typeof(int), typeof(int), int.MaxValue);

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Feb 14, 2018

Member

I'd use -1 as default value

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Feb 14, 2018

Member

use nameof(MaxLength)

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Feb 14, 2018

Member

we usually use tabs for indentation

This comment has been minimized.

Copy link
@jfversluis

jfversluis Feb 15, 2018

Author Member

In the #1663 we sort of decided to go with int.MaxValue. What would be the benefit of using -1?
Fixed nameof and tabs.

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Feb 15, 2018

Member

I'll discuss the defaultValue with @hartez. both are fine, both make sense. But having Int32.MaxValue prevents us from having a really long text in, while -1 does not.

@@ -60,6 +62,7 @@ protected override void OnElementChanged(ElementChangedEventArgs<Editor> e)
Control.Changed += HandleChanged;
Control.Started += OnStarted;
Control.Ended += OnEnded;
Control.ShouldChangeText += ShouldChangeText;

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Feb 14, 2018

Member

it looks like you do not do anything on property change

This comment has been minimized.

Copy link
@jfversluis

jfversluis Feb 15, 2018

Author Member

Not sure what you mean here. The event is hooked up? If you mean that I don't act on the MaxLength being changed you're right. Wasn't sure what to do with that. The logical thing to do would be to cut off the current value if its length exceeds the new MaxLength value?

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Feb 15, 2018

Member

The logical thing to do would be to cut off the current value if its length exceeds the new MaxLength value

that's what I'd do

This comment has been minimized.

Copy link
@jfversluis

jfversluis Feb 15, 2018

Author Member

Definitely makes sense, implemented it

@rmarinho

This comment has been minimized.

Copy link
Member

commented Feb 15, 2018

@jfversluis the build is failing in the Tizen platform

Xamarin.Forms.Platform.Tizen\Renderers\EditorRenderer.cs (31, 31)
Xamarin.Forms.Platform.Tizen\Renderers\EditorRenderer.cs(31,31): Error CS1503: Argument 1: cannot convert from 'method group' to 'Func<Entry, string, string>'
@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2018

@rmarinho should be resolved now

@@ -35,6 +35,9 @@ protected override void Build (StackLayout stackLayout)
var textColorDisabledContainer = new ViewContainer<Editor> (Test.Editor.TextColor,
new Editor { Text = "I should have the default disabled text color", TextColor = Color.Red, IsEnabled = false });

var maxLengthContainer = new ViewContainer<Editor>(Test.Editor.MaxLength,
new Editor { MaxLength = 3 });

This comment has been minimized.

Copy link
@jassmith

jassmith Feb 15, 2018

Contributor

Funky whitespace for fun and profit :)


Control?.SetFilters(currentFilters.ToArray());

if (Control?.Text.Length > Element.MaxLength)

This comment has been minimized.

Copy link
@jassmith

jassmith Feb 15, 2018

Contributor

You fetch Control.Text 3 times here. This causes 3 GetValue calls to be sent down to BindableObject. In this case it should be trivial to use a temporary variable to avoid that :)


Control?.SetFilters(currentFilters.ToArray());

if (Control?.Text.Length > Element.MaxLength)

This comment has been minimized.

Copy link
@jassmith

jassmith Feb 15, 2018

Contributor

GetValue calls here as well


void UpdateMaxLength()
{
if (Control?.StringValue?.Length > Element?.MaxLength)

This comment has been minimized.

Copy link
@jassmith

jassmith Feb 15, 2018

Contributor

Multiple calls to Control.StringValue again :)


void UpdateMaxLength()
{
if (Control?.StringValue?.Length > Element?.MaxLength)

This comment has been minimized.

Copy link
@jassmith

jassmith Feb 15, 2018

Contributor

Here too

{
Control.MaxLength = Element.MaxLength;

if (Control.Text.Length > Element.MaxLength)

This comment has been minimized.

Copy link
@jassmith

jassmith Feb 15, 2018

Contributor

And here

@@ -105,6 +108,14 @@ void UpdateTextColor()
Control.UpdateDependencyColor(System.Windows.Controls.Control.ForegroundProperty, Element.TextColor);
}

void UpdateMaxLength()
{
Control.MaxLength = Element.MaxLength;

This comment has been minimized.

Copy link
@jassmith

jassmith Feb 15, 2018

Contributor

Not going to call anymore of these out, but there are quite a few more :)

This comment has been minimized.

Copy link
@jfversluis

jfversluis Feb 15, 2018

Author Member

I think you got pretty much all of them? :P But thanks! Learning a lot here :)

This comment has been minimized.

Copy link
@jassmith

jassmith Feb 16, 2018

Contributor

Yeah its fun, just doing these little optimizations on our first pass once we realized this was a problem gave Forms a fairly sizeable performance boost. Now its about making sure we dont end up needing to do that again :)

@jassmith

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

Scheduling builds now

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2018

That’s curious. Does anyone have details on the failing build?

@rmarinho

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

@jfversluis is failing on docs, you need to generate the docs needed and commit also.

jfversluis added 2 commits Feb 16, 2018
@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2018

@rmarinho done!

@@ -451,9 +451,6 @@
<AssemblyVersion>2.0.0.0</AssemblyVersion>
</AssemblyInfo>
<Attributes>
<Attribute>

This comment has been minimized.

Copy link
@rmarinho

rmarinho Feb 16, 2018

Member

no need to commit this, build in Debug, then generate docs

This comment has been minimized.

Copy link
@jfversluis

jfversluis Feb 16, 2018

Author Member

@rmarinho I'm not sure what went wrong..? Rebuild the whole solution in Debug and then commit the results? Or what am I supposed to do here?

@@ -512,9 +512,6 @@
<AssemblyVersion>2.0.0.0</AssemblyVersion>
</AssemblyInfo>
<Attributes>
<Attribute>

This comment has been minimized.

Copy link
@rmarinho

rmarinho Feb 16, 2018

Member

no need to commit this

@@ -3,13 +3,13 @@
<Assembly Name="Xamarin.Forms.Core" Version="2.0.0.0">
<Attributes>
<Attribute>

This comment has been minimized.

Copy link
@rmarinho

rmarinho Feb 16, 2018

Member

no need to commit this

@@ -3,13 +3,13 @@
<Assembly Name="Xamarin.Forms.Maps" Version="2.0.0.0">
<Attributes>
<Attribute>
<AttributeName>System.Diagnostics.Debuggable(System.Diagnostics.DebuggableAttribute+DebuggingModes.Default | System.Diagnostics.DebuggableAttribute+DebuggingModes.DisableOptimizations | System.Diagnostics.DebuggableAttribute+DebuggingModes.EnableEditAndContinue | System.Diagnostics.DebuggableAttribute+DebuggingModes.IgnoreSymbolStoreSequencePoints)</AttributeName>

This comment has been minimized.

Copy link
@rmarinho

rmarinho Feb 16, 2018

Member

No need to commit this or the others similar

jfversluis added 2 commits Feb 16, 2018
Revert "Updated the docs"
This reverts commit 416e287.
@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2018

@rmarinho better like this? :)

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

Is there anything I need to do for the failing UI tests?

@rmarinho

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

@jfversluis no it's good

@jassmith jassmith merged commit 1f770f6 into xamarin:master Feb 19, 2018

12 of 13 checks passed

VSTS: iOS11 Validation UITests Finished
Details
VSTS: Android API19 Validation Fast Renderers UITests Finished
Details
VSTS: Android API19 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API23 Validation Fast Renderers UITests Finished
Details
VSTS: Android API23 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API25 Validation Fast Renderers UITests Finished
Details
VSTS: Android API25 Validation Legacy Renderers UITests Finished
Details
VSTS: Xamarin Forms (PR Builds) Succeeded PR process
Details
VSTS: Xamarin Forms OSX PR-1880 - (1393226) succeeded
Details
VSTS: Xamarin Forms Windows VS2017 PR-1880 - (1393200) succeeded
Details
VSTS: iOS10 Validation UITests Finished
Details
VSTS: iOS9 Validation UITests Finished
Details
license/cla All CLA requirements met.
Details

v3.1.0 automation moved this from In Review to Done Feb 19, 2018

Enhancements automation moved this from In Progress to Done Feb 19, 2018

@jfversluis jfversluis deleted the XamarinFormsCommunity:feature/1663-entry-maxlength branch Feb 19, 2018

jsuarezruiz added a commit to XamarinFormsCommunity/Xamarin.Forms that referenced this pull request Feb 28, 2018
Merge branch 'master' of https://github.com/xamarin/Xamarin.Forms
* 'master' of https://github.com/xamarin/Xamarin.Forms: (23 commits)
  [C] use direct cast
  [Core, iOS, Android, UWP, WPF] Hide scroll view scroll bars (xamarin#1910)
  Allow users to specify resolution method for handlers, effects, and services (xamarin#1870) fixes xamarin#1739
  [Build] Update submodule
  Capitalization keyboard flag additions for Entry/Editor (xamarin#1683) (xamarin#1833)
  [Build] Don't specify .net sdk version
  Simplify event raising invocation pattern (xamarin#1971)
  [iOS Maps] Pin rendering customization (xamarin#1065)
  set csharp_space_after_keywords_in_control_flow_statements to true to fit our design guide lines (xamarin#1964)
  [iOS] Add shadow effect (xamarin#1896)
  Added support for ListView full width separators on iOS (xamarin#1854) fixes xamarin#1665
  Support CascadeInputTransparent to Tizen (xamarin#1916)
  [Android]?Remove UserVisibleHint (xamarin#1550) fixes xamarin#1438
  [iOS] ViewDidLayoutSubviews after removing page (xamarin#1532) fixes xamarin#1426
  Use relative URL to support recursive checkout in VSTS (xamarin#1926)
  [UITest] Fix test for UITest package update (xamarin#1923)
  Implemented MaxLength property on Entry and Editor (xamarin#1880)
  [Build]Fix master and build (xamarin#1920)
  [Build] Fix windows cert
  Fix to absolute URL
  ...

# Conflicts:
#	Xamarin.Forms.Controls/CoreGalleryPages/EditorCoreGalleryPage.cs
#	Xamarin.Forms.Controls/CoreGalleryPages/EntryCoreGalleryPage.cs
#	Xamarin.Forms.Core/InputView.cs
#	Xamarin.Forms.CustomAttributes/TestAttributes.cs
#	Xamarin.Forms.Platform.Android/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.Android/Renderers/EntryRenderer.cs
#	Xamarin.Forms.Platform.MacOS/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.MacOS/Renderers/EntryRenderer.cs
#	Xamarin.Forms.Platform.Tizen/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.Tizen/Renderers/EntryRenderer.cs
#	Xamarin.Forms.Platform.WPF/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.WPF/Renderers/EntryRenderer.cs
#	Xamarin.Forms.Platform.iOS/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.iOS/Renderers/EntryRenderer.cs

@samhouts samhouts added the API-change label Apr 3, 2018

@samhouts samhouts added this to the 3.0.0 milestone May 5, 2018

@samhouts samhouts removed this from Done in v3.1.0 May 7, 2018

@samhouts samhouts removed this from Done in Enhancements Jun 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.