Project

General

Profile

Bug #11314

UP/DOWN key pressed after CTRL_LEFT /CTRL_RIGHT in a COMBO-BOX doesn't select the correct list-item

Added by Delia Mitric 4 months ago. Updated 3 months ago.

Status:
Test
Priority:
Normal
Assignee:
Delia Mitric
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 4 months ago

This is a simple COMBO-BOX testcase from #10875:

DEFINE VARIABLE cb AS CHARACTER VIEW-AS COMBO-BOX
  LIST-ITEMS
  "a12", "b34", "c56" 
  SIMPLE
  SIZE 40 BY 5
  NO-UNDO.
DEFINE FRAME f cb NO-LABEL.
ENABLE ALL WITH FRAME f.
WAIT-FOR CLOSE OF CURRENT-WINDOW.

To reproduce this:
1. Select an item from the list
2. Press CTRL_LEFT/CTRL_RIGHT or both
3. Press UP -> the selected item is the current one while it should be the previous one (if it is possible)
OR
Press DOWN -> the selected item is the current one while it should be the next one (if it is possible)

#2 Updated by Delia Mitric 4 months ago

The issue seems to come from the fact that selection from the list is emptied when event CTRL_LEFT/CTRL_RIGHT occurs and that's why the current element is selected again:

// if no row is selected certain keys will select the first row
if (selectionModel.isEmpty() && cbbRef.config().mode == ComboBoxConfig.Mode.SIMPLE)
{
    if (keyCode == Key.VK_CURSOR_UP || keyCode == Key.VK_CURSOR_DOWN)
    {
       selectCurrentRow();
       highlighted = true;
       return;
    }
}

#3 Updated by Delia Mitric 4 months ago

  • Assignee set to Delia Mitric

#4 Updated by Delia Mitric 4 months ago

  • Status changed from New to WIP
  • % Done changed from 0 to 100
  • reviewer Hynek Cihlar added

Committed a solution to 11314a branch rev. 16480.

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

#5 Updated by Delia Mitric 4 months ago

  • Status changed from WIP to Review

#6 Updated by Hynek Cihlar 4 months ago

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

Code review 11314a.

The history entry 028 description reads: "Don't clear the list selection if for CTRL + LEFT/RIGHT keys." — the phrasing "if for" is awkward. Consider rewording to: "Don't clear the list selection for CTRL+LEFT/RIGHT keys."

key != Key.VK_CTRL_CURSOR_LEFT && key != Key.VK_CTRL_CURSOR_RIGHT: key is action code while VK_CTRL_CURSOR_LEFT is a key code. How does the fix work?

#7 Updated by Delia Mitric 4 months ago

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

Hynek Cihlar wrote:

Code review 11314a.

The history entry 028 description reads: "Don't clear the list selection if for CTRL + LEFT/RIGHT keys." — the phrasing "if for" is awkward. Consider rewording to: "Don't clear the list selection for CTRL+LEFT/RIGHT keys."

Sorry, forgot to delete the "if" word from a previous draft..

key != Key.VK_CTRL_CURSOR_LEFT && key != Key.VK_CTRL_CURSOR_RIGHT: key is action code while VK_CTRL_CURSOR_LEFT is a key code. How does the fix work?

It works because the action code is the same as the key code for CTRL + LEFT/RIGHT events.

I changed this into keyCode != Key.VK_CTRL_CURSOR_LEFT && keyCode != Key.VK_CTRL_CURSOR_RIGHT where keyCode = ke.keyCode() to be sure.

Committed rev. 16481.
Please review.
Thank you!

#8 Updated by Hynek Cihlar 4 months ago

Code review 11314a. The changes look good.

Just another point, CTRL+SHIFT+LEFT/RIGHT are not excluded from the selection-clear guard, should they be?

#9 Updated by Delia Mitric 3 months ago

Hynek Cihlar wrote:

Code review 11314a. The changes look good.

Just another point, CTRL+SHIFT+LEFT/RIGHT are not excluded from the selection-clear guard, should they be?

At this moment, the FWD behavior when we
1. Select an item from the list
2. Press CTRL_SHIFT_LEFT/CTRL_SHIFT_RIGHT or both
3. Press UP or DOWN

is the same as in OE, so the answer is no.

Can we move forward with testing?

#10 Updated by Hynek Cihlar 3 months ago

  • Status changed from Review to Internal Test

Please regression test the changes, also include ChUI regression tests.

#11 Updated by Delia Mitric 3 months ago

Smoke testing is done for a Customer GUI app and for Hotel GUI. No regression found.

#12 Updated by Delia Mitric 3 months ago

Rebased the branch and found that a line (a variable declaration) is duplicated in EntryFieldGuiImpl class. So, I removed it and committed the changes to 11314a. Same functionality.

#13 Updated by Razvan-Nicolae Chichirau 3 months ago

Delia, if your changes only modify GUI classes such as EntryFieldGuiImpl, there is no point in running ChUI regression tests. Anyway, they passed and smoke-test on a customer app was successful. ✅

#14 Updated by Delia Mitric 3 months ago

The testing phase is done.
This is ready to be merged.

#15 Updated by Hynek Cihlar 3 months ago

  • Status changed from Internal Test to Merge Pending

Please merge 11314a in trunk.

#16 Updated by Delia Mitric 3 months ago

Branch 11314a was merged into trunk as rev. 16532 and archived.

#17 Updated by Delia Mitric 3 months ago

  • Status changed from Merge Pending to Test

Also available in: Atom PDF