Text field gui #4
Conversation
phantamanta44
left a comment
phantamanta44
left a comment
There was a problem hiding this comment.
this is great work; the "k" thing is a fantastic idea. here are a few concerns:
- the same concern about enter being used as a navigation key. avoid this
- the same concern about text boxes auto-committing their changes. this should not be possible
- the same concern about auto-formatted code
- you should find some way to define a generic concept of "focusability" for gui components
- it should encapsulate the means of getting previous/next focus targets and also handle the focus state
- could be done by a "focusable" interface that gui components can implement
- once implemented, this should allow you to remove the next-component field from gui components and to avoid hardcoding focusing behaviour for text boxes
- shift+tab should be usable to navigate backwards through focusable components
| return passThrough; | ||
| } | ||
|
|
||
| private boolean focusFirstTextFieldOnKeys(List<Integer> keys, int keyCode) { |
There was a problem hiding this comment.
what is the keys parameter for? it can be factored out
(in fact, this whole method can be inlined, since it only has one call site)
There was a problem hiding this comment.
For now it's useless. On extending functionality may come in handy
| this.mouseOver = false; | ||
| } | ||
|
|
||
| public GuiComponent getComp() { |
There was a problem hiding this comment.
should be called getComponent
There was a problem hiding this comment.
It's auto-generated. Then comp should be called component, but you named it comp
| boolean handleKeyTyped(char typed, int keyCode) { | ||
| return comp.onKeyPress(keyCode, typed); | ||
| boolean success = comp.onKeyPress(keyCode, typed); | ||
| if (keyCode == Keyboard.KEY_ESCAPE |
There was a problem hiding this comment.
we don't want component-level keys and gui-level keys to process at the same time. it might be best to just check the gui-level keys first then skip the component-level keys if needed
| private String value; | ||
| @Nullable | ||
| private final String tooltipKey; | ||
| private FieldType fieldType = FieldType.ALL_TEXT; |
There was a problem hiding this comment.
field type should not be mutable
There was a problem hiding this comment.
You're right. It's inconvenient without a builder instead of just constructors though
| updateValidity(); | ||
| onTextCharInput(typed); | ||
| } else if (keyCode == Keyboard.KEY_DELETE) { | ||
| value = ""; |
There was a problem hiding this comment.
this is no good; users expect the delete key to delete the character in front of the cursor. since the cursor cannot be moved here, the delete key must stay nonfunctional. you can perhaps put this behaviour on ctrl+shift+backspace or something
(is it really even necessary? ctrl+backspace should be good enough for most applications, and certainly for a numeric input)
There was a problem hiding this comment.
this is not necessary, but I didn't know about ctrl backpace
| value = val; | ||
| break; | ||
| case ALL_TEXT: | ||
| default: |
| val += typed; | ||
| } else if (typed == 107) { // letter "k", add 000 (multiply by thousand) if can | ||
| if (val.equals("") || val.equals("0")) { | ||
| val = "1000"; |
There was a problem hiding this comment.
no big deal, it should be invalidated in that case
| val += "0"; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
maybe also add a case for "m", standing for millions
There was a problem hiding this comment.
k k works fine in that case, no need unless you have 10^15 and more
|
I feel like we have different ideas of what as and isn't fine. I think enter navigation is ok(like in excel, which LM kinda is); auto-commit is a big yes and doing otherwise is very much user hand-holding; auto-format is just convenience. |
2 participants
Companion update to Lazy AE2