[Enhancement] Entry: Read-only Entry and Editor #1972

almirvuk · 2018-02-24T15:03:00Z

Description of Change

This PR adds the IsReadOnly bindable property to the InputView i.e.Editor and Entry controls. Default value is false so this change and usage of this property is backwards compatible.

Bugs Fixed

API Changes

Added:

BindableProperty InputView.IsReadOnlyProperty;

Changed:

None

Behavioral Changes

None

Supported Platforms

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

Remark

There was a section "Implications for CSS", can I get more info about that, is there any way for me to implement that also.

adrianknight89 · 2018-02-26T02:41:50Z

Xamarin.Forms.Platform.Tizen/Renderers/EntryRenderer.cs

@@ -19,6 +19,7 @@ public EntryRenderer()
 			RegisterPropertyHandler(Entry.PlaceholderProperty, UpdatePlaceholder);
 			RegisterPropertyHandler(Entry.PlaceholderColorProperty, UpdatePlaceholderColor);
 			RegisterPropertyHandler(InputView.MaxLengthProperty, UpdateMaxLength);
+			RegisterPropertyHandler(Entry.IsReadOnlyProperty, UpdateIsReadOnly);


I think the whitespace preceeding the if was lost here.

You are right, I will fix that also :)

adrianknight89 · 2018-02-26T02:42:56Z

Xamarin.Forms.Platform.UAP/EditorRenderer.cs

@@ -106,6 +107,10 @@ protected override void OnElementPropertyChanged(object sender, PropertyChangedE
 				UpdateMaxLength();
 			else if (e.PropertyName == Specifics.DetectReadingOrderFromContentProperty.PropertyName)
 				UpdateDetectReadingOrderFromContent();
+			else if (e.PropertyName == InputView.IsReadOnlyProperty.PropertyName)
+			{


Can skip adding curly braces if there is a single statement nested under a predicate.

I will fix this :)

this is fine

adrianknight89 · 2018-02-26T02:44:55Z

Xamarin.Forms.Platform.Tizen/Renderers/EntryRenderer.cs

@@ -146,5 +147,10 @@ string MaxLengthFilter(ElmSharp.Entry entry, string s)

 			return null;
 		}
+
+		void UpdateIsReadOnly()


Can move this above MaxLengthFilter

this is fine.

adrianknight89 · 2018-02-26T03:05:50Z

Xamarin.Forms.Platform.iOS/Renderers/EditorRenderer.cs

+
+		void UpdateIsReadOnly()
+		{
+			Control.UserInteractionEnabled = !Element.IsReadOnly;


Just an FYI that UpdateEditable is also changing UserInteractionEnabled. If I set IsEnabled to false, then the control becomes read-only even though IsReadOnly is false.

Also, please look at other renderers to see if they are doing similar work. We should make sure IsEnabled and IsReadOnly play nice together.

Nice point, thanks! :) Will fix that

"If I set IsEnabled to false, then the control becomes read-only even though IsReadOnly is false.".

Yes, the main task of this Enhancement is to make control ReadOnly without changing the look of it, so if you set IsEnabled to false it will be "read-only" and also it will have a look like it is disabled. But if you are using only IsReadOnly = true, it will become ReadOnly and the look will not change.

So, maybe the best approach is to set IsReadOnly to true if IsEnabled == false ? :)

Rename UpdateIsReadOnly() to UpdateUserInteraction()

In UpdateEditable, change the assignment of UserInteractionEnabled to point at this function instead.

Then in this function, do
Control.UserInteractionEnabled = !Element.IsReadonly && Element.IsEnabled

I think that should work, but you get the gist. You need to account for both properties now.

Yap @adamped suggestion is the correct one, we should have 1 method to handle this, the others call this.

Ok, and my personal deadline for this is like end of this weekend, we will see :)

samhouts

Need to address the IsEnabled/IsReadOnly possible conflicts raised by @adrianknight89.
Failing unit tests.

Thanks!

rmarinho · 2018-03-21T19:36:26Z

Hey @almirvuk do you think you can address the comments on the PR and rebase?!

Thanks

almirvuk · 2018-03-21T20:47:49Z

@rmarinho can you please take a look at this comment/question #1972 (comment)

What do you think about this?

Also I am very close to finishing this, I think I need to finish Unit Tests, I have made some changes but I am stuck and I need somebody opinion about IsEnabled/IsReadOnly behaviour... and resolving these conflicts with rebase will be easy.

dnfclas · 2018-04-09T21:04:53Z

All CLA requirements met.

almirvuk · 2018-04-09T21:20:30Z

I made some changes for iOS EditorRenderer that was problematic in the last pull request. I still need to do rebase and see what is the problem with unit tests (if they are mandatory?)... Can you please take a look. Thanks! 🙂

rmarinho · 2018-04-30T17:01:59Z

Hey @almirvuk can you rebase , and yes unit tests are mandatory

almirvuk · 2018-05-05T15:48:21Z

@rmarinho I will do this by the end of the tomorrow :) Thanks

almirvuk · 2018-05-07T06:28:55Z

