Testcase:

DEF VAR cb1 AS CHARACTER
     VIEW-AS COMBO-BOX INNER-LINES 10
     LIST-ITEMS 
     "a", "b", "c" 
     SIMPLE
     SIZE 40 BY 5
     NO-UNDO.

DEFINE FRAME f
    cb1
    WITH 1 DOWN SIDE-LABELS.
ENABLE ALL WITH FRAME f. 

DEFINE VAR i AS INTEGER NO-UNDO INITIAL 0.

ON VALUE-CHANGED OF cb1 DO:
    i = i + 1.
    MESSAGE cb1:SCREEN-VALUE i.
END.

WAIT-FOR CLOSE OF CURRENT-WINDOW.

If you will introduce something in the editable field and press BACKSPACE you'll see that VALUE-CHANGED is triggered two times.

If we select all the text from the field and press BACKSPACE, the behavior is the same.

As Vladimir said in #10680-27, when deleting with no selection, VALUE-CHANGED is triggered three times by DELETE-CHARACTER and BACKSPACE events. It should be triggered once.

New problem unlocked 🧐:
Even the editable field is empty, DELETE_CHARACTER and BACKSPACE generate VALUE-CHANGED (three or two times), but they shouldn't because there is no character deleted.

Delia Mitric wrote:

New problem unlocked 🧐:
Even the editable field is empty, DELETE_CHARACTER and BACKSPACE generate VALUE-CHANGED (three or two times), but they shouldn't because there is no character deleted.

Actually, in Progress when deleting through DELETE_CHARACTER when the content of the widget is empty it triggers VALUE-CHANGED when the widget is a FILL-IN.

Testing 10736a, rev. 16229.

The issues found:

1. When the user deletes all content in the combo-box:
   1. In OE: The SCREEN-VALUE becomes unknown.
   2. In FWD: It becomes an empty string.
2. When the user types a value and then switches to the list by pressing DOWN:
   1. In OE: The matching item is selected (or the first item if no match).
   2. In FWD: The previously selected item is re-selected.
3. REGRESSION: When the user types a matching value, switches to the list with DOWN, then deletes the value using DELETE-CHARACTER or BACKSPACE:
   1. In OE: The selected item is deselected, and the VALUE-CHANGED event is triggered.
   2. In FWD: The selected item remains selected (no deselection), the VALUE-CHANGED event is not triggered.
4. When the user switches to the list with DOWN, then presses Left or Right, then Down or Up:
   1. In OE: The next (for Down) item or previous (for Up) item is selected, and the VALUE-CHANGED event is triggered with the newly selected value.
   2. In FWD: The selected item remains the same, the VALUE-CHANGED event is triggered for this currently selected item.

Vladimir Tsichevski wrote:

1. REGRESSION: When the user types a matching value, switches to the list with DOWN, then deletes the value using DELETE-CHARACTER or BACKSPACE:

1. In OE: The selected item is deselected, and the VALUE-CHANGED event is triggered.
2. In FWD: The selected item remains selected (no deselection), the VALUE-CHANGED event is not triggered.

About this, I've tested and the scenario when no deselection is made and no VALUE-CHANGED is triggered is when the text from the EntryFieldGuiImpl is selected (because we can have an item from the list selected and the text from the entry field to be the same, but no selected if makes sense :) ). This is treated as a CLEAR event which is avoided in FillIn.processKeyEvent for an EntryFieldGuiImpl instance. (refs. #9259).

I'm working on this.

Most probably, we should "process" the deleting events after the super.processKeyEvent in EntryFieldGuiimpl when the event is viewed as a CLEAR event because this is the case when the VALUE-CHANGED event is not triggered by the parent class.

Some conclusions after more debugging:
With trunk, for a CLEAR event generated by selected text in entry field and BACKSPACE or DELETE_CHARACTER, VALUE-CHANGED is triggered twice.
For deleting a char (no selection in entry field) through BACKSPACE or DELETE_CHARACTER, VALUE-CHANGED is triggered three times.

1. For a CLEAR event like above, the only place where a VALUE-CHANGED is posted is in EntryFieldGuiImpl.processKeyEvent at the final of the method. VALUE-CHANGED event is processed in EntryFieldGuiImpl.processKeyEvent which calls ComboBoxGuiImpl.entryFieldChanged where another VALUE-CHANGED is posted, but for the COMBO-BOX widget. => 2 VALUE-CHANGED

