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

[TIMOB-25855] Android: TextArea lines and maxLines support #9927

Merged
merged 12 commits into from Jun 27, 2018

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented Mar 11, 2018

JIRA: https://jira.appcelerator.org/browse/TIMOB-25855

var win = Ti.UI.createWindow({
	backgroundColor: '#fff',
	
});

var sv = Ti.UI.createScrollView({
	layout:"vertical",
	height:Ti.UI.FILL
})

var tf1 = Ti.UI.createTextArea({
	borderColor: "#000",
	borderWidth: 1,
	color: "#000",
	width: 200,
	hintText:"1 to 5",
	hintTextColor:"#999",
	lines: 1,
	maxLines: 5,
	
})

var tf2 = Ti.UI.createTextArea({
	lines: 2,
	maxLines: 2,
	borderColor: "#000",
	borderWidth: 1,
	color: "#000",
	width: 200,
	hintText:"2 to 2 -> setting lines to 4",
	hintTextColor:"#999"
});
tf2.lines = 4;


var tf3 = Ti.UI.createTextArea({
	lines: 2,
	maxLines: 2,
	borderColor: "#000",
	borderWidth: 1,
	color: "#000",
	width: 200,
	hintText:"2 to 2",
	hintTextColor:"#999"
});
tf3.maxLines = 2;

var tf4 = Ti.UI.createTextArea({
	lines: 2,
	maxLines: 2,
	borderColor: "#000",
	borderWidth: 1,
	color: "#000",
	width: 200,
	hintText:"2 to 2 -> setting max to 4",
	hintTextColor:"#999"
});
tf4.maxLines = 4;

var textField1 = Ti.UI.createTextField({
	borderColor: "#000",
	borderWidth: 1,
	color: "#000",
	width: 200,
	hintText:"Textfield",
	hintTextColor:"#999"
});

var lbl1 = Ti.UI.createLabel({
	color:"#000",
	bottom: 10,
	text:"one line"
});
var lbl2 = Ti.UI.createLabel({
	color:"#000",
	bottom: 10,
	text:"two\nline"
});
var lbl3 = Ti.UI.createLabel({
	color:"#000",
	bottom: 10,
	text:"max 3 multi\n1 multi\n2 multi\n3 multi\n4 multi\nmulti\nline",
	maxLines: 3
});
var lbl4 = Ti.UI.createLabel({
	color:"#000",
	bottom: 10,
	text:"line 2 multi\n1 multi\n2 multi\n3 multi\n4 multi\nmulti\nline",
	lines: 2
});
win.add(sv);
sv.add(tf1);
sv.add(tf2);
sv.add(tf3);
sv.add(tf4);
sv.add(textField1);
sv.add(lbl1);
sv.add(lbl2);
sv.add(lbl3);
sv.add(lbl4);
win.open();

@Topener Topener changed the title [AC-5653] Android: TextArea lines and maxLines support [TIMOB-25855] Android: TextArea lines and maxLines support Mar 12, 2018
@jquick-axway
Copy link
Contributor

@m1ga, I remember the call order for Google's TextView setLines() and setMaxLines() being important. See our Label code here. You may need to do something similar.
https://github.com/appcelerator/titanium_mobile/blob/master/android/modules/ui/src/java/ti/modules/titanium/ui/widget/TiUILabel.java#L698

From what I remember, calling setLines() will also reset min-lines and max-lines. So, if you call setLines(), you need to call setMaxLines() right afterwards to restore what was last set.

Try testing it with the following...

var tf = Ti.UI.createTextArea({
	lines: 2,
	maxLines: 2,
});
tf.lines = 4;

Also note that the TiUIText Java class you are editing is used by Titanium's TextField and TextArea. So, we need to test that this doesn't break a single-line TextField. Our TiUILabel code I linked above supports both single-line and multiline, so, it's probably a good code base to work off of.

Thanks.

@m1ga
Copy link
Contributor Author

