Project

General

Profile

Bug #11170

The caret position is initially wrong for DECIMAL/INTEGER FILL-IN widgets

Added by Delia Mitric 6 months ago. Updated 27 days ago.

Status:
Test
Priority:
Normal
Assignee:
Razvan-Nicolae Chichirau
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
reviewer:
Hynek Cihlar
production:
No
env_name:
topics:

History

#1 Updated by Delia Mitric 6 months ago

This is a simple testcase:

DEFINE VARIABLE l AS DECIMAL NO-UNDO.
DEFINE FRAME f l NO-LABEL.
ENABLE ALL WITH FRAME f.
MESSAGE LENGTH(l:SCREEN-VALUE).
WAIT-FOR WINDOW-CLOSE OF FRAME f.

In OE: the cursor position is initially 0
In FWD: the cursor position is initially > 0

#2 Updated by Delia Mitric 6 months ago

  • Status changed from New to WIP
  • Assignee set to Delia Mitric

I'll look into this

#3 Updated by Delia Mitric 4 months ago

The initial cursor position for DECIMAL FILL-IN is 8 because of the screen.length() value which is 9.
When the execution reaches the initClear method, the screen contains this text: " 0.00".

Initially, the screen shouldn't contain any string until the user introduce a number into the fill-in or deletes its content. Even if the screen value is known as " 0.00", no value is displayed. This affects just the cursor position.

#4 Updated by Delia Mitric 4 months ago

  • % Done changed from 0 to 20
  • reviewer Hynek Cihlar added

Committed a fix to 11170a branch as rev 16407, but while testing it on different DECIMAL formats, found that for "9999.99" the cursor is still placed wrong initially..

#5 Updated by Delia Mitric 4 months ago

  • Status changed from WIP to Review
  • % Done changed from 20 to 100

The solution is committed to 11170a branch rev. 16408.

Hynek, please review.
Thank you!

#6 Updated by Hynek Cihlar 4 months ago

  • Status changed from Review to WIP
  • % Done changed from 100 to 90

Code review 11170.

  1. ThinClient dependency creates layer violation, NPE risk, and inconsistency: The getInitialCursorPos method introduces a dependency on ThinClient.getInstance().isChui() that has multiple critical issues:
    1. Layer violation: The format package (ui.client.format) is a lower-level layer than the CHUI client layer (ui.chui), and upward dependencies break the existing architecture.
    2. NPE in batch/headless contexts: ThinClient.getInstance() returns null when no ThinClient is bound to the current thread (e.g., in BatchDriver mode or server-side code paths). The unconditional call will throw a NullPointerException in these contexts.
    3. Inconsistency with existing patterns: The entire file uses Utils.isChUI() for CHUI detection at lines 411, 1141, 1349, and 1377. Introducing ThinClient creates a style inconsistency.
    4. Solution: Check whether isCharInterface() (inherited from DisplayFormat) can be used in this case. Consider the other execution modes - batch and redirected streams.

Can the block

         if (firstInitClear)
         {
            invalidateScreen();
            screenBuf = new StringBuilder("");
            firstInitClear = false;
            return;
         }

be moved to init() method and firstInitClear removed?

#7 Updated by Delia Mitric 4 months ago

Rebased 11170a branch.

#8 Updated by Delia Mitric 4 months ago

Hynek Cihlar wrote:

Code review 11170.

Consider the other execution modes - batch and redirected streams.

Hynek, please explain what do you mean by batch and redirected streams execution modes in this context? How should I check also these execution modes for a NumberFormat FILL-IN?

#9 Updated by Razvan-Nicolae Chichirau 2 months ago

  • Assignee changed from Delia Mitric to Razvan-Nicolae Chichirau

#10 Updated by Razvan-Nicolae Chichirau 2 months ago

Suppose a numeric-based fill-in (int, int64 or decimal). In 4GL, if the widget is cleared, either by realizing it (with no screen-value set), or clearing the frame, or clearing the fill-in (CLEAR() method), the following hold:
  • The widget screen value is empty, regardless of the format.
  • If any valid input is passed to a NumberBuf instance, then the updated value will be displayed.
    Tested on GUI + ChUI.

