Bug #8012: Fix combo-box corner cases - User Interface - Golden Code Redmine

Bug #8012

Fix combo-box corner cases

Added by Radu Apetrii 8 months ago. Updated about 1 month ago.

Status:

Internal Test

Priority:

Normal

Assignee:

Radu Apetrii

Target version:

Start date:

Due date:

% Done:

100%

billable:

vendor_id:

GCD

case_num:

version_reported:

version_resolved:

History

#1 Updated by Radu Apetrii 8 months ago

Status changed from New to WIP
Assignee set to Radu Apetrii

There are quite a lot of corner cases regarding combo-boxes. While working on another task, I managed to identify a few by creating some tests. The point of this task is to have a solution for all of them, even if there will be more to come.

The tests that I've created until this moment of time are on testcases/uast/combo_box/screen_value, added in rev. 2388. They mainly target the screen-value of the combo-boxes with/without list-item-pairs. Also, there is a file called ComboBoxAnalyzer.java which highlights the mismatches in results between Progress and FWD. One can use that file after they've made sure that the generated results (i.e. combo-box1-Progress.txt and combo-box1-FWD.txt) are in the same folder.

I've used branch 7806b to make changes and test these cases. I'm not done with the implementation, but I wanted to point out the branch that I've been using.

Feel free to add more people to the watching list and to post other cases that provide wrong results.

#4 Updated by Radu Apetrii 7 months ago

I added to testcases/uast/combo_box/screen_value some more tests regarding ADD-LAST, ADD-FIRST, LIST-ITEM-PAIRS, SCREEN-VALUE, INSERT, DELETE and combinations of them. The commit is on testcases rev. 2389.

Also, on 7806b rev. 14839 I added some more changes related to combo-boxes:

Added a delete(long) function in ComboBoxWidget.
Done some better handling on the append-type methods (such as INSERT, ADD-FIRST, ADD-LAST).
Also, I rebased 7806b with trunk rev. 14835 since there were some changes of interest in one of the latest revisions.

As there are quite a lot of logic-based changes, I will create a separate post to explain each one of them.

#5 Updated by Radu Apetrii 7 months ago

After putting some thought into this, I came up with a format that, IMO, would make the changes relatively easy to read and examine. That is, present the problems in the following way: Hypothesis, Reason/Test that backs it up, Solution.

1. Calling SCREEN-VALUE before setting LIST-ITEM-PAIRS affects the next SCREEN-VALUE values. Show Hide

Test (extracted from testcases/uast/combo_box/screen_value/combo-box2.p):

DEFINE VARIABLE cbType1 AS CHARACTER
     VIEW-AS COMBO-BOX.
DEFINE VARIABLE cbType2 AS CHARACTER
     VIEW-AS COMBO-BOX.

DEFINE FRAME f-main cbType1 cbType2.
DO WITH FRAME f-main:
    cbType1:LIST-ITEM-PAIRS = "G,".
    MESSAGE cbType1:SCREEN-VALUE. // "" 

    MESSAGE cbType2:SCREEN-VALUE. // "?" 
    cbType2:LIST-ITEM-PAIRS = "G,".
    MESSAGE cbType2:SCREEN-VALUE. // "?" 
END.

Solution:

When setting the LIST-ITEM-PAIRS, check if we have used the SCREEN-VALUE before setting it again:
```
(listItemPairsConfig.usedScreenValue == null || listItemPairsConfig.usedScreenValue == false)
```
- listItemPairsConfig.usedScreenValue equal to null means that no getScreenValue was called up until this point.
- listItemPairsConfig.usedScreenValue equal to false means that getScreenValue was not called before setting LIST-ITEM-PAIRS.
- listItemPairsConfig.usedScreenValue equal to true means that getScreenValue was called before setting LIST-ITEM-PAIRS.
Update the usedScreenValue field when calling getScreenValue.

2. When adding/inserting/deleting in/from LIST-ITEM-PAIRS, SCREEN-VALUE takes into account the first label used. Show Hide

Test (extracted from testcases/uast/combo_box/screen_value/combo-box12.p):

DEFINE VARIABLE cbType1 AS CHARACTER
     VIEW-AS COMBO-BOX.
DEFINE VARIABLE cbType2 AS CHARACTER
     VIEW-AS COMBO-BOX.

DEFINE FRAME f-main cbType1 cbType2.
DO WITH FRAME f-main:
    cbType1:LIST-ITEM-PAIRS = "A,,B,".
    cbType1:DELETE(""). // Items are "B," 
    MESSAGE cbType1:SCREEN-VALUE. // "?" 

    cbType2:LIST-ITEM-PAIRS = "A,,B,".
    cbType2:LIST-ITEM-PAIRS = "B,". // Items are "B," 
    MESSAGE cbType2:SCREEN-VALUE. // "" 
