FWD » Core Development » User Interface

I've committed a solution for the first issue to 11044a branch rev. 16324.

I've committed the final solution to 11044a branch rev. 16325

• fixed also the second issue from #11044-1
• added a new abstract method in DisplayFormat to implement it in NumberFormat in order to set the SCREEN-VALUE to an empty string
• prevented to trigger VALUE-CHANGED for a numeric FILL-IN which contains unknown when the user enters an ignored key.

Vladimir, please review. Thank you!

Delia Mitric wrote:

I've committed the final solution to 11044a branch rev. 16325

• fixed also the second issue from #11044-1
• added a new abstract method in DisplayFormat to implement it in NumberFormat in order to set the SCREEN-VALUE to an empty string
• prevented to trigger VALUE-CHANGED for a numeric FILL-IN which contains unknown when the user enters an ignored key.

Vladimir, please review. Thank you!

Code review of rev. 16325.

The recent commit introduces a new abstract method clearScreenValue() in the base class, but does not provide implementations in all subclasses. As a result, the code no longer compiles.
Is the commit incomplete?

Vladimir Tsichevski wrote:

Code review of rev. 16325.

The recent commit introduces a new abstract method clearScreenValue() in the base class, but does not provide implementations in all subclasses. As a result, the code no longer compiles.
Is the commit incomplete?

Yes, it should contain the implementation of the abstract method in al subclasses, but the only one subclass which needs that method is NumberFormat.
I'll try to come with a solution in which only the NumberFormat class will contain clearScreenValue method.

I've changed the solution and committed the new one to 11044a branch rev. 16326
The method which clears the screen-value is added just in NumberFormat class.

Vladimir, please review this one when you get a chance. Thank you!

11044a rev. 16326 – Code Review and Testing

Code Review¶

1. The following block:
   

   // use additional flag for KA_CLEAR event to find out if we should emit VALUE-CHANGED or not
   fireValueChanged = !ke.isSpecial();
   // do not fire VALUE-CHANGED for a DECIMAL/INTEGER FILL-IN which contains unknown value when processing KA_CLEAR
   if (config.appFormat instanceof NumberFormat && getText() == "?")
   {
      fireValueChanged = false;
   }
   

   
   can be simplified to:
   

   // do not fire VALUE-CHANGED for a DECIMAL/INTEGER FILL-IN which contains unknown value when
   // processing KA_CLEAR
   // use additional flag for KA_CLEAR event to find out if we should emit VALUE-CHANGED or not
   fireValueChanged = !ke.isSpecial()
      && !(config.appFormat instanceof NumberFormat && getText() == "?");
   

   
   The reformatting also adheres to our 110-character line limit.
2. New clearScreenValue() method:
   • Make the method static.
   • Correct Javadoc formatting to conform to GCD style guidelines.
   • Reorder method declaration per GCD conventions.
3. New clearScreenBuffer() method:
   • Make the method private and reorder per GCD conventions.
   • Correct Javadoc formatting to GCD style.
   • Replace screenBuf = new StringBuilder(); with screenBuf.setLength(0);.
   • Replace screen = screenBuf.toString(); with screen = "";.

Testing Results¶

Swing GUI¶

Both original issues are resolved for right- and left-aligned FILL-IN widgets.

Swing ChUI¶

Previous correct behavior is fully preserved.

Delia Mitric wrote:

Found that the LOGICAL FILL-IN doesn't behave the same as DECIMAL/INTEGER FILL-IN when it contains "?" (selected or not) or when it contains "yes"/"no" and so on :
LOGICAL:

• contains "?" (selected) and the user press a printable ignored key
  OE: nothing happens
  FWD: the fill-in is emptied

• contains a selected value like "yes" / "no" and the user press a printable ignored key:
  OE: nothing happens
  FWD: the fill-in is emptied

Committed the fix to 11044b rev. 16457