For example, suppose a decimal fill-in. Upon initialization, the 4GL screen-value internally is 0.00, but the shown value is empty. When a user inputs the character 2, then 0.00 should be considered for input validation purposes. In the end, as the character is valid for the current presentation, the updated value will be displayed (i.e, 2.00).

As such, Delia's approach seems wrong, as the presentation should display blanks on every clear call, not just on the first one.

I have 2 implementation ideas:
  1. Store an emptyMode flag in NumberBuf. The default value is false, and on any .clear() call this will be enabled, and disabled again on any valid input. In this mode, the reported screen value will be empty. Methods such as setting the cursor on the first/last position will set the cursor on the 0 position. Methods such as moving cursors to left or right will be a NO-OP.
  2. Implement EmptyPresentation which should act as a wrapper over another presentation. The delegate can control this wrapper through a function which tells if the empty mode should be enabled. In this way, we can decouple the empty presentation logic from the actual implementation.

#11 Updated by Razvan-Nicolae Chichirau 2 months ago

Some updates:
  • Successfully implemented the first idea from #11170-10. The second one was bringing too much overhead for essentially a binary problem: should the presentation act like an empty presentation or not?
  • Initially the solution was for both ChUI and GUI, but testing showed this empty mode (where screen value is empty, cursor can not be moved in either direction) is only true for GUI based interfaces. As such, my solution will touch only GUI.
  • The final idea (most probably) of the changes will be as follows:
    1. Keep 2 states for a GUI number presentation, private and public.
    2. When the empty mode is enabled, external callers of this presentation will see it just as an empty one. Cursor position is always 0, screen value is always the empty string.
    3. Internal callers, such as methods working with inputs will see the "real" presentation state: the screen value is actually the default value for that data-type and the cursor position is aligned accordingly.

#12 Updated by Razvan-Nicolae Chichirau 2 months ago

  • Status changed from WIP to Review
  • % Done changed from 90 to 100

Hynek: Please review 11170a/rev. 16564.

#13 Updated by Hynek Cihlar about 2 months ago

  • % Done changed from 100 to 90
  • Status changed from Review to WIP

Code review 11170a revisions 16561..16564

  • [MINOR] style NumberBufGui.deleteRegion: Javadoc @param tags lack spacing padding — @param start and @param end should be @param start / @param end, and description continuation lines should align to match, consistent with the rest of the file.
  • [MINOR] style NumberBuf.receiveInput: Javadoc @return tag uses two spaces (@return ) instead of the standard three spaces (@return ) used throughout the file.
  • [MINOR] style NumberBufGui.input: Javadoc @param ch description "Input character" is missing a trailing period, inconsistent with the same description in NumberBuf.input ("Input character.").

Otherwise the changes look good.

#14 Updated by Razvan-Nicolae Chichirau about 2 months ago

  • Status changed from WIP to Review
  • % Done changed from 90 to 100

Please check rev. 16565.

#15 Updated by Hynek Cihlar about 2 months ago

  • Status changed from Review to Internal Test

Razvan-Nicolae Chichirau wrote:

Please check rev. 16565.

It's good, please go ahead with regression testing. ChUI regression tests will be needed, too.

#16 Updated by Razvan-Nicolae Chichirau 27 days ago

Testing that passed:
  • ChUI regression tests
  • fill-in, text and combo-box tests from testcases
  • Hotel GUI + 2 customer app smoke-tests
  • Unit-tests from another customer

If this is enough, 11170a should be merged.

#17 Updated by Radu Apetrii 27 days ago

  • Status changed from Internal Test to Merge Pending

Seems good. 11170a can be merged right now.

#18 Updated by Razvan-Nicolae Chichirau 27 days ago

  • Status changed from Merge Pending to Test

Branch 11170a was merged into trunk as rev. 16614 and archived.

Also available in: Atom PDF