END.

Solution:

When setting the LIST-ITEM-PAIRS, check if we are in append mode (ADD, INSERT, DELETE) and the label that we are trying to set is the same as the first one ever set.
```
(!append || listItemPairsConfig.labelWhenFirstListItemPairs.equals(labelText)) // if this is true, the program will continue to evaluate if empty value is valid
```

3. By setting multiple LIST-ITEM-PAIRS, SCREEN-VALUE can remain empty string only if it is not accessed (through a getter) in between the time. Show Hide

Test:

DEFINE VARIABLE cbType1 AS CHARACTER
     VIEW-AS COMBO-BOX.
DEFINE VARIABLE cbType2 AS CHARACTER
     VIEW-AS COMBO-BOX.

DEFINE FRAME f-main cbType1 cbType2.
DO WITH FRAME f-main:
    cbType1:LIST-ITEM-PAIRS = "A,".
    MESSAGE cbType1:SCREEN-VALUE. // "" 
    cbType1:LIST-ITEM-PAIRS = "B,".
    MESSAGE cbType1:SCREEN-VALUE. // "?" 

    cbType2:LIST-ITEM-PAIRS = "A,".
    cbType2:LIST-ITEM-PAIRS = "B,".
    MESSAGE cbType2:SCREEN-VALUE. // ""

Solution:

Even if LIST-ITEM-PAIRS is set multiple times, consider each one as the first time as long as there are no SCREEN-VALUE gets in the meantime.

When going from one value to another, listItemPairsConfig.firstSetListItemPairs will become false (meaning that we have set LIST-ITEM-PAIRS more than once) only if we have got SCREEN-VALUE earlier. Otherwise, it will be considered true (with the meaning that it is the first LIST-ITEM-PAIRS):

else if (listItemPairsConfig.firstSetListItemPairs != null && 
                  listItemPairsConfig.firstSetListItemPairs == true &&
                  listItemPairsConfig.usedScreenValue != null)
         {
            listItemPairsConfig.firstSetListItemPairs = false;
         }

4. Using append-type methods (such as INSERT, ADD-FIRST, ADD-LAST) instead of directly setting LIST-ITEM-PAIRS will not influence SCREEN-VALUE. Show Hide

Test:

DEFINE VARIABLE cbType1 AS CHARACTER
     VIEW-AS COMBO-BOX.
DEFINE VARIABLE cbType2 AS CHARACTER
     VIEW-AS COMBO-BOX.

DEFINE FRAME f-main cbType1 cbType2.
DO WITH FRAME f-main:
    cbType1:LIST-ITEM-PAIRS = "B,".
    MESSAGE cbType1:SCREEN-VALUE. // "" 
    cbType1:LIST-ITEM-PAIRS = "A,,B,".
    MESSAGE cbType1:SCREEN-VALUE. // "?" 

    cbType1:LIST-ITEM-PAIRS = "B,".
    MESSAGE cbType1:SCREEN-VALUE. // "" 
    cbType1:HANDLE:ADD-FIRST("A", "").
    MESSAGE cbType1:SCREEN-VALUE. // "" 
END.

Solution:

If we previously worked with an empty value, and setting the current LIST-ITEM-PAIRS is through an append-type method, then empty value (for the SCREEN-VALUE) can still be considered valid and further analyzed.

The correspondent in ComboBoxWidget.checkItems is the following part of the if statement:

                append &&
                listItemPairsConfig.firstSetListItemPairs == null &&
                listItemPairsConfig.initialEmptyValue != null &&
                listItemPairsConfig.initialEmptyValue == true

5. When setting SCREEN-VALUE, "" is not treated the same as " " (shocking, I know). Show Hide

Test (extracted from testcases/uast/combo_box/screen_value/combo-box3.p):

DEFINE VARIABLE cbType1 AS CHARACTER
     VIEW-AS COMBO-BOX.
DEFINE VARIABLE cbType2 AS CHARACTER
     VIEW-AS COMBO-BOX.

DEFINE FRAME f-main cbType1 cbType2.
ENABLE ALL WITH FRAME f-main.
DO WITH FRAME f-main:
    cbType1:LIST-ITEM-PAIRS = "A,".
    cbType2:LIST-ITEM-PAIRS = "A,".

    cbType1:SCREEN-VALUE = "".
    cbType2:SCREEN-VALUE = " ".

    MESSAGE cbType1:SCREEN-VALUE. // "?" 
    MESSAGE cbType2:SCREEN-VALUE. // " " 
END.

Solution:

This was solved in #7806 by adding an unparsedValue parameter to ComboBoxWidget.validateScreenValue and comparing, for the second case (with " "), the unparsedValue and the parsedValue and determining if empty string is a valid case for the SCREEN-VALUE.