Committed a final solution to 11044b rev. 16458.
(The latest commit (rev. 16457) had a problem: when the user introduces a printable valid key to a numberformat FILL-IN that contains a valid selected number, nothing happens which is wrong. This is what this commit should solve.)

Hynek, when you get a chance, please review. Thanks! :)

Code review 11044b revisions 16454..16458

• [CRITICAL] functional FillIn.processKeyEvent: In the KA_CLEAR branch (line 2022), the getText() "?" guard is evaluated after appDataPres.clear() / deleteRegion() have already executed (lines 2007-2013). clear() calls initClear() which allocates a fresh digits array via initArray() and re-renders the screen buffer via renderScreen(), replacing the "?" unknown-value representation with a numeric zero/blank. Was this intended? Also fireValueChanged is assigned twice.

• [MAJOR] functional FillIn.processKeyEvent: getText() "?" uses Java reference equality (==) instead of .equals("?") to compare strings.

• [MAJOR] style NumberFormat.(file header): Two new history entries (080 and 081) both have sequence IDs. Per convention, when multiple entries are added in the same diff, only the first new entry gets a sequence ID - entry 081 should not have one.

• [MAJOR] style FillInGuiImpl.processKeyEvent: Blank line after opening { on line 1262 (inside the if (invalidateSelection && isSelectionValid()) block). Style rules prohibit blank lines immediately after {.

• [MINOR] style FillIn.(file header): History entry 237 contains a double space ("ignored key") and a grammatical error ("in this cases" should be "in these cases").

• [MINOR] style FillIn.processKeyEvent: Cast ((NumberFormat)config.appFormat) on line 1571 has no space between closing ) of the cast and the identifier, inconsistent with the style ((NumberBuf) pres) used in NumberFormat.clearScreenValue.

• [MINOR] style FillInGuiImpl.processKeyEvent: Trailing space before ; on line 1266: KC_SHIFT_BACKSPACE ;.

Hynek Cihlar wrote:

Code review 11044b revisions 16454..16458

• [CRITICAL] functional FillIn.processKeyEvent: In the KA_CLEAR branch (line 2022), the getText() "?" guard is evaluated after appDataPres.clear() / deleteRegion() have already executed (lines 2007-2013). clear() calls initClear() which allocates a fresh digits array via initArray() and re-renders the screen buffer via renderScreen(), replacing the "?" unknown-value representation with a numeric zero/blank. Was this intended?

Yes, it was intended because the execution can reach that point with fill-in's value equals to "?" (for example, when digits value for a numeric fill-in is null).

Also fireValueChanged is assigned twice.

• [MAJOR] functional FillIn.processKeyEvent: getText() "?" uses Java reference equality (==) instead of .equals("?") to compare strings.

• [MAJOR] style NumberFormat.(file header): Two new history entries (080 and 081) both have sequence IDs. Per convention, when multiple entries are added in the same diff, only the first new entry gets a sequence ID - entry 081 should not have one.

• [MAJOR] style FillInGuiImpl.processKeyEvent: Blank line after opening { on line 1262 (inside the if (invalidateSelection && isSelectionValid()) block). Style rules prohibit blank lines immediately after {.

• [MINOR] style FillIn.(file header): History entry 237 contains a double space ("ignored key") and a grammatical error ("in this cases" should be "in these cases").

• [MINOR] style FillIn.processKeyEvent: Cast ((NumberFormat)config.appFormat) on line 1571 has no space between closing ) of the cast and the identifier, inconsistent with the style ((NumberBuf) pres) used in NumberFormat.clearScreenValue.

• [MINOR] style FillInGuiImpl.processKeyEvent: Trailing space before ; on line 1266: KC_SHIFT_BACKSPACE ;.

These are fixed.

• numeric FILL-IN : do not fire VALUE-CHANGED when the cursor is at the end and the event is DELETE
• logical FILL-IN: do not fire VALUE-CHANGED when the event is BACKSPACE no matter what the FILL-IN contains

Committed all changes to 11044b rev. 16461.
Hynek, please review.

Code review 11044b revisions 16454..16461

The condition getText().equals("?") to check unknown value is not right, "?" is also used for the overflow state. NumberFormat.isUnknown is what you want I believe.

• [CRITICAL] functional FillInGuiImpl.processKeyEvent: The new early return for BoolFormat + printable key (inside if (invalidateSelection && isSelectionValid())) exits before super.processKeyEvent(ke), so the character is never routed to the FillIn input pipeline and appDataPres.input(ch) is never called. A LOGICAL FILL-IN with an active selection receives no data update when the user types a valid char (Y/N/space).

• [CRITICAL] functional FillInGuiImpl.processKeyEvent: For the BoolFormat + KA_DELETE_CHARACTER branch of the new early return, the method exits without calling ke.consume(), clear(...), or super.processKeyEvent(ke). The DELETE is neither applied nor consumed, which both leaves the selection intact and lets an unconsumed DELETE key propagate to other handlers. If the intent is "do nothing on DELETE in LOGICAL", at minimum ke.consume() must still be called.

• [CRITICAL] functional FillIn.processKeyEvent: The new else branch at line ~1568 (config.appFormat instanceof NumberFormat && getText().equals("?")) calls clearScreenValue(config.appDataPres) then sets inputRes = true and falls through. Unlike the sibling if (inputRes) branch, it never assigns appDataPres = config.appDataPres = zapDataPres. The outer block was entered precisely when appDataPres null || appDataPres.isUnknown(), so downstream appDataPres.isLastCharEntered() can NPE when appDataPres was null, and the ch that caused zapDataPres.input(ch) to fail is silently discarded. Please check this case.

• [MAJOR] style FillInGuiImpl.processKeyEvent: The two if (!(Keyboard.isPrintableKey(ke) && config.appFormat instanceof NumberFormat && !getText().equals("?"))) lines exceed the 110-character line-length limit. The condition should be wrapped across multiple lines with operators at line ends.

• [MAJOR] style FillIn.processKeyEvent: The static method NumberFormat.clearScreenValue is invoked via an instance (((NumberFormat) config.appFormat).clearScreenValue(config.appDataPres)). Per style rules, Java static members must be accessed via ClassName.member, i.e. NumberFormat.clearScreenValue(config.appDataPres) (the cast becomes unnecessary).

• [MAJOR] functional NumberBuf.clearScreenBuffer: Leaves NumberBuf in an inconsistent state - only resets screenBuf, screen, and presCursorPos, but does not reset screenInfo[], digits, scale, negative, overflow, extra, justActivated, presInsertMode. Callers indexing screenInfo[presCursorPos] or calling renderScreen()/validateScreen() between this call and the next input()/initClear() will see stale data; also isUnknown() returns true (digitsnull) while toScreenValue() returns "" instead of "?".

• [MAJOR] functional NumberBuf.clearScreenBuffer: Setting presCursorPos = 0 is incorrect for right-aligned GUI FILL-INs where initClear() positions the cursor at the rightmost digit. The empty screen masks the symptom, but this pre-positions the cursor incorrectly if any code inspects presCursorPos before the next input triggers a full initClear(). Please check this.

• [MAJOR] functional NumberFormat.clearScreenValue: Design/API issue - declaring as public static taking a parent DisplayFormat.Presentation and doing an internal instanceof NumberBuf check silently no-ops for unexpected types. Preferred pattern is either an overridable instance method on DisplayFormat.Presentation (default no-op, override in NumberBuf) or a typed instance method on NumberBuf invoked after an explicit cast at the call-site, parallel to how clear(Presentation) is an abstract instance method on DisplayFormat. The current form hides intent and loses polymorphism.

• [MAJOR] functional FillIn.processKeyEvent: In the new zap-failure branch that calls clearScreenValue and sets inputRes = true, the code does not call ConfigHelper.setModified(config, true), updateCursorOffsetValue(), or clearOverride(), all of which are called in the sibling if (inputRes) success branch a few lines above. Because the screen value effectively changes ("?" -> ""), at least the modified flag and cursor offset update should be invoked to keep config state consistent. Please check this.

• [MAJOR] functional FillIn.processKeyEvent: Asymmetric treatment of BACKSPACE vs DELETE_CHARACTER. The BACKSPACE block (line ~2288) adds only !BoolFormat, while the DELETE_CHARACTER block (line ~2335) adds only !(NumberFormat && getText().equals("?")). BACKSPACE on a DECIMAL/INTEGER FILL-IN containing "?" will still fire VALUE-CHANGED, and DELETE_CHARACTER on a LOGICAL FILL-IN will still fire VALUE-CHANGED. The suppression rules from #11044 should apply uniformly to BACKSPACE/SHIFT-BACKSPACE/DELETE-CHARACTER for both formats. Please check this.

• [MAJOR] functional FillInGuiImpl.processKeyEvent: The new early-return guard at the top bypasses the remainder of processKeyEvent, including super.processKeyEvent(ke) and offset = getCursorOffset(). Consider narrower guards that only skip the specific clear/invalidate actions rather than returning from the method.

• [MAJOR] functional FillInGuiImpl.processKeyEvent: The new if (actionCode SE_VALUE_CHANGED && NumberFormat) { invalidateSelection(); } sits inside if (invalidateSelection && isSelectionValid()) and unconditionally invalidates selection on SE_VALUE_CHANGED. The original exclusion list (KA_CLEAR, SE_ENTRY, SE_VALUE_CHANGED) intentionally skipped invalidation for SE_VALUE_CHANGED; forcing invalidation breaks that contract for NumberFormat even when the value-change originated from paths that should have preserved the selection. Please check this.

• [MINOR] style FillInGuiImpl.processKeyEvent: The new compound condition if ((((actionCode KA_DELETE_CHARACTER || isBkspOrShiftBksp) && getText().equals("?")) && NumberFormat) || ...) uses one redundant outer parenthesis pair around the whole expression.

• [MINOR] style NumberFormat.clearScreenValue: The @param pres description "The data presentation instance which is a NumberBuf instance" is missing a trailing period, inconsistent with other Javadoc entries in the file.

• [MINOR] performance FillInGuiImpl.processKeyEvent: Redundant repeated getText(), Keyboard.isPrintableKey(ke), and instanceof evaluations across the three new sequential if blocks on every keystroke; hoisting these into local booleans once would avoid repeated work and improve readability.

• [MINOR] functional FillInGuiImpl.processKeyEvent: The first guard condition is dense and nearly impossible to read; extract to named booleans (e.g., isDeleteOnUnknownNumber, isEditOnBool) for clarity and to expose intent to future maintainers.

• [MINOR] functional FillInImpl: The GUI-side selection-handling changes in FillInGuiImpl.processKeyEvent have no CHUI counterpart. The shared FillIn.java does receive the fireValueChanged updates, but the selection-invalidation logic is GUI-only. If the ticket's fix is semantically about selection handling around unknown DECIMAL/INTEGER/LOGICAL values, some of the logic may belong in the base FillIn.processKeyEvent rather than only in the GUI impl.

• [MINOR] functional FillInGuiImpl.processKeyEvent: The BoolFormat early-return branch triggers on KA_DELETE_CHARACTER, but BACKSPACE, SHIFT-BACKSPACE, KA_CLEAR, and KA_CUT are not treated symmetrically (BACKSPACE on a LOGICAL FILL-IN with selection will still clear+consume and fire super). If the intent is only DELETE_CHARACTER, the rationale should be documented.

• [MINOR] functional FillIn.processKeyEvent: In the KA_CLEAR branch, invalidateSelection() is called unconditionally (line ~2007) immediately after computing the new fireValueChanged. Even when VALUE-CHANGED is deliberately suppressed for a LOGICAL/"unknown numeric" FILL-IN, the selection is still cleared; whether that is intentional should be verified.