Unit tests and rebase, done ✔️

almirvuk · 2018-05-07T12:36:29Z

Obviously...there is a issue with my last push, I will fix this and do another one. Sorry 🙂

Xamarin.Forms.Platform.Android/Renderers/EntryRenderer.cs

almirvuk · 2019-01-08T21:11:46Z

@PureWeen I invited you as collaborator on my fork repo, do I need to do anything else? :)

PureWeen · 2019-01-08T22:51:15Z

@almirvuk that looks like it worked! thank you

PureWeen · 2019-01-08T23:50:12Z

Xamarin.Forms.Platform.MacOS/Renderers/EditorRenderer.cs

+		{
+			Control.Editable = !Element.IsReadOnly;
+			if (Element.IsReadOnly && Control.Window?.FirstResponder == Control.CurrentEditor)
+				Control.Window?.MakeFirstResponder(null);


@rmarinho I added this code on MacOS to fix #4834 so please re-review macos

PureWeen · 2019-01-08T23:50:20Z

Xamarin.Forms.Platform.MacOS/Renderers/EntryRenderer.cs

+		{
+			Control.Editable = !Element.IsReadOnly;
+			if (Element.IsReadOnly && Control.Window?.FirstResponder == Control.CurrentEditor)
+				Control.Window?.MakeFirstResponder(null);


@rmarinho I added this code on MacOS to fix #4834 so please re-review macos

PureWeen · 2019-01-09T16:31:27Z

Android UI test failures are on 39821 (not caused by this change, fixed by #4866)
IOS10 passed but github is just reporting it wrong

Once MacOS changes are approved we can merge

almirvuk · 2019-01-09T19:22:34Z

Nice, thank you @PureWeen for the help with this PR 🤗

PureWeen · 2019-01-10T00:08:36Z

@almirvuk Thank you for the good work! Hopefully we can get it merged before the PRs anniversary next month

- fixes #1678 - fixes #4834 * InputView BindableProperty IsReadOnly * Android Editor and Entry setup * MacOS Editor and Entry setup * Tizen Editor and Entry setup * UAP Editor and Entry setup * WPF Editor and Entry setup * iOS Editor and Entry setup * Issue1678 - TestContentPage added * Tests added * Tizen Editor and Entry fix * UI Test fix * Android fix for Entry and Editor. * Unit Tests fix. * Android Renderer first try. * MacOS Entry renderer small fix. * UpdateEditable fix. * Android Entry and Editor reduced number of calls to BP. * fix TestAttributes conflicts * Remove keyboard * [macOS] relinquish first responder * [Android] remove call to UpdateCursorSelection * [UWP] fix tabs

- fixes xamarin#1678 - fixes xamarin#4834 * InputView BindableProperty IsReadOnly * Android Editor and Entry setup * MacOS Editor and Entry setup * Tizen Editor and Entry setup * UAP Editor and Entry setup * WPF Editor and Entry setup * iOS Editor and Entry setup * Issue1678 - TestContentPage added * Tests added * Tizen Editor and Entry fix * UI Test fix * Android fix for Entry and Editor. * Unit Tests fix. * Android Renderer first try. * MacOS Entry renderer small fix. * UpdateEditable fix. * Android Entry and Editor reduced number of calls to BP. * fix TestAttributes conflicts * Remove keyboard * [macOS] relinquish first responder * [Android] remove call to UpdateCursorSelection * [UWP] fix tabs

minaairsupport · 2019-02-11T15:55:18Z

Nice work guys but I have small comment regarding to test case you provide to prove this feature is working , I think it will be better if you not just check the flag but implment the feature itseld
like

Entry entry = new Entry();
entry.SetValue(InputView.IsReadOnlyProperty, isReadOnly);
entry.Text = "Any thing"; // I make not sure if this the right poisition may be it should before set flag 
Assert.AreEqual(isReadOnly, entry.IsReadOnly);
//other assert

test case provide

[TestCase(true)]
		public void IsReadOnlyTest(bool isReadOnly)
		{
			Entry entry = new Entry();
			entry.SetValue(InputView.IsReadOnlyProperty, isReadOnly);
			Assert.AreEqual(isReadOnly, entry.IsReadOnly);
		}

almirvuk changed the title ~~[Enhancement] Entry: Read-only entry #1678~~ [Enhancement #1678] Entry: Read-only Entry and Editor Feb 24, 2018

almirvuk changed the title ~~[Enhancement #1678] Entry: Read-only Entry and Editor~~ [Enhancement] Entry: Read-only Entry (and Editor) Feb 24, 2018

samhouts added t/enhancement ➕ F100 community-sprint labels Feb 24, 2018

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

samhouts changed the title ~~[Enhancement] Entry: Read-only Entry (and Editor)~~ [Enhancement] Entry: Read-only Entry and Editor Feb 24, 2018

adrianknight89 reviewed Feb 26, 2018

View reviewed changes

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

samhouts previously requested changes Mar 2, 2018

View reviewed changes

samhouts added this to In Progress in Enhancements Mar 22, 2018

samhouts added this to In Review in v3.6.0 May 7, 2018

samhouts removed this from In Review in v3.1.0 May 7, 2018

davidortinau added this to the 3.1.0 milestone May 17, 2018

samhouts removed this from In Review in v3.6.0 May 18, 2018

samhouts removed this from the 3.1.0 milestone May 18, 2018

samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label May 18, 2018

samhouts added this to In Review in v3.1.0 May 18, 2018

samhouts added this to In Review in v3.6.0 May 18, 2018

PureWeen suggested changes Jan 8, 2019

View reviewed changes

Xamarin.Forms.Platform.Android/Renderers/EntryRenderer.cs Outdated Show resolved Hide resolved

Merge branch 'master' into pr/1972

1b3509e

PureWeen added 2 commits January 8, 2019 16:40

[macOS] relinquish first responder

571764e

[Android] remove call to UpdateCursorSelection

4b21b84

PureWeen reviewed Jan 8, 2019

View reviewed changes

[UWP] fix tabs

e314ef7

PureWeen requested a review from rmarinho January 8, 2019 23:51

PureWeen approved these changes Jan 9, 2019

View reviewed changes

rmarinho approved these changes Jan 10, 2019

View reviewed changes

PureWeen merged commit 5001460 into xamarin:master Jan 10, 2019

v3.6.0 automation moved this from In Progress to Done Jan 10, 2019

PureWeen deleted the feature/1678-read-only-entry branch January 10, 2019 16:21

samhouts added this to Done in v4.0.0 Feb 2, 2019

samhouts removed this from Done in v3.6.0 Feb 2, 2019

samhouts modified the milestones: 4.0.0, 3.6.0 Feb 6, 2019

samhouts removed this from Done in v4.0.0 Feb 7, 2019

samhouts added this to Done in v3.6.0 Feb 7, 2019

samhouts added inactive Issue is older than 6 months and needs to be retested t/bug 🐛 labels Jun 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Entry: Read-only Entry and Editor #1972

[Enhancement] Entry: Read-only Entry and Editor #1972

almirvuk commented Feb 24, 2018 •

edited by PureWeen

Loading

adrianknight89 Feb 26, 2018

almirvuk Feb 26, 2018

adrianknight89 Feb 26, 2018

almirvuk Feb 26, 2018

StephaneDelcroix Feb 27, 2018

adrianknight89 Feb 26, 2018

almirvuk Feb 26, 2018

StephaneDelcroix Feb 27, 2018

adrianknight89 Feb 26, 2018

adrianknight89 Feb 26, 2018

almirvuk Feb 26, 2018

almirvuk Mar 11, 2018

adamped Mar 22, 2018 •

edited

Loading

almirvuk Mar 22, 2018

rmarinho Mar 22, 2018

almirvuk Mar 22, 2018 •

edited

Loading

samhouts left a comment •

edited

Loading

rmarinho commented Mar 21, 2018

almirvuk commented Mar 21, 2018 •

edited

Loading

dnfclas commented Apr 9, 2018 •

edited

Loading

almirvuk commented Apr 9, 2018 •

edited

Loading

rmarinho commented Apr 30, 2018

almirvuk commented May 5, 2018

almirvuk commented May 7, 2018

almirvuk commented May 7, 2018

almirvuk commented Jan 8, 2019

PureWeen commented Jan 8, 2019

PureWeen Jan 8, 2019

PureWeen Jan 8, 2019

rmarinho Jan 10, 2019

PureWeen commented Jan 9, 2019

almirvuk commented Jan 9, 2019

PureWeen commented Jan 10, 2019

minaairsupport commented Feb 11, 2019

[Enhancement] Entry: Read-only Entry and Editor #1972

[Enhancement] Entry: Read-only Entry and Editor #1972

Conversation

almirvuk commented Feb 24, 2018 • edited by PureWeen Loading

Description of Change

Bugs Fixed

API Changes

Behavioral Changes

Supported Platforms

PR Checklist

Remark

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamped Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almirvuk Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

samhouts left a comment • edited Loading

Choose a reason for hiding this comment

rmarinho commented Mar 21, 2018

almirvuk commented Mar 21, 2018 • edited Loading

dnfclas commented Apr 9, 2018 • edited Loading

almirvuk commented Apr 9, 2018 • edited Loading

rmarinho commented Apr 30, 2018

almirvuk commented May 5, 2018

almirvuk commented May 7, 2018

almirvuk commented May 7, 2018

almirvuk commented Jan 8, 2019

PureWeen commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PureWeen commented Jan 9, 2019

almirvuk commented Jan 9, 2019

PureWeen commented Jan 10, 2019

minaairsupport commented Feb 11, 2019

almirvuk commented Feb 24, 2018 •

edited by PureWeen

Loading

adamped Mar 22, 2018 •

edited

Loading

almirvuk Mar 22, 2018 •

edited

Loading

samhouts left a comment •

edited

Loading

almirvuk commented Mar 21, 2018 •

edited

Loading

dnfclas commented Apr 9, 2018 •

edited

Loading

almirvuk commented Apr 9, 2018 •

edited

Loading