Most likely there are some cases that I've missed, but I will keep everyone up to date as soon as I find them. Also, 7806b reached rev. 14840 after I added the fix for point 3.

Currently, all 34 test files that I have created are working as in Progress. Also, navigating through a large customer application raised no errors. However, this needs extensive testing.

#7 Updated by Radu Apetrii about 1 month ago

I rebased 7806b with trunk rev. 15226. I suggest waiting for the #8407 result before we decide what to do with 7806b.

#8 Updated by Radu Apetrii about 1 month ago

Greg, the branch 7806b solves the issue on #8407. The problem (?) is that this branch contains multiple fixes for combo-boxes and it would be quite difficult to pinpoint the exact one that solves #8407. How should we approach this? If the plan is to get 7806b into trunk (and maybe create a 7806c for future combo-box fixes), I definitely need to do a double check of the changes, then we would need a review, and then some regression testing.

#9 Updated by Greg Shah about 1 month ago

Yes, let's move ahead with 7806b and further changes can go in another branch. Please set the %Done of this task accordingly. If you have a list of the things fixed in 7806b, that would probably make sense to post as well.

#10 Updated by Radu Apetrii about 1 month ago

% Done changed from 0 to 100
Status changed from WIP to Review

Check the 7806b changes: Done.
Check that the 34 tests that I've created pass: Done.
Status: waiting for review.

In terms of the fixed cases, they can be found in #8012-5 (don't forget to click Show) plus the addition of the delete function in #8012-4. To be more specific, let's consider the first point from #8012-5: 1. Calling SCREEN-VALUE before setting LIST-ITEM-PAIRS affects the next SCREEN-VALUE values.. In there, one can find an example (4GL code), then the conclusion drawn from that example in the title of the list element (for example 1. Calling SCREEN-VALUE before setting LIST-ITEM-PAIRS ... is the conclusion), and then the solution with which I have come up. I recommend going through these cases because in ComboBoxWidget there's a massive if statement now, and knowing the initial problems would help divide that into smaller/easier to read conditions.

I'll put this at 100% Done for now because all the cases that are presented in this task are solved. When new examples will appear, I'll revert the percentage accordingly.

#11 Updated by Greg Shah about 1 month ago

Hynek: Please review.

#12 Updated by Hynek Cihlar about 1 month ago

Code review 7806b. The changes are pretty dense so it is hard to follow every case, but they look OK to me.

For the added append parameter, could we deduce new items are being added from the newAddedItems argument? I.e. if it is not empty, we are appending?

Radu, if you haven't please also regression test the other widgets that this set of changes affect, i.e. RADIO-SET, SELECTION-LIST and BROWSE-COLUMN.

#13 Updated by Alexandru Lungu about 1 month ago

Radu, please look into 4GL_Unit_Testing and Writing_4GL_Testcases, more exactly into what we call "xfer testcases".

This is quite a broad task and the changes are dense as Hynek mentioned. I think it is for the best to:

Create a baseline with the available tests. I see tests which seem quite complex:
- tests/ui_tests/screen_value/Combo*.cls

#14 Updated by Marian Edu about 1 month ago

Alexandru Lungu wrote:

Radu, please look into 4GL_Unit_Testing and Writing_4GL_Testcases, more exactly into what we call "xfer testcases".
This is quite a broad task and the changes are dense as Hynek mentioned. I think it is for the best to:

Create a baseline with the available tests. I see tests which seem quite complex:

tests/ui_tests/screen_value/Combo*.cls

There are also a bunch of test cases written already in tests/ui_tests/combo_box, split in attributes and methods that you can checkout.

#15 Updated by Radu Apetrii about 1 month ago

Hynek Cihlar wrote:

For the added append parameter, could we deduce new items are being added from the newAddedItems argument? I.e. if it is not empty, we are appending?

Theoretically, yes, we could. Practically, this would be a bit of a hassle. In the current situation, newAddedItems seems to behave differently from what we expect (or is simply not working on all the cases). For example, in the test combo-box6.p, line cbType1:HANDLE:ADD-LAST ("A", "")., we reach a situation in which append is true (correct because we add an item using ADD), but newAddedItems is null (although we would expect to see the values that are being added). Now, if

newAddedItems is correctly null, then the answer to the question would be no, we won't be able to deduce the value of append.
newAddedItems is wrong and should not be null, then it is possible to provide a fix for that. From what I currently observe, a possible solution would be to go through all the places in which a method that contains newAddedItems is called, and then try to create a List<ControlSetItem> that will be passed as an argument. It looks like this is possible (creating the list) from all the places, but I haven't 100% checked.

Hynek/Greg: let me know if this is worth spending time on.

#16 Updated by Hynek Cihlar about 1 month ago

Radu Apetrii wrote:

Hynek Cihlar wrote:

newAddedItems is correctly null, then the answer to the question would be no, we won't be able to deduce the value of append.

newAddedItems is wrong and should not be null, then it is possible to provide a fix for that. From what I currently observe, a possible solution would be to go through all the places in which a method that contains newAddedItems is called, and then try to create a List<ControlSetItem> that will be passed as an argument. It looks like this is possible (creating the list) from all the places, but I haven't 100% checked.

If newAddedItems is correctly null then there is no way, just add more clarification to javadoc why new parameter is needed.

If newAddedItems is incorrectly null then it would be better to have this fixed. But only if this is a simple fix without significant risk for regressions. If the change would be too much effort, let's just add more clarification documentation in the code and leave it as is.

#17 Updated by Radu Apetrii about 1 month ago

Hynek Cihlar wrote:

Radu Apetrii wrote:

Hynek Cihlar wrote:

newAddedItems is correctly null, then the answer to the question would be no, we won't be able to deduce the value of append.

newAddedItems is wrong and should not be null, then it is possible to provide a fix for that. From what I currently observe, a possible solution would be to go through all the places in which a method that contains newAddedItems is called, and then try to create a List<ControlSetItem> that will be passed as an argument. It looks like this is possible (creating the list) from all the places, but I haven't 100% checked.

If newAddedItems is correctly null then there is no way, just add more clarification to javadoc why new parameter is needed.

If newAddedItems is incorrectly null then it would be better to have this fixed. But only if this is a simple fix without significant risk for regressions. If the change would be too much effort, let's just add more clarification documentation in the code and leave it as is.

Do you have another suggestion other than seeing who added the newAddedItems code and checking the purpose of the change? Thank you!

#18 Updated by Hynek Cihlar about 1 month ago

Radu Apetrii wrote:

Do you have another suggestion other than seeing who added the newAddedItems code and checking the purpose of the change? Thank you!

Yes, checking all the call-sites of setItems (there is 4 of them) and following the logic of the newAddedItems arguments.

#19 Updated by Radu Apetrii about 1 month ago

I've followed the leads and reached multiple roots of setting newAddedItems. All of them pass as a parameter the value null, with one exception: ControlSetEntity.addLast(String itemList) which provides a proper list. However (more of a side note), in the tests that I have, the program sets the combo-box items via ControlSetEntity.addLast with two parameters (so not the function with only one), which uses a different type of Expander, resulting in passing the null value as well. So this means that, at least in my tests, newAddedItems will always be null. "Fixing" this part of code seems quite challenging from my point of view.

Also, one more thing, now that I've checked the changes again. Assume that newAddedItems works as we expect. When we use the delete() function to remove some items from the combo-box, logically, newAddedItems should not contain any items (I believe) as we are performing a delete operation, but append needs to be true in order to make things work. This means that even if we manage to fix newAddedItems, there is still at least one case which highlights a difference between newAddedItems and append.

My opinion is that we should stick with append (a.k.a. not removing it), but I am open to other points of view.

#20 Updated by Hynek Cihlar about 1 month ago

Status changed from Review to Internal Test

Radu Apetrii wrote:

My opinion is that we should stick with append (a.k.a. not removing it), but I am open to other points of view.

OK, let's keep the new parameter. Did you finish regression testing?

#21 Updated by Radu Apetrii about 1 month ago

Hynek Cihlar wrote:

Radu Apetrii wrote:

My opinion is that we should stick with append (a.k.a. not removing it), but I am open to other points of view.

OK, let's keep the new parameter. Did you finish regression testing?

Not yet. I'll update the task when this is done.

Also available in: Atom PDF

Project

General

Profile

FWD » Core Development » User Interface

Issues

Custom queries

Bug #8012

Fix combo-box corner cases

History

#1 Updated by Radu Apetrii 8 months ago

#4 Updated by Radu Apetrii 7 months ago

#5 Updated by Radu Apetrii 7 months ago

#7 Updated by Radu Apetrii about 1 month ago

#8 Updated by Radu Apetrii about 1 month ago

#9 Updated by Greg Shah about 1 month ago

#10 Updated by Radu Apetrii about 1 month ago

#11 Updated by Greg Shah about 1 month ago

#12 Updated by Hynek Cihlar about 1 month ago

#13 Updated by Alexandru Lungu about 1 month ago

#14 Updated by Marian Edu about 1 month ago

#15 Updated by Radu Apetrii about 1 month ago

#16 Updated by Hynek Cihlar about 1 month ago

#17 Updated by Radu Apetrii about 1 month ago

#18 Updated by Hynek Cihlar about 1 month ago

#19 Updated by Radu Apetrii about 1 month ago

#20 Updated by Hynek Cihlar about 1 month ago

#21 Updated by Radu Apetrii about 1 month ago