2. For deleting a char (no selection in entry field), the event (BACKSPACE or DELETE_CHARACTER) is processed in parent (FillIn class) where VALUE-CHANGED is posted (first time). The other two VALUE-CHANGED events are posted like in the first case (in EntryGuiImpl.processKeyEvent) after super call. => 3 VALUE-CHANGED

An idea to solve this is to "mark" by using a flag in EntryFieldGuiImpl class when in processKeyevent "arrives" a "hidden" CLEAR event made through BACKSPACE/SHIFT-BACKSPACE/DELETE_CHARACTER.
This flag can be checked to post a VALUE-CHANGED event just for CLEAR event and also to be used in ComboBoxGuiImpl.entryFieldGuiImpl method to avoid a new VALUE-CHANGED event.

I'll put together a patch to make it clearer 🙂.

Also, I'll try to see if I can build another solution based on changing the condition from FillIn.processKeyEvent which avoids a CLEAR event for an EntryFieldGuiImpl instance.

So, Vladimir, does this mean that the status can be changed to Internal Test? (I saw that you changed the Done percentage)

Also, should I treat the other issues from #10736-11 under this one?

Sorry for bothering, just to be sure :). Thank you!

Delia Mitric wrote:

So, Vladimir, does this mean that the status can be changed to Internal Test? (I saw that you changed the Done percentage)