m1ga commented Mar 18, 2018

@jquick-axway thanks for the tips! I'm checking that. TextFields shouldn't be a problem since I only set the new values if !field. But I'll add this to the check.
I current have a problem if I put everything into a vertical layout view. Very strange but I'm on it was using the wrong sdk

@m1ga
Copy link
Contributor Author

m1ga commented Mar 18, 2018

I've added a better example with more cases. It actually looks correct even with setting tf.lines = 4; after creating it with 2/2. It will show 4 lines but still uses the maxLines:2 so you have 2 empty non-usable lines below it.

}

if (d.containsKey(TiC.PROPERTY_MAX_LINES)) {
if (!field) tv.setMaxLines(TiConvert.toInt(d, TiC.PROPERTY_MAX_LINES));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From looking at Google's Textview Java code, they don't expect setMaxLines() to be set to a value <= 0. Google defaults this to Integer.MAX_VALUE by default. We should do the same if the property is set to <= 0. This is to avoid possible layout issues.
https://github.com/aosp-mirror/platform_frameworks_base/blob/master/core/java/android/widget/TextView.java#L658

I believe we might have a similar issue with Android's setLines() method since it set private member variable mMaximum (used as max lines) to the given lines value.
https://github.com/aosp-mirror/platform_frameworks_base/blob/master/core/java/android/widget/TextView.java#L4616

So, if the lines property is <= 0, then we should call setMinLines(0) instead.

I think the best thing to do is to apply lines and maxLines property changes via a method like how I'm doing it in the code below. You don't need the setSingleLine() call, but you do need to call setMaxLines() after calling setLines() since Google resets the maximum afterwards.
https://github.com/appcelerator/titanium_mobile/blob/master/android/modules/ui/src/java/ti/modules/titanium/ui/widget/TiUILabel.java#L706

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice, I'll copy the parts over to the UIText!

@@ -58,6 +60,8 @@ public TextAreaProxy()
{
super();
defaultValues.put(TiC.PROPERTY_VALUE, "");
defaultValues.put(TiC.PROPERTY_LINES, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lines should default to -1.

For backward compatibility, we should respect the TextArea's height property. The lines property is used to set the height of the TextArea by line height instead (definitely a good feature) but the developer should opt-in to it.

@@ -366,6 +366,14 @@ properties:
default: <Titanium.UI.KEYBOARD_DEFAULT>
platforms: [android, iphone, ipad]

- name: lines
summary: Line count of text field input.
description: Sets the initial size of lines that the field will have at start.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make it clear that lines sets the TextArea height by line height... and get rid of the "text field" references since this only applies to text areas.

How about the following...

summary: Number of lines tall the text area height will be, if set.
description: Sets the height of the text are by line height. Set to -1 to respect the view's height property instead.

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR: Pass

Thanks @m1ga :)

@build
Copy link
Contributor

build commented May 10, 2018

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

Messages
📖

🎉 Another contribution from our awesome community member, m1ga! Thanks again for helping us make Titanium SDK better. 👍

📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@sgtcoolguy sgtcoolguy added this to the 7.4.0 milestone Jun 26, 2018
@build
Copy link
Contributor

build commented Jun 26, 2018

Messages
📖

🎉 Another contribution from our awesome community member, m1ga! Thanks again for helping us make Titanium SDK better. 👍

📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn
Copy link
Collaborator

hansemannn commented Jun 26, 2018

@m1ga Can you sign the CLA again? There is a new server for it. Also, a unit-test for each property would be great!

@m1ga
Copy link
Contributor Author

m1ga commented Jun 27, 2018

👍 test added!

@hansemannn
Copy link
Collaborator

Looks great, ready for merge!

@hansemannn hansemannn merged commit cb97b36 into tidev:master Jun 27, 2018
@hansemannn hansemannn modified the milestones: 7.4.0, 7.5.0 Aug 24, 2018
@m1ga m1ga deleted the maxlines branch December 10, 2022 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants