JavaFX custom control - editable label

Question

I have a few custom components, some bigger ones and some smaller ones. I'd like to know if the structure is correct. As in: Is the code to do x in the right places. For starters, I'd just like to show a very small one, take your feedback and adjust my other components accordingly.

It can be found here.

Before you click on the GitHub repo and have a look at it:

It's just very small adjustments made to TextField to make it feel like an editable label. I know that there is a lot more missing to make it a true editable label, but for that I'd have to extend a base class closer to control itself. This question is purely regarding the custom component structure in general + this implementation of EditableLabel fits all that I need it to do, that's why I chose to not go deeper into the rabbit hole with that.

Did I structure the methods and functionality into the correct classes (control, skin, behavior)? What could I have done better there?
Should I do more comments? Could you give me an example (or more)?
Are there any monstrously wrong implementations in there?
Are there smaller mistakes in the implementations?

EditableLabel.java

package com.github.rjwestman.editableLabel;

import javafx.beans.property.*;
import javafx.scene.control.Skin;
import javafx.scene.control.TextField;

import java.net.URL;

/**
 * A TextField, that implements some Label functionality
 *
 * It acts as a Label, by removing the TextField style and making it non-editable.
 * It is also not focus traversable.
 *
 * When clicking on it, it will switch to editable mode
 * Changing focus away from the EditableLabel or pressing ENTER will save the changes made and deactivate editable mode.
 * When pressing ESC it will exit editable mode without saving the changes made.
 *
 * @sa EditableLabelSkin, EditableLabelBehavior
 */
public class EditableLabel extends TextField {

    /************************************************************************
     *                                                                      *
     *                                                                      *
     * \defgroup Constructors                                               *
     * Constructors and helper methods for constructors                     *
     *                                                                      *
     * @{                                                                   *
     ***********************************************************************/

    public EditableLabel() {
        this("");
    }

    public EditableLabel(String text) {
        super(text);
        getStyleClass().setAll("editable-label");
        init();
    }

    private void init() {
        editableClicks = new SimpleIntegerProperty(1);
        baseText = new SimpleStringProperty(getText());
        setFocusTraversable(false);
        setEditable(false);
    }

    /************************************************************************
     * @}                                                                   *
     *                                                                      *
     * \defgroup Properties                                                 *
     * Declaration, getters and setters for the properties of this control  *
     *                                                                      *
     * @{                                                                   *
     ***********************************************************************/

    /**
     * Clicks needed to enter editable-mode
      */
    private IntegerProperty editableClicks;
    public int getEditableClicks() { return editableClicks.get(); }
    public IntegerProperty editableClicksProperty() { return editableClicks; }
    public void setEditableClicks(int editableClicks) { this.editableClicks.set(editableClicks); }

    /**
     * This saves the text that is to be displayed
     *
     * Since we can't override the final set/get methods of the super class, we need to use this
     * to set the text that is to be displayed.
     * Since the displayed text can be a truncated base text we need to save the base in it's own property.
     */
    private StringProperty baseText;
    public String getBaseText() { return baseText.get(); }
    public StringProperty baseTextProperty() { return baseText; }
    public void setBaseText(String baseText) { this.baseText.set(baseText); }

    /************************************************************************
     * @}                                                                   *
     *                                                                      *
     * \defgroup Methods                                                    *
     *                                                                      *
     * @{                                                                   *
     ***********************************************************************/

    @Override
    protected Skin<?> createDefaultSkin() { return new EditableLabelSkin(this); }

    /************************************************************************
     * @}                                                                   *
     *                                                                      *
     * \defgroup StylesheetRelated                                          *
     *                                                                      *
     * @{                                                                   *
     ***********************************************************************/

    @Override
    public String getUserAgentStylesheet() {
        URL pathToCSS = EditableLabel.class.getResource("editablelabel.css");
        if ( pathToCSS != null ) {
            return pathToCSS.toExternalForm();
        } else {
            System.err.println("CSS file for EditableLabel could not be found.");
            return null;
        }
    }

    /** @} */

}

EditableLabelSkin.java

package com.github.rjwestman.editableLabel;

import com.sun.javafx.scene.control.skin.TextFieldSkin;
import javafx.application.Platform;
import javafx.collections.SetChangeListener;
import javafx.css.PseudoClass;
import javafx.scene.text.Text;

/**
 * The Skin Class for EditableLabel
 *
 * @sa EditableLabel, EditableLabelBehavior
 */
public class EditableLabelSkin extends TextFieldSkin {

    private EditableLabel editableLabel;
    private Boolean editableState;

    /************************************************************************
     *                                                                      *
     *                                                                      *
     * \defgroup Constructors                                               *
     * Constructors and helper methods for constructors                     *
     *                                                                      *
     * @{                                                                   *
     ***********************************************************************/

    public EditableLabelSkin(final EditableLabel editableLabel) {
        this(editableLabel, new EditableLabelBehavior(editableLabel));
    }
    public EditableLabelSkin(final EditableLabel editableLabel, final EditableLabelBehavior editableLabelBehavior) {
        super(editableLabel, editableLabelBehavior);
        this.editableLabel = editableLabel;
        init();
    }

    private void init() {
        editableState = false;

        Platform.runLater(this::updateVisibleText);

        // Register listeners and binds
        editableLabel.getPseudoClassStates().addListener( (SetChangeListener<PseudoClass>) e -> {
            if (e.getSet().contains(PseudoClass.getPseudoClass("editable"))) {
                if ( !editableState ) {
                    // editableState change to editable
                    editableState = true;
                    updateVisibleText();
                }
            } else {
                if ( editableState ) {
                    // editableState change to not editable
                    editableState = false;
                    updateVisibleText();
                }
            }
        });
        editableLabel.widthProperty().addListener( observable -> updateVisibleText() );
        editableLabel.baseTextProperty().addListener( observable -> updateVisibleText() );
    }

    /************************************************************************
     * @}                                                                   *
     *                                                                      *
     * \defgroup ControlStateChanges                                        *
     * Handles visual changes on state change that are not or cannot be     *
     * handled via css                                                      *
     *                                                                      *
     * @{                                                                   *
     ***********************************************************************/

    /**
     * Updates the visual text using the baseText
     */
    private void updateVisibleText() {
        String baseText = editableLabel.getBaseText();
        if ( !editableState ) {
            editableLabel.setText(calculateClipString(baseText));
        } else {
            editableLabel.setText(baseText);
            editableLabel.positionCaret(baseText.length());
        }
    }

    /**
     * Truncates text to fit into the EditableLabel
     *
     * @param text The text that needs to be truncated
     * @return The truncated text with an appended "..."
     */
    private String calculateClipString(String text) {
        double labelWidth = editableLabel.getWidth();

        Text layoutText = new Text(text);
        layoutText.setFont(editableLabel.getFont());

        if ( layoutText.getLayoutBounds().getWidth() < labelWidth ) {
            return text;
        } else {
            layoutText.setText(text+"...");
            while ( layoutText.getLayoutBounds().getWidth() > labelWidth ) {
                text = text.substring(0, text.length()-1);
                layoutText.setText(text+"...");
            }
            return text+"...";
        }
    }

    /************************************************************************
     * @}                                                                   *
     *                                                                      *
     * \defgroup SkinLayout                                                 *
     * Lays out the elements of the control                                 *
     * (e.g. calculating and setting sizes and bounds or changing number    *
     * of grid rows and columns)                                            *
     *                                                                      *
     * @{                                                                   *
     ***********************************************************************/

    /** @} */

}

EditableLabelBehavior.java

package com.github.rjwestman.editableLabel;

import com.sun.javafx.scene.control.behavior.TextFieldBehavior;
import javafx.css.PseudoClass;
import javafx.scene.input.KeyEvent;
import javafx.scene.input.MouseEvent;

/**
 * The Behavior Class for EditableLabel
 *
 * @sa EditableLabel, EditableLabelSkin
 */
public class EditableLabelBehavior extends TextFieldBehavior {

    private EditableLabel editableLabel;
    private Boolean focusTraversable;

    /************************************************************************
     *                                                                      *
     * \defgroup Constructors                                               *
     * Constructors and helper methods for constructors                     *
     *                                                                      *
     * @{                                                                   *
     ***********************************************************************/

    public EditableLabelBehavior(final EditableLabel editableLabel) {
        super(editableLabel);
        this.editableLabel = editableLabel;
        init();
    }

    private void init() {
        focusTraversable = false;

        // Register listeners and events
        editableLabel.setOnMouseClicked(this::handleMouseClicked);
        editableLabel.setOnKeyPressed(this::handleKeyPressed);
        editableLabel.focusedProperty().addListener( (observable, oldValue, newValue) -> handleFocusChange(newValue));
        editableLabel.focusTraversableProperty().addListener( (observable, oldValue, newValue) -> handleFocusTraversableChange(newValue));
    }

    /************************************************************************
     * @}                                                                   *
     *                                                                      *
     * \defgroup BehaviorMethods                                            *
     *                                                                      *
     * @{                                                                   *
     ***********************************************************************/

    private void handleKeyPressed(KeyEvent event) {
        switch ( event.getCode() ) {
            case ENTER:
                editableLabel.setBaseText(editableLabel.getText());
                exitEditableMode();
                break;
            case ESCAPE:
                exitEditableMode();
                break;
        }
    }

    private void handleMouseClicked(MouseEvent event) {
        if ( event.getClickCount() == editableLabel.getEditableClicks() && !this.isEditing()) {
            enterEditableMode();
        }
    }

    private void handleFocusChange(Boolean newValue) {
        if ( !newValue ) {
            // Save changes and exit editable mode
            editableLabel.setBaseText(editableLabel.getText());
            exitEditableMode();
        } else if ( focusTraversable ){
            enterEditableMode();
        }
    }

    private void handleFocusTraversableChange(Boolean newValue) {
        focusTraversable = newValue;
    }

    private void enterEditableMode() {
        editableLabel.setEditable(true);
        editableLabel.deselect();
        editableLabel.pseudoClassStateChanged(PseudoClass.getPseudoClass("editable"), true);
    }

    private void exitEditableMode() {
        editableLabel.setEditable(false);
        editableLabel.deselect();
        editableLabel.pseudoClassStateChanged(PseudoClass.getPseudoClass("editable"), false);
    }

    /** @} */

}

editablelabel.css

.editable-label:editable {
    -fx-background-color: rgba(0, 191, 255, 0.2);
}

Is there anything I can do, to make this question better, so that people would be more motivated to answer? :'( — JohnRW
– JohnRW, Commented Feb 22, 2016 at 13:45
Yes and no. While you could read meta posts like this to help you out this actually is a good question already. Reviews can take time. It's not uncommon having to wait a day or so before the first answer arrives. — Mast
– Mast ♦, Commented Feb 22, 2016 at 19:59
I'm wondering why you didn't just make a CSS class that turns a TextField into a what you're looking for? I must be missing something. — Emily L.
– Emily L., Commented Mar 1, 2016 at 15:47
Yeah the abbreviation for one (even though i only implemented one kind of it, while label supports a lot of different ones), the click to enter toggle mode with a custom number of clicks needed (single click, double click, ...) and the traversibility i think. Not sure anymore what else I implemented. (Ill comment on the answer from @Marc-Andre tomorrow. Thanks already :) ) — JohnRW
– JohnRW, Commented Mar 1, 2016 at 21:08
Plus i wanted to choose a small custom component to find out if i structured it correctly. Didn't want to bombard codereview with a custom control 10 times the size, when this one suffices to answer some basic questions i have/had. — JohnRW
– JohnRW, Commented Mar 1, 2016 at 21:10

Marc-Andre · Accepted Answer · 2016-02-27 16:47:22Z

Comments and Javadoc

I really like your Javadoc and this is something I rarely say/see. It's clear to the point and does not feel like it does not belong there. What I don't personally like is think like this :

/************************************************************************
 * @}                                                                   *
 *                                                                      *
 * \defgroup StylesheetRelated                                          *
 *                                                                      *
 * @{                                                                   *
 ***********************************************************************/

I guess this is to separate part of the code into logic unit that you can probably either expand/hide with a plugin. The problem I have is it take a lot of place and it "itch" my eyes. The real deal breaker of your comments habit is :

                // editableState change to not editable
                editableState = false;

The comment is really just repeating what the code is doing. Your code is really good and readable, with clear variable name, I really don't need the comment to understand the piece of code. This is happening a couple of times in your code. While it's not critic, it's taking space and make your class bigger for no real benefit. If you have to use those kind of comments, try to explain why the code is written like that, not what the code is. Every comment should add value.

One line method

This is not something I like that you're doing. While it's perfectly accepted and will compile just fine, I always feel it sacrifice readability for space.

@Override
protected Skin<?> createDefaultSkin() { return new EditableLabelSkin(this); }

I've read a good deal of code and normally one line code is a variable declaration, a method call, an if declaration or something like that. It's not a method declaration. What it's doing is I'm quickly scanning your classes and I think there is a tons of variables (which would be weird) when in fact it was a method declaration.

private StringProperty baseText;
public String getBaseText() { return baseText.get(); }
public StringProperty baseTextProperty() { return baseText; }
public void setBaseText(String baseText) { this.baseText.set(baseText); }

I had to look carefully to see that you "packaged" the setter and getter in the same area that you're variable declaration. I'm not used to mixing class variable with methods. Does it imper readability? I would say yes. Would everyone say the same probably not. What I'm used too is something like :

public class Example {
    //variables
    private Long id;
    private String name;

    //methods of the class (no getter/setter)
    @Override
    protected Skin<?> createDefaultSkin() { 
        return new EditableLabelSkin(this); 
    }       

    //getter and setter at the end since normally there nothing special
    public Long getId() {
        return id;
    }

    public void setId(Long id) {
        this.id = id;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

}

This make it easy to see what my class use as internal data, dependencies on others services, etc. Second thing I will read is what I can do with the class. I can quickly see what is the internal function of the class. At the end of the class, I'll have all my setters/getters since most of the times there is nothing special about. I'll probably move up getters and setters with special comportment.

Conclusion

Your code is really of great quality. Everything I had to say was about presentation rather than the implementation. To be honest, I did not your component to see if everything was correct, but from the look of it it's fine. I think you did a great job, but someone with more experience with JavaFX could comment on the implementation. The only thing I would add is that you could change "editable" to a constant and other String like this ("editablelabel.css" and "editablelabel.css").

Thank you for your answer! A lot of the stuff you mentioned i chose, because that is how the official JavaFX components do it. But i defiantly see and agree with the points you made! Ill try to write a more comprehensive comment on your answer tomorrow. Thanks already! Helps me out a lot! Now I'm much more confident about tackling bigger components/classes and really start my project. — JohnRW
– JohnRW, Commented Mar 1, 2016 at 21:10
@JohnRW I'm glad you're now more confident. I really like your code! I think you're in a good track! — Marc-Andre
– Marc-Andre, Commented Mar 1, 2016 at 21:14
Comments: I am planning to switch over to javadoc, however i used doxygen (which is similar) syntax in this case. the defgroup is something doxygen uses to group methods in the generated documentation. Javadoc does not have something like that afaik, but will have to look into that when looking into javadoc in general. Thanks for the feedback though! :D — JohnRW
– JohnRW, Commented Mar 4, 2016 at 17:23
One line method: The places I used them are those, that i tried to keep as close to the structure of “official” javafx components. However I think the same way as you do and think it might make sense, to do it differently the next time. — JohnRW
– JohnRW, Commented Mar 4, 2016 at 17:23
Constants: Good idea! Defiantly going to try to do it that way! — JohnRW
– JohnRW, Commented Mar 4, 2016 at 17:23

Stack Exchange Network

JavaFX custom control - editable label

1 Answer 1

Comments and Javadoc

One line method

Conclusion

You must log in to answer this question.

Hot Network Questions