This was by accident, I think :(

Also, should I treat the other issues from #10736-11 under this one?

It's up to you: either address them now while you're focused on the topic, or create separate issue(s) in the UI project.

Ok :)
I'll treat the first issue (with the unknown screen-value) here because it is trivial, but I'll create a separate task for the other ones (2 and 4 issues form #10736-11).

Created #10791 for the issues related to DOWN and UP events.

I've committed to 10736a rev. 16231 a fix for ComboBoxGuiImpl.getScreenValue() to return unknown the entry field is empty.

Vladimir, can you review please?
Thank you!

Delia Mitric wrote:

Created #10791 for the issues related to DOWN and UP events.

---

I've committed to 10736a rev. 16231 a fix for ComboBoxGuiImpl.getScreenValue() to return unknown the entry field is empty.

Vladimir, can you review please?
Thank you!

If the entry contains only spaces, the value returned by SCREEN-VALUE is:

1. In FWD 10736a rev. 16231: the unknown value.
2. In OE: an empty character value.

Delia Mitric wrote:

Vladimir, I've fixed what you wrote in #10736-24 in rev. 16232.
Please review it. Thank you!

The code is OK. The only note: I'd optimize the processKeyEvent code you added:

      boolean clearedUsingBkspOrDelete = false;

      // Mark if this is a clear event generated by BACKSPACE or DELETE_CHARACTER.
      // This type of events don't lead to a VALUE-CHANGED event in parent class for an EntryFieldGuiImpl instance,
      // this is why it's necessary to post one VALUE-CHANGED in this method.
      if (key == Keyboard.KA_BACKSPACE ||
          key == Keyboard.KA_DELETE_CHARACTER ||
          key == Keyboard.KC_SHIFT_BACKSPACE)
      {
         String selectedText = getSelectedText();
         clearedUsingBkspOrDelete = selectedText != null ? selectedText.length() > 0 : false;
      }

as (use && instead of ?:false, and avoid the clearedUsingBkspOrDelete being assigned twice:

      final boolean clearedUsingBkspOrDelete;

      // Mark if this is a clear event generated by BACKSPACE or DELETE_CHARACTER.
      // This type of events don't lead to a VALUE-CHANGED event in parent class for an EntryFieldGuiImpl instance,
      // this is why it's necessary to post one VALUE-CHANGED in this method.
      if (key == Keyboard.KA_BACKSPACE ||
          key == Keyboard.KA_DELETE_CHARACTER ||
          key == Keyboard.KC_SHIFT_BACKSPACE)
      {
         String selectedText = getSelectedText();
         clearedUsingBkspOrDelete = selectedText != null && selectedText.length() > 0;
      }
      else
      {
         clearedUsingBkspOrDelete = false;
      }

Considering that getSelectedText() code smells for me (see below), it could be even shorter:

      final boolean clearedUsingBkspOrDelete =
               (key == Keyboard.KA_BACKSPACE ||
                key == Keyboard.KA_DELETE_CHARACTER ||
                key == Keyboard.KC_SHIFT_BACKSPACE) &&
               isSelectionValid();

Or, if you want to still use getSelectedText() here, you may omit the && selectedText.length() > 0 part, since if selection exist, its text can never be empty.

Next, the only usage of this new local is:

         // in GUI delete, backspace generate combo-box change.
         // Process a VALUE-CHANGED event if this is a clear event
         if ((key == Keyboard.KA_BACKSPACE ||
              key == Keyboard.KA_DELETE_CHARACTER ||
              key == Keyboard.KC_SHIFT_BACKSPACE) &&
             config.modified && clearedUsingBkspOrDelete)

Here the check for the key value is redundant.

Also, the value is neither used not can be affected until the call to super.processKeyEvent(ke), so its declaration and calculation can be safely moved down the code.

So the final method body could look like this:

   @Override
   public void processKeyEvent(KeyInput ke)
   {
      int key = ke.actionCode();
      ...
         final boolean wasSelectionValid = isSelectionValid();         
         super.processKeyEvent(ke);

         ...

         // Mark if this is a clear event generated by BACKSPACE or DELETE_CHARACTER.
         // This type of events don't lead to a VALUE-CHANGED event in parent class for an EntryFieldGuiImpl instance,
         // this is why it's necessary to post one VALUE-CHANGED in this method.
         if ((key == Keyboard.KA_BACKSPACE ||
              key == Keyboard.KA_DELETE_CHARACTER ||
              key == Keyboard.KC_SHIFT_BACKSPACE) &&
             config.modified && wasSelectionValid)
         {
            ...
         }
      }
   }

General note about why getSelectedText() looks strange for me.

There seems to be a global issue with the COMBO-BOX entry implementation in FWD: The visible text does not always match the value returned internally by getText(), which causes multiple bugs and difficulties in code understanding.
The value returned by getText() strips trailing zeros. Meanwhile, the cursor position and selection range still correspond to the actual screen value.
This has already caused at least two issues I encountered today: #10807 (client crash) and #10808 (combo value changed by navigation keys and mouse clicks).

Vladimir, I changed what you've said.
Please review the 16233 revision.
Thank you!

Delia Mitric wrote:

Vladimir, I changed what you've said.
Please review the 16233 revision.
Thank you!

Testing 10736a rev. 16233

Regression found: StringIndexOutOfBoundsException in client when the user presses DELETE.

Scenario:

1. Enter any text with trailing spaces.
2. Press Delete.

Stack trace:

  Thread [main] (Suspended (exception StringIndexOutOfBoundsException))
  String.substring(int, int) line: 1963
  EntryFieldGuiImpl(FillInGuiImpl).adjustEditScroller(int, boolean) line: 1925
  EntryFieldGuiImpl(FillInGuiImpl).processKeyEvent(KeyInput) line: 1387
  EntryFieldGuiImpl.processKeyEvent(KeyInput) line: 302
  ComboBoxGuiImpl.processKeyEvent(KeyInput) line: 632
  ComboBoxGuiImpl(ComboBox<O>).lambda$2(KeyInput) line: 1759

Vladimir Tsichevski wrote:

Testing 10736a rev. 16233

Regression found: StringIndexOutOfBoundsException in client when the user presses DELETE.

Scenario:

1. Enter any text with trailing spaces.
2. Press Delete.

Stack trace:

[...]

This is because of the changes that skip DELETE_CHARACTER event handling (in FillIn.processKeyEvent) if the cursor is at the end of the text in the entry field.
The exception comes from FillInGuiImpl.adjustEditScroller method when the entry field's content has trailing spaces. getCursorOffset provides the correct position of the cursor, but the length of the text is computed by trimming the spaces from the end. When attempting to obtain the substring: txt = txt.substring(0, offset);, the exception is thrown because the endIndex is greater than the length of the text.

Actually, the regression from #10736-29 seems to be introduced with a trunk revision. I'll search for it.

Vladimir, the trunk revision causing the regression from #10736-29 is 16270.

Delia Mitric wrote:

Vladimir, the trunk revision causing the regression from #10736-29 is 16270.

I see this issue in 10736a rev. 16233, which is based on trunk rev. 16227, which does not contain change from trunk rev. 16270.
Also, I do not see this problem in the current trunk.
Please, rebase 10736a to the recent trunk before we can continue.

Vladimir Tsichevski wrote:

Delia Mitric wrote:

Vladimir, the trunk revision causing the regression from #10736-29 is 16270.

I see this issue in 10736a rev. 16233, which is based on trunk rev. 16227, which does not contain change from trunk rev. 16270.
Also, I do not see this problem in the current trunk.
Please, rebase 10736a to the recent trunk before we can continue.

I tested with the current trunk and problem occurs: getCursorOffset() result is greater than getText().length() if the text contains trailing spaces and we press delete.

Rebased 10736a with the recent trunk.

Delia Mitric wrote:

Vladimir Tsichevski wrote:

Delia Mitric wrote:

Vladimir, the trunk revision causing the regression from #10736-29 is 16270.

I see this issue in 10736a rev. 16233, which is based on trunk rev. 16227, which does not contain change from trunk rev. 16270.
Also, I do not see this problem in the current trunk.
Please, rebase 10736a to the recent trunk before we can continue.

I tested with the current trunk and problem occurs: getCursorOffset() result is greater than getText().length() if the text contains trailing spaces and we press delete.

Rebased 10736a with the recent trunk.

I still see 16233. Probably, you did not push the branch after rebase?

Vladimir Tsichevski wrote:

I still see 16233. Probably, you did not push the branch after rebase?

Yes, sorry.. now it should be rebased :)

