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

Made a resettable UIText widget with a button for clearing text. #2543

Merged
merged 2 commits into from
Oct 15, 2016
Merged

Made a resettable UIText widget with a button for clearing text. #2543

merged 2 commits into from
Oct 15, 2016

Conversation

arpan98
Copy link
Member

@arpan98 arpan98 commented Oct 14, 2016

Contains

Fixes #2507 using the second approach mentioned in the issue, making a new widget ResettableUIText based on the UIText widget. Also added the widget to the migTestScreen, so it can be seen in different states using the console command showSreen migTestScreen.

Outstanding before merging

On clicking the button, the text is cleared but focus is lost from the UIText box. Any suggestions for a workaround?

@GooeyHub
Copy link
Member

Can one of the admins please verify this patch?

@Cervator
Copy link
Member

@GooeyHub: add to whitelist

Thanks for this @arpan98 along with the other two PRs! :-)

Aiming to merge those now since they're easy, maybe @rzats can take a look at this one to provide some feedback

@Cervator Cervator added the Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience label Oct 15, 2016
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

Copy link
Member

@rzats rzats left a comment

Choose a reason for hiding this comment

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

Needs some minor tweaks and a fairly major one, but you're definitely on the right track!

import org.terasology.rendering.nui.widgets.UILabel;
import org.terasology.rendering.nui.widgets.UIList;
import org.terasology.rendering.nui.widgets.UIText;
import org.terasology.rendering.nui.widgets.*;
Copy link
Member

Choose a reason for hiding this comment

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

No star imports, please. (Go to Settings → Editor → Code Style → Java → Imports and set "Class count to use import with '*'" along with "Names count to use static import with '*'" to 999)

import org.terasology.rendering.nui.UIWidget;

public class ResettableUIText extends UIText {
private UIButton clearButton = new UIButton("clearButton", "X");
Copy link
Member

Choose a reason for hiding this comment

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

This is a working approach, but IMO Terasology's default button doesn't look great when used as a small control element.

UIDropdown handles control elements slightly differently - it uses a texture (dropdown.png) the right part of which masquerades as a button. I would use this idea for ResettableUIText as well:

  • Create a modified version of box.png (the texture that UIText uses as a background) similar to dropdown.png. The left 80% of it should be the same as box.png, but the right 20% will contain a reset button. (Different percentage values are also fine - use what works best!)
  • In ResettableUIText.onDraw() separate the canvas region into two parts:
    • The left (80%) will display the current text and place the cursor at a specified position when clicked on (same as the default UIText).
    • The right (20%) will reset the text field when clicked on. (This should also fix the issue mentioned in the PR as the "button" won't make the text field lose focus!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice idea! I'm having some trouble finding where these textures are set to the corresponding UI elements. Any pointers?

Copy link
Member

Choose a reason for hiding this comment

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

default.skin 🙂

You'll need to add a new entry looking something like this:

"ResettableUIText": {
    "background": "engine:yourTexture"
}

super.lastWidth = (int) (canvas.size().x * 0.8f);
if (isEnabled()) {
canvas.addInteractionRegion(super.interactionListener, Rect2i.createFromMinAndMax(0, 0, (int) (canvas.size().x * 0.8f), canvas.size().y));
clearButton.subscribe(new ActivateEventListener() {
Copy link
Member

Choose a reason for hiding this comment

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

This should be lambdified:

clearButton.subscribe(button -> setText(""));

IDEA should prompt you to convert anonymous inner classes to lambdas - do this whenever it does 🙂


@LayoutConfig
private boolean multiline;
protected boolean multiline;

@LayoutConfig
private boolean readOnly;
Copy link
Member

Choose a reason for hiding this comment

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

Consider setting UIText's other private variables and methods (except logger, BLINK_RATE, blinkCounter and cursorTexture as they are unlikely to be changed by a child class) to protected - don't just make half of class extensible! 😛

Copy link
Member Author

@arpan98 arpan98 Oct 15, 2016

Choose a reason for hiding this comment

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

Should I redefine the methods as well? They do have the exact same functionality as in the parent class (which was the entire point of inheriting from UIText) and will lead to a lot of code repetition.

Copy link
Member

Choose a reason for hiding this comment

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

No, not at all - just change the access modifiers to protected! (The reason for this is that a programmer in the future might want to create their own UIText override, and to them it might not be obvious why some fields are marked as protected but others aren't.)

import org.terasology.rendering.nui.SubRegion;
import org.terasology.rendering.nui.UIWidget;

public class ResettableUIText extends UIText {
Copy link
Member

Choose a reason for hiding this comment

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

A short multiline comment explaining what the class does would be nice here.

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@arpan98
Copy link
Member Author

arpan98 commented Oct 15, 2016

@rzats I added a default texture and one for disabled as well as the other changes. Have a look.

@rzats rzats merged commit 5831067 into MovingBlocks:develop Oct 15, 2016
@rzats
Copy link
Member

rzats commented Oct 15, 2016

@arpan98: the new widget works great - thanks! 🙂
Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants