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

PasswordVisible Property of PasswordTextBox & its zh_CN translation #943

Merged
merged 9 commits into from Dec 20, 2017

Conversation

Projects
None yet
3 participants
@ColinTree
Copy link
Contributor

ColinTree commented Sep 28, 2017

No description provided.

@ColinTree

This comment has been minimized.

Copy link
Contributor

ColinTree commented Sep 28, 2017

this allowed PasswordTextBox to be switched to Visible mode(seems like TextBox) and also can be switched back easily

@ColinTree ColinTree changed the title added a PasswordVisible Property to PasswordTextBox & zh_CN translate PasswordVisible Property of PasswordTextBox & its zh_CN translation Nov 17, 2017

@ewpatton
Copy link
Member

ewpatton left a comment

@ColinTree Thank you for submitting this. There are a few minor changes that you will need to address before we can merge this.

@@ -839,7 +839,9 @@ private YaVersion() {
// - The Alignment property was renamed to TextAlignment.
// For PASSWORDTEXTBOX_COMPONENT_VERSION 3:
// - Added RequestFocus Function (via TextBoxBase)
public static final int PASSWORDTEXTBOX_COMPONENT_VERSION = 3;
// For PASSWORDTEXTBOX_COMPONENT_VERSION 4:
// - Added PasswordVisible property (via TextBoxBase)

This comment has been minimized.

@ewpatton

ewpatton Dec 12, 2017

Member

According to this comment you added PasswordVisible via TextBoxBase, but that is not true according to the code. Would you mind changing the comment to reflect this?

@@ -7,8 +7,11 @@
package com.google.appinventor.components.runtime;

import com.google.appinventor.components.annotations.DesignerComponent;
import com.google.appinventor.components.annotations.DesignerProperty;

This comment has been minimized.

@ewpatton

ewpatton Dec 12, 2017

Member

You add the DesignerProperty import here, but do not use it on the PasswordVisible property defined below. Either remove the import or add the annotation on the property. I think it would be reasonable to make the property visible in the designer.

@@ -42,7 +45,8 @@ public PasswordTextBox(ComponentContainer container) {
super(container, new EditText(container.$context()));

// Disable auto-suggestion.
view.setRawInputType(InputType.TYPE_TEXT_VARIATION_PASSWORD);
//view.setRawInputType(InputType.TYPE_TEXT_VARIATION_PASSWORD);

This comment has been minimized.

@ewpatton

ewpatton Dec 12, 2017

Member

Remove commented out code.

import com.google.appinventor.components.common.ComponentCategory;
import com.google.appinventor.components.common.PropertyTypeConstants;

This comment has been minimized.

@ewpatton

ewpatton Dec 12, 2017

Member

Ditto PropertyTypeConstants import.

@@ -54,4 +58,20 @@ public PasswordTextBox(ComponentContainer container) {
view.setImeOptions(EditorInfo.IME_ACTION_DONE);

}

private boolean passwordVisible;

This comment has been minimized.

@ewpatton

ewpatton Dec 12, 2017

Member

Java convention is to define fields at the top of the class definition. You should move this definition of passwordVisible to before the constructor.

@ewpatton

This comment has been minimized.

Copy link
Member

ewpatton commented Dec 14, 2017

@ColinTree If you can address the requested changes, we should be able to put this into the upcoming components release.

@ewpatton ewpatton added this to the Components Release nb164 milestone Dec 14, 2017

@moliata

This comment has been minimized.

Copy link
Contributor

moliata commented Dec 14, 2017

I would like to keep password text box component as a completely different component.

@ColinTree

This comment has been minimized.

Copy link
Contributor

ColinTree commented Dec 15, 2017

@moliata it is however not another individual textbox, it is changed by a property.

@moliata

This comment has been minimized.

Copy link
Contributor

moliata commented Dec 15, 2017

Yea that's what I meant, I would like to keep PasswordTextBox component

ColinTree added some commits Dec 15, 2017

@ColinTree

This comment has been minimized.

Copy link
Contributor

ColinTree commented Dec 15, 2017

@ewpatton Done.

@ewpatton
Copy link
Member

ewpatton left a comment

When I tested this on an emulator running Android 4.2, the first time I set the property to true the font changed to a different password symbol and it still behaved as a PasswordTextBox would. After setting the property to false and then true, it worked as one would expect. I think this may be related to the lack of the TYPE_CLASS_TEXT flag. Please test this on some older devices to make sure that it behaves as expected.

@@ -54,4 +58,19 @@ public PasswordTextBox(ComponentContainer container) {
view.setImeOptions(EditorInfo.IME_ACTION_DONE);

}

@SimpleProperty

This comment has been minimized.

@ewpatton

ewpatton Dec 15, 2017

Member

@ColinTree Please set a description here so that there is a tooltip on the block.

public void PasswordVisible(boolean visible){
passwordVisible=visible;
if(visible){
view.setInputType(InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD);

This comment has been minimized.

@ewpatton

ewpatton Dec 15, 2017

Member

I think you need to include TYPE_CLASS_TEXT here as well.

ColinTree added some commits Dec 18, 2017

@ColinTree

This comment has been minimized.

Copy link
Contributor

ColinTree commented Dec 19, 2017

@ewpatton I tested on a series of devices including the old official appinvenor emulator, it should works now with an extra TYPE_CLASS_TEXT

@ewpatton

This comment has been minimized.

Copy link
Member

ewpatton commented Dec 19, 2017

@ColinTree Thanks. I'll take a look.

@ewpatton

This comment has been minimized.

Copy link
Member

ewpatton commented Dec 19, 2017

I'm still having trouble with the most recent change and the emulator. Try your build with this aia. If I click on the "One true" it will not change to visible, but if I hit "One false" then "One true" it will. I did find that if I moved the PasswordVisible(false); statement in the constructor to the end, it worked as one would expect. It might be an interaction with the setTransformationMethod call. I would recommend moving the call to PasswordVisible to the end of the constructor.

@ColinTree

This comment has been minimized.

Copy link
Contributor

ColinTree commented Dec 20, 2017

@ewpatton Looks good to me now, at least I tested on more devices and no unexpected situations (with your aia).

@ewpatton

This comment has been minimized.

Copy link
Member

ewpatton commented Dec 20, 2017

LGTM. Thanks @ColinTree.

@ColinTree ColinTree deleted the ColinTree:PasswordTextBox-PasswordVisible branch Dec 26, 2017

@ColinTree ColinTree restored the ColinTree:PasswordTextBox-PasswordVisible branch Dec 26, 2017

@ColinTree ColinTree deleted the ColinTree:PasswordTextBox-PasswordVisible branch Dec 26, 2017

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