Vladimir Tsichevski wrote:

Also, I do not see this problem in the current trunk.

Now I see this in the trunk as well, so this is not a regression. Sorry for the false alarm.
Please, register this as another issue, link to this task ans #10801.

Code review and testing: 10736a rev. 16281.

The change is good and fixes the original issue.

Vladimir, should we proceed by setting the status to Internal Test?

Delia Mitric wrote:

Vladimir, should we proceed by setting the status to Internal Test?

Yes

What I'll do:

• run the xfer tests for COMBO-BOX widget
• smoke testing on a GUI Customer App

I'll talk with Razvan for ChUI tests and with Serban for smoke testing on another GUI Customer app.

Delia Mitric wrote:

What I'll do:

• smoke testing on a GUI Customer App

This is done. No regression found.

I did some testing with my combo-box testcases and the customer app and have some findings. Note that those are not necessarily regressions from 10736a, they might just be missing features from FWD, this needs to be checked.

Simple COMBO-BOX¶

Testcase:¶

define variable cb as character
     view-as combo-box inner-lines 20
     list-items
     "a","b":U,
     "b":U,"b":U,
     "c":U,"c":U,
     "d", "e", "f", "g", "h", "j", "k", "l" 
     simple
     size 50 by 4
     bgcolor 15 fgcolor 19  no-undo.

define frame fr.
enable cb with frame fr.

on value-changed of cb do:
message cb:screen-value now.
end.

wait-for close of current-window.

Findings:¶

1. CTRL-A triggers VALUE-CHANGED in OE but doesn't in FWD. Weird behavior by OE, the value doesn't actually change.
2. Page down/up don't work in FWD.

Besides this, the customer app successfully passed testing.

Delia Mitric wrote:

What I'll do:

• run the xfer tests for COMBO-BOX widget

Related xfer tests passed.

Șerban Bursuc wrote:

I did some testing with my combo-box testcases and the customer app and have some findings. Note that those are not necessarily regressions from 10736a, they might just be missing features from FWD, this needs to be checked.

Simple COMBO-BOX¶

Testcase:¶

[...]

Findings:¶

1. CTRL-A triggers VALUE-CHANGED in OE but doesn't in FWD. Weird behavior by OE, the value doesn't actually change.
2. Page down/up don't work in FWD.

I checked and these findings occur also with the clean trunk, so those are not regressions from 10736a.

I checked and these findings occur also with the clean trunk, so those are not regressions from 10736a.

Great, then no issue from my side.

From past experiences, please also run ChUI tests, there are changes in FillIn which affect ChUI.

Nvm I looked at the changes and they are GUI specific.

ChUI regression tests passed for 10736a.

Testing is done.

Please let me know when 10736a branch can be merged :)