FWD » Core Development » User Interface

This method is available on selection list, combo-box, radio-set and browse column. Follow the same approach as for #2494.

For COMBO-BOX and SELECTION-LIST:
1. If LIST-ITEM-PAIRS is set then ADD-LAST(items) is rejected with two warnings (4064 and 4065)
2. If LIST-ITEM is SET then ADD-LAST(item) is accepted with and ADD-LAST(label, value) generates a warning (8590) and adds label
3. The added items will be last only for if SORT = FALSE

The RADIO-SET widget has only ADD-LAST(label, value) method.
It just adds the new label/value to the widget.
If AUTO-RESIZE attribute of a widget is TRUE and after ADD-LAST() the widget size exceeds the enclosing frame size a warning (4474) is generated (but method returns TRUE). After that the behaviour becomes a little bit strange:
1. One can delete the added label
2. If some other labels are deleted the added label becomes visible, but not selectable.

If AUTO-RESIZE attribute of a widget is FALSE and after ADD-LAST() the widget limit no warning is generated and method returns TRUE. After that the widget behaves as described above.

Greg,
1. Can I start Java runtime implementation?
2. Should I try to implement the described "strange" behaviour if the runtime doesn't already behave like this?

Please check how this works with parameters that are unknown value and empty string.

Please also check how it works when a duplicate label or duplicate value (one that already exists in the list) is used as a parameter.

Should I try to implement the described "strange" behaviour if the runtime doesn't already behave like this?

Not at this time. See if you can find the right place for a TODO comment in the code where this condition would need to be detected and implemented. I suspect that is on the client side somewhere.

Greg Shah wrote:

Please check how this works with parameters that are unknown value and empty string.

Please also check how it works when a duplicate label or duplicate value (one that already exists in the list) is used as a parameter.

Both duplicate and unknown labels/values can be added w/o a complain.
If one deletes duplicate/unknown item by value the fist one is deleted.

Do you have testcases for the errors and edge cases?

Please collect all of your testcases and check them into the testcases project in a testcases/uast/list_widgets/ directory. As long as they are checked in there, you don't need to post them here or in #2494.

Go ahead with the implementation.

I have a question.
As far as I understand there is no "null" value in ABL - it has "unknown" (literal ?) instead. If this is correct can I treat null argument as "unknown" (at least for ADD-LAST() method)?

Yes and yes.

If "unknown value is specified for ADD-LAST(items) method it is accepted but on the screen a blank line is shown (instead of "?"). I've implemented it, but noticed that the runtime implementation of the ADD-FIRST() method rejects "unknown" and (as far as I can see from the code) even if it was accepted the "?" will be seen on the screen (in 4GL ADD-FIRST behaviour is essentially the same as ADD-LAST one (except the place where items are added).
May be it makes sense to fix the implementation of ADD-FIRST as well (the current implementation also doesn't show warnings)?

May be it makes sense to fix the implementation of ADD-FIRST as well (the current implementation also doesn't show warnings)?

Yes, please fix it to be compatible.

Additional details about COMBO-BOX widget behaviour in presence of duplicated/"unknown" labels/values:
1. empty label/value is equivalent to the unknown one.
2. If label empty/unknown label is selected the SCREEN-VALUE is always ?
3. The item with empty label can still be removed be value (either empty/unknown or not)
4. If duplicated labels with different values are present and different items (with same labels) are selected the SCREEN-VALUE will be different

It is easy to implement section 2 (e.g by adding realValue field to ControlSetItem in addition to value - for ControlSetItem with empty/unknown label the value will be set to "unknown", the realValue will be used for DELETE(item) method).

However it seems that to provide a correct SCREEN-VALUE in case of duplicated labels with different values a more complicated changes are required. It this moment in such a case GenericFrame.getSelection() and GenericFrame.getSelectionValue() methods return index/value for the first found item with a selected label which this different from the 4GL behaviour. As far as I understand changes are required in the LogicalTerminal and/or at the client side.

I've also notices another discrepancy between 4GL and Java runtime COMBO-BOX behaviour. In 4GL after ADD-FIRST the value should in the combo box doesn't change, however with Java runtime a newly added value is shown.

Should I start working on the issues mentioned above?

Good work in finding these issues.

Should I start working on the issues mentioned above?

Yes, please do.

Yes, please fix it.

Just noticed that ListSelectionModel implementation should be revised - currently it behaves incorrectly after ADD-FIRST - must be based on the recently introduced item ids instead of indexes.

Regression test performed twice.
The following steps failed identically in both runs:

4 ctrlc_11_session1 CTRL-C 11 (Session 1) - CL - GSO - RFQ. FAILED CONCURRENT BACKOUT Driver 0 318.465                                                                                                    
failure in step 18: 'Timeout while waiting for event semaphore to be posted.'                                                                                                                                
   5 ctrlc_11_session3 CTRL-C 11 (Session 3) - CL - GSO - RFQ. FAILED CONCURRENT BACKOUT Driver 1 218.621                                                                                                    
failure in step 13: 'timeout before the specific screen buffer became available (Mismatched data at line 1, column 0. Expected '┌' (0x250C at relative Y 1, relative X 0) and found 'i' (0x0069 at relative Y
 1, relative X 0).)'                                                                                                                                                                                  6 ctrlc_11_session4 CTRL-C 11 (Session 4) - CL - GSO - RFQ. FAILED CONCURRENT BACKOUT Driver 2 300.000                                                                                                    
failure in step 1: 'Timeout while waiting for event semaphore to be posted.'                                                                                                                                 
   391 tc_dc_slot_026 TIMCO TC-DC-SLOT-026 testcase. FAILED NONE BACKOUT Driver 8 181.520                                                                                                                    
failure in step 8: 'timeout before the specific screen buffer became available (Mismatched data at line 4, column 9. Expected '' (0x0000 at relative Y 4, relative X 9) and found '0' (0x0030 at relative Y 4
, relative X 9).)'                                                                                                                                                                                  392 tc_dc_slot_027 TIMCO TC-DC-SLOT-027 testcase. FAILED NONE BACKOUT Driver 10 182.422                                                                                                                   
failure in step 8: 'timeout before the specific screen buffer became available (Mismatched data at line 4, column 9. Expected '' (0x0000 at relative Y 4, relative X 9) and found '0' (0x0030 at relative Y 4
, relative X 9).)'                                                                                                                                                                                 396 tc_job_002 TIMCO TC-JOB-002 testcase. FAILED NONE BACKOUT Driver 0 86.066                                                                                                                             
failure in step 40: 'Unexpected EOF in actual at page # 1.'                                                                                                                                                  
   400 tc_job_clock_004 TIMCO TC-JOB-CLOCK-004 testcase. FAILED NONE BACKOUT Driver 5 254.314                                                                                                                
failure in step 35: 'Timeout while waiting for event semaphore to be posted.'

All failures seem to be unrelated to my changes (no corresponding controls)

I will do the code review next. Sorry that I have been slow to respond. I've just been very busy.

In regard to the conversion problems, it is time for you to start working on changes in that area. Clearly, there are fixes to be made. You can make them as part of this work.

Caused by: com.goldencode.p2j.pattern.CommonAstSupport$UserGeneratedException: Unsupported method or attribute KW_HORIZ. [COLON id <146028888862> 0:0]

This means we are missing conversion support for the HORIZONTAL attribute.

You can add method and/or attribute conversion support by:

1. Find the correct interface or create a new one in which it should be included. For attrs/meths that are widely shared across all or nearly all widgets, we use CommonWidget. But for things implemented in only a small set of widgets, we create an interface that is implemented only by those widgets. We try to make that interface something that is meaningfully named and which maximizes the number of attributes and methods that can be placed there. See ScrollbarHorizontalElement as an example. Make sure to handle all the possible parameter variants needed to handle both the wrapper types (e.g. character) and their Java native literal types (e.g. String).

2. Please note that each method or attribute added to the above interface (whether new or existing) needs a matching LegacyMethod or LegacyAttribute annotation, so that we can do some automatic processing at runtime regarding the behavior of those attributes. You should see examples of these in ScrollbarHorizontalElement.

3. Determine the place or places in the widget hierarchy that need to implement that interface and place stubs in those classes and in any subclass that likewise need overrides. By stubs, I mean an set of "empty" methods that implement the interface without any functionality. It should allow P2J to properly compile AND any converted code (with the new attrs/meths) should be able to compile against it.

4. If you add a new interface (as opposed to just extending one that already exists), edit util/handle.java to add an unwrap method. Search on ScrollbarHorizontalElement to see what I mean.

5. If you add a new interface (as opposed to just extending one that already exists), edit util/HandleCommon.java to add the interface to the list.

6. Edit rules/convert/methods_attributes.rules to add definitions for the methods and/or attributes you are adding. For attrs/meths that are not defined in system handles (e.g. NOT in something like SESSION), please edit the load_descriptors function which makes the addition pretty easy. Just follow the comments and headings in that function. Your changes would all go in this section. For system handles we still implement more verbose code in the main portion of the rules.

7. If you are adding any writable attributes, you must add their keywords to the list in function read_only_attribute in rules/include/common-progress.rules.

First of all the handle class doesn't have unwrapControlEntity() method.

The above notes hopefully shed light on why these failures occurred.

Secondly CommonWidget (a result of handle.unwrapWidget() doesn' have addLast(String, String) method - in a correct signature should be addLast(character, character)

We normally add all possible combinations of addLast: (S, S), (S, c), (c, S), (c, c). That way we don't have to do any unnecessary "wrapping" of string literals into character values.

Got it. Actually I've already stated to look at the conversion details.
I've added unwrapConrolEntity() method to the handle class and addLast() to the CommonWidget interface and GenericWidget class. After that the converted radio2.p (without HORIZONTAL) compiles and even running, but in a strange way - the RADIO-SET widget is invisible, but working. After your comments I understand that it was a very naive approach. I've also notices that all handle.unwrapXXX() methods work with interfaces while ControlEntity is a class. I don't understand all the details but based on my experience it is not good (at least if dynamic proxies are used). Does it makes sense to extract interface?

I've also notices that all handle.unwrapXXX() methods work with interfaces while ControlEntity is a class. I don't understand all the details but based on my experience it is not good (at least if dynamic proxies are used).

Yes, these must be interfaces because we do use dynamic proxies. It also allows us to implement the interface in multiple places in the widget hierarchy instead of implementing at the top and having to write extra "this is undefined" methods to disable the function in places it should not be.

Does it makes sense to extract interface?

Anytime there is a small sub-set of widgets with some functionality it generally makes sense to separate it. You should probably move related methods/attrs with it. For example, ADD-FIRST and so forth seem to be highly related.

Got it. Thank you.

Code Review ias_upd20150205a.zip

I really like your nicely factored ControlSetEntity add*() implementation. I also like how you refactored processKey(). Some things to work on:

1. The new methods added to ComboBoxModel should follow our getter/setter "bean" standard. I realize that whomever added selected did not follow this rule, but I don't want to extend that. So, selectedIndex() should be getSelectedIndex() and index() should be setSelectedIndex(). The same changes apply to id, value...

2. Is there any way to introduce problems in ComboBoxModel.calcSelection() by relying upon the "usually" case-insensitive behavior of the 4GL?

3. ControlSetEntity.add() (both versions), ControlSetEntity.ItemsListExpander, ControlSetEntity.ItemsListByOneExpander, ControlSetEntity.ItemsListExpander.addItems(), ControlSetEntity.ItemsListByOneExpander.addItems(), and both ControlSetEntity.controlSetItem() methods need javadoc.

4. Line 348 of ControlSetEntity is too long.

5. Please order the ControlSetEntity features you've added by access modifier (protected, private...).

6. The item ID generator should not be in the ControlSetConfig class. We are trying to eliminate all logic/methods from the config classes.

7. I don't think the copying of heightChars should be in ControlSetConfig.applyConfig(). Please discuss this with Hynek.

8. I don't understand the purpose of RadioSet.getScreenValue().

9. Please insert a blank line between all data members and methods. This helps with readability. For example, in ControlSetItem, this:

   /** Item id - required for correct restore of the selection after list modification. */
   private int id;
   /** Item label. */
   private character label = null;

would be this:

   /** Item id - required for correct restore of the selection after list modification. */
   private int id;

   /** Item label. */
   private character label = null;

The only caveat to this is that the member or method just after the class opening { should NOT have a preceding blank line and the member or method just before the class closing } should NOT have a following blank line.

10. There are some coding standard issues with if in all of your files. Instead of things like if( frameConfig.box) {, it should be:

if (frameConfig.box)
{

11. I would also prefer you to put some blank lines into your core code, where it may enhance readability. RadioSetWidget.addLast() is a good example. I would have formatted it like this:

      ControlSetItem[] items = config.items;

      if (items == null)
         items = new ControlSetItem[0];

      boolean selectable = config.autoResize || (items.length < config.heightChars);
      FrameConfig frameConfig = frame.getFrameWidget().getConfig();
      int maxSize = (int)frameConfig.heightChars;

      if (frameConfig.box)
      {
         maxSize -= 2;
      }

      if (maxSize <= items.length)
      {
         ErrorManager.recordOrShowError(4474, String.format(
               "New height for %s %s is too large to fit in frame.", type(),
                  frame.getName(this)), false);
         selectable = false;
      }

      ControlSetItem[] nitems = Arrays.copyOf(items, items.length + 1);
      ControlSetItem   item   = controlSetItem(label, value);

      item.setSelectable(selectable);
      nitems[items.length] = item;
      setItems(nitems);

      return new logical(true);

I don't expect that all code you write will exactly follow my personal sensibilities. :) But I would ask that you use spacing to separate control flow sections, variable defs/assignments and general logic.

12. There is an extra history entry at the top of ComboBox.java, DefaultList.

13. RadioButton is missing a history entry.

14. Please reorder the newly protected methods in DefaultList to be above the private methods.

15. The SelectionListBody.changeRow() and several of the new features in ControlSetEntity have their method opening { that needs to be on the next line.

Greg Shah wrote:

2. Is there any way to introduce problems in ComboBoxModel.calcSelection() by relying upon the "usually" case-insensitive behaviour of the 4GL?

I will double check if 4GL really treats values in a case-insensitive way. If this is the fact I'll change this. The values' comparison is used now only if selected item is deleted.

6. The item ID generator should not be in the ControlSetConfig class. We are trying to eliminate all logic/methods from the config classes.

The item ID should be unique per config - this is what it was introduced for. It is possible of course to use some global id generator (based on UUID or global long counter), but the current version is the most efficient in terms of memory (and network traffic) consumption.

7. I don't think the copying of heightChars should be in ControlSetConfig.applyConfig(). Please discuss this with Hynek.

OK, I will ask him.

8. I don't understand the purpose of RadioSet.getScreenValue().

It was present before. I understand it supports SCREEN-VALUE attribute. I will double check.
I will fix other mentioned issues at the weekend. Sorry - I used to work with a relaxed formatting requirement before. I'll do my best to avoid these problems in the future.

6. The item ID generator should not be in the ControlSetConfig class. We are trying to eliminate all logic/methods from the config classes.

The item ID should be unique per config - this is what it was introduced for. It is possible of course to use some global id generator (based on UUID or global long counter), but the current version is the most efficient in terms of memory (and network traffic) consumption.

I'm not suggesting that we remove the id in the config. We can always have an external implementation that modifies the ID that resides in the config. That way there is a config-unique value that can be used without any network trips. Since the method is only used in ControlSetEntity, just move it there and directly increment the config.itemId.

I will fix other mentioned issues at the weekend. Sorry - I used to work with a relaxed formatting requirement before. I'll do my best to avoid these problems in the future.

I understand.

A more thorough analysis revealed the following details about the list widgets' behaviour:
1. For COMBO-BOX: the DELETE(items) method deletes items by comparing provided value(s) with items' labels (case-insensitive). However if a selected item is deleted a new selection is found by value (case-insensitive).
2. For SELECTION-LIST : the DELETE(items) method deletes items by comparing provided value(s) with items' values (case-insensitive)
2. For RADIO-SET : the DELETE(items) method deletes items by comparing provided value with items' labels (case-sensitive). Please note however that if one tries to delete an item with label which doesn't exists in a case-sensitive way but do exists in a case-insensitive sense the mpro crashes with SYSTEM ERROR: Memory violation. (49)

Please note however that if one tries to delete an item with label which doesn't exists in a case-sensitive way but do exists in a case-insensitive sense the mpro crashes with SYSTEM ERROR: Memory violation. (49)

The one we don't need to duplicate.

Please duplicate all of the rest. In RadioSet, make the comparison on a case-insensitive basis but put a comment that explains that on version 10.2B (or whatever version you tested on), that Progress "crashes with SYSTEM ERROR: Memory violation. (49)". Explain that we are not going to duplicate that behavior because working 4GL code cannot rely upon that while still successfully executing.

Code Review ias_upd20150209a.zip

1. Coding standard issues that remain:

• for( should be for ( - ComboBoxModel, RadioSet, SelectionListBody, ControlSetEntity
• if(expr should be if (expr - ComboBoxModel, ComboBox, ControlSetEntity
• if( expr) should be if (expr) - ComboBoxModel, ComboBox, SelectionListBody, RadioSetWidget
• Empty blank lines at the end of the class should be deleted. - ComboBoxModel
• Lines 712, 734, 756 and 858 of ControlSetEntity are too long.
• ItemsList*Expander inner classes of ControlSetEntity have { that need to be on their own line.
• ComboBoxWidget and SelectionListWidget each need a history entry.

2. Don't we still need addLast(character, String)?

3. The change in handle is incorrect. ControlEntity is not an interface and MUST NOT be an unwrap target. Are you planning to move the ADD-LAST, ADD-FIRST... into their own interface in a different update?

3. The change in handle is incorrect. ControlEntity is not an interface and MUST NOT be an unwrap target. Are you planning to move the ADD-LAST, ADD-FIRST... into their own interface in a different update?

It appears that the problem can be solved just by changing a single rule. However the converted radio2.p still exhibits strange runtime behaviour - the RADIO-SET widget is invisible but "working" (it fires triggers). Investigating.

Just by marking the method as "Widget" in methods_attributes.rules would work. It is not how we want it to work, but it would work. But all of the points in note 27 above need to be done to move things into their own new interface.

Perhaps that interface would be called ListManagement.

2. Don't we still need addLast(character, String)?

So far I do not see any need for it. There are a lot of other methods where nee signatures can be potentially added (e.g. addFirst()).

Are you saying that you cannot write code like this: my-widget:add-last(my-char-var, "Some Text").?

I want to add the cases now so that we don't have to add them later when we encounter new 4GL customer code that needs those signatures.

Greg Shah wrote:

Are you saying that you cannot write code like this: my-widget:add-last(my-char-var, "Some Text").?

I want to add the cases now so that we don't have to add them later when we encounter new 4GL customer code that needs those signatures.

Well, now I understand what you mean. I can of course add this method, but as I wrote before there is a number of other places which can cause similar issues. Don't you think that it is more practical to change the code generation? I mean that the code generator can validate the signature of the method it is about to emit and generate new character() if there is a method with a character argument but not with a String (or at least complain that there is no suitable method).

Just by marking the method as "Widget" in methods_attributes.rules would work. It is not how we want it to work, but it would work. But all of the points in note 27 above need to be done to move things into their own new interface.

I see. I will do it.

Don't you think that it is more practical to change the code generation? I mean that the code generator can validate the signature of the method it is about to emit and generate new character() if there is a method with a character argument but not with a String (or at least complain that there is no suitable method).

This is easy to do, but it has the unwanted result of making the generated code much more verbose. Unless the number of permutations are very large, we don't "wrap" literals. Instead we just create overloads for those cases.

A big objective for us is to make the converted code look very clean. If we expand the code a great deal over what it was in the 4GL, that is a big negative for customers. Right now, unfortunately, we do expand it somewhat. So we are always striving to find places where we can put more into the runtime and generate less code.

Greg Shah wrote:

This is easy to do, but it has the unwanted result of making the generated code much more verbose. Unless the number of permutations are very large, we don't "wrap" literals. Instead we just create overloads for those cases.

A big objective for us is to make the converted code look very clean. If we expand the code a great deal over what it was in the 4GL, that is a big negative for customers. Right now, unfortunately, we do expand it somewhat. So we are always striving to find places where we can put more into the runtime and generate less code.

I see. OK - i will add additional signatures.

I've extracted methods which are defines for lists widgets only to a separate interface. Should I remove these methods from CommonWidget/GenericWidget? Please note that in this case these methods should also be removed from BrowseColumnWidget but at this moment ComboBox in BROWSE is not supported by conversion anyway.

Should I remove these methods from CommonWidget/GenericWidget?

Yes.

Please note that in this case these methods should also be removed from BrowseColumnWidget

We really haven't come up with a proper solution for BrowseColumnWidget. The 4GL docs are quite unclear about this "widget". I understand that some of the attributes or methods executed on it are redirected to the contained widget. But in other cases, there are real attrs/meths of the column itself. I don't think we have determined which things go where and how to handle the delegated attrs/meths. For this reason, I believe our approach is pretty "jumbled" and may not make much sense.

If you believe that the moved attrs/meths are all delegated to the contained widget, then you can remove them from the BrowseColumnWidget. At some point we will have to detect when these are executed on a browse column and either do some additional unwrapping to get the contained widget OR we will have to implement these attrs/meths on BrowseColumnWidget and simply call the contained widget's associated method.

The situation with invisible dynamic RADIO-SET widget is still unclear. It appears that dynamic COMBO-BOX and SELECTION-LIST widgets exhibit the similar behavior - they are invisible but "working". Moreover when COMBO-BOX is activated the corresponding drop-down list is visible but at a wrong position.

Another finding: the conversion doesn't support RADIO-BUTTONS attribute in assignment (but supports is in VIEW-AS RADIO-SET phrase and wrongly accepts LIST-ITEM-PAIRS attribute in assignment)

One more question.
I've noticed that in some cases an invalid attribute result in 4GL compilation failure while in other situation (such as referring to LIST-ITEM-PAIRS for RADIO-SET) there is just a runtime warning. Do we want to support this difference (this will require an enormous testing)?

Igor Skornyakov wrote:

The situation with invisible dynamic RADIO-SET widget is still unclear. It appears that dynamic COMBO-BOX and SELECTION-LIST widgets exhibit the similar behavior - they are invisible but "working". Moreover when COMBO-BOX is activated the corresponding drop-down list is visible but at a wrong position.

This sounds like a problem that Constantin is currently working on. Don't do anything further on this.

Another finding: the conversion doesn't support RADIO-BUTTONS attribute in assignment (but supports is in VIEW-AS RADIO-SET phrase and wrongly accepts LIST-ITEM-PAIRS attribute in assignment)

This you can fix. Please post a simplified testcase (in tags) so that I can see what you are describing.

Igor Skornyakov wrote:

One more question.
I've noticed that in some cases an invalid attribute result in 4GL compilation failure while in other situation (such as referring to LIST-ITEM-PAIRS for RADIO-SET) there is just a runtime warning. Do we want to support this difference (this will require an enormous testing)?

We don't want to support any case where the code can never be valid or where we are mis-converting.

However, if there is code that may be valid depending on runtime state, then we must convert it in a way that will work or fail the same way as in the 4GL.

Greg Shah wrote:

We don't want to support any case where the code can never be valid or where we are mis-converting.

However, if there is code that may be valid depending on runtime state, then we must convert it in a way that will work or fail the same way as in the 4GL.

I have the following in mind. As you know I've moved some methods from CommonWdget/GenericWidget to a separate interface. This means that some programs which previously where successfully converted and event be able to run (with warning) may not covert/compile anymore (or fail).
For example if the program accesses dynamic widget's attribute which does not supported by this widget then (depending on rules) it will be either unwrapped to a wrong interface and a subsequent call will fail (I guess) or it will be unwrapped to e.g. CommonWidget and the compilation will fail.

Is it OK?

I have the following in mind. As you know I've moved some methods from CommonWdget/GenericWidget to a separate interface. This means that some programs which previously where successfully converted and event be able to run (with warning) may not covert/compile anymore (or fail).

Do they also fail in the 4GL? If not then it is probably a conversion issue we must fix.

For example if the program accesses dynamic widget's attribute which does not supported by this widget then (depending on rules) it will be either unwrapped to a wrong interface and a subsequent call will fail (I guess) or it will be unwrapped to e.g. CommonWidget and the compilation will fail.

Is it OK?

If it would fail at runtime in the 4GL, then it is necessary to fail the same way in P2J. If it would not compile in the 4GL, that is different. We don't provide support for that.

If there is any question about this, post the testcases here and we will discuss specifics.

Greg Shah wrote:

This you can fix. Please post a simplified testcase (in [...] tags) so that I can see what you are describing.

For example the following procedure

PROCEDURE P_Create_RS PRIVATE :
CREATE RADIO-SET h ASSIGN
HEIGHT-CHARS = 1
WIDTH-CHARS = 10.0
FRAME = FRAME b:HANDLE
AUTO-RESIZE = TRUE
LIST-ITEM-PAIRS ="J1,1,J2,2,J3,3"
.
END PROCEDURE.


which is illegal for 4GL can be converted and can run (in 4GL it will compile and run but with warning and with empty RADIO-SET. For the "right" version with RADIO-BUTTONS instead of LIST-ITEM-PAIRS the conversion fails.

which is illegal for 4GL

Do you mean that the code will not EXECUTE in the 4GL? It fails at compilation?

Igor Skornyakov wrote:

Greg Shah wrote:

This you can fix. Please post a simplified testcase (in [...] tags) so that I can see what you are describing.

For example the following procedure

PROCEDURE P_Create_RS PRIVATE :
CREATE RADIO-SET h ASSIGN
HEIGHT-CHARS = 1
WIDTH-CHARS = 10.0
FRAME = FRAME b:HANDLE
AUTO-RESIZE = TRUE
LIST-ITEM-PAIRS ="J1,1,J2,2,J3,3"
.
END PROCEDURE.


which is illegal for 4GL can be converted and can run (in 4GL it will compile and run but with warning and with empty RADIO-SET. For the "right" version with RADIO-BUTTONS instead of LIST-ITEM-PAIRS the conversion fails.

Igor, as you are using a dynamic RADIO-SET and not a static RADIO-SET (created via the VIEW-AS RADIO-SET phrase), any 4GL known method/attribute can be called/set. Although the CREATE widget stmt may pose some restrictions (which I doubt).

Anyway, as you can't tell the resource referenced by a handle at conversion time, the runtime needs to take care of checking if the attribute/method exists for the referenced resource. If the attribute/method does not exist for the resource, P2J already manages this properly - see the handle.unwrapImpl and handle.invalidAttrAccessProxy which is called when the resource does not implement the expected interface.

Greg Shah wrote:

Do you mean that the code will not EXECUTE in the 4GL? It fails at compilation?

It will compile and execute, but a warning **LIST-ITEM-PAIRS is not a setable attribute for RADIO-SET widget. (4052) will be generated and empty RADIO-SET will be shown. The converted code runs w/o any complains (the RADIO-SET will not be visible but "working" as I wrote before)

Constantin Asofiei wrote:

Igor, as you are using a dynamic RADIO-SET and not a static RADIO-SET (created via the VIEW-AS RADIO-SET phrase), any 4GL known method/attribute can be called/set. Although the CREATE widget stmt may pose some restrictions (which I doubt).

Anyway, as you can't tell the resource referenced by a handle at conversion time, the runtime needs to take care of checking if the attribute/method exists for the referenced resource. If the attribute/method does not exist for the resource, P2J already manages this properly - see the handle.unwrapImpl and handle.invalidAttrAccessProxy which is called when the resource does not implement the expected interface.

I see. Thank you. I will take a closer look at the unwrapping. However if I understand you correctly it will be right to extract LIST-ITEM and LIST-ITEM-PAIRS attribute to a a separate interface which will be implemented by COMBO-BOX and SELECTION-LIST but not a RADIO-SET (and add support for the RADIO-BUTTONS/HORIZONTAL/EXPAND attributes to the RADIO-SET only) - I'm working on it now.

The converted code runs w/o any complains

The 4GL docs state that LIST-ITEM-PAIRS is valid for combo-box, selection list and browse column. If you have implemented support at the ControlSetEntity level, then any sub-classes that should not support a particular attribute or method must override the implementation with one that will generate the correct runtime error. The setter method can call handle.readOnlyError() and the getter can call handle.invalidAttribute().

It may be best to implement this in ControlSetEntity if only a small amount of the interface needs to be disabled in the sub-class. But if most of the interface needs to be disabled, then it is best to have 2 interfaces.

(the RADIO-SET will not be visible but "working" as I wrote before)

This is the part that Constantin is working on.

Code Review ias_upd20150211b.zip

1. This code in CommonWidget is incorrect:

   @LegacyMethod(name = "SELECT-FOCUSED-ROW")
   public void setAutoReturn(boolean autoReturn);

This method sets the AUTO-RETURN widget option. If we had this implemented as an attribute, it would also double as the setter and would need a LegacyAttribute annotation.

2. CommonWidget is missing setAutoResize(boolean).

3. We don't want any Hibernate dependencies in our UI code. Please remove the import org.hibernate.metamodel.source.binder.*; from ControlSetEntity.

4. In ControlSetEntity, the extends ControlEntity<T> implements CommonListWidget<T> should be split into 2 lines.

5. Why is ControlSetEntity.entry(int) needed? The long version should work for int too.

6. CommonListWidget should not include features that are specific to radio-set (and are in no other widgets). The RADIO-BUTTONS and EXPAND should be in their own RadioSetInterface. And the HORIZONTAL should only be radio-set and slider.

7. methods_attributes.rules, common-progress.rules, HandleCommon and BrowseColumnWidget all need a history entry.

Code Review ias_upd20150212a.zip

It is very good. Some minor things:

1. Add RadioSetInterface to HandleCommon.

2. Code standard problems:

RadioSet line 355
RadioSetWidget line 417

RadioSetWidget line 421:

         int maxSize = (int)(config.horizontal ? frameConfig.widthChars :frameConfig.heightChars);

Try this instead (line length + space after the colon char):

         int maxSize = (int)(config.horizontal ? frameConfig.widthChars
                                               : frameConfig.heightChars);

RadioSetWidget line 427, add a space after the <:

         overflow = maxSize <(config.horizontal ? width : height); 

Code Review ias_upd20150216a.zip

1. The RadioSetInterface header should be above the import and package statements.

2. The RadioSetInterface header should not have a JPRM column.

3. About the NPE in LogicalTerminal.pushScreenDefinition(), can you show the 4GL code that can recreate this?

The code seems very complicated, with many more frames than I would have expected to be used or needed. Did you get the core of this from some other example?

From just reading the code it is not clear where the problem is being caused. But the NPE is just a symptom of a larger issue. There should not need to be any null protection code in LogicalTerminal.pushScreenDefInt().

You said that this occurs when exiting the application. Look into why we are trying to push screen definitions when we are exiting the application code. We only push screen defs when a frame is newly created OR when a frame's state changes on the server side. But if we are exiting, why do we need to be changing state?

You can post the NPE stack trace here and that should explain what is being done that is requiring pushing the screen defs. Since this is during exit of the application, the frames may have already been destroyed/resources cleaned up before this call.

The sample code is a result of my test evolution - I've added more code while investigating the widget behavior. Please find the stack trace below.

[02/17/2015 19:25:51 MSK] (StandardServer.invoke:SEVERE) {00000001:00000007:bogus} Abnormal end!
java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at com.goldencode.p2j.util.Utils.invoke(Utils.java:1244)
at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1772)
at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1272)
at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:435)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:693)
at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.RuntimeException: Unresolvable remote export public abstract void com.goldencode.p2j.ui.ClientExports.removeWindow(int).
at com.goldencode.p2j.net.RemoteObject$RemoteAccess.obtainRoutingKey(RemoteObject.java:1531)
at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1415)
at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:97)
at com.sun.proxy.$Proxy1.removeWindow(Unknown Source)
at com.goldencode.p2j.ui.LogicalTerminal.deregisterWindow(LogicalTerminal.java:11977)
at com.goldencode.p2j.ui.WindowWidget.resourceDelete(WindowWidget.java:680)
at com.goldencode.p2j.util.HandleChain.delete(HandleChain.java:238)
at com.goldencode.p2j.util.ProcedureManager.deleteResources(ProcedureManager.java:2041)
at com.goldencode.p2j.util.ProcedureManager.access$3400(ProcedureManager.java:128)
at com.goldencode.p2j.util.ProcedureManager$WorkArea.scopeFinished(ProcedureManager.java:2239)
at com.goldencode.p2j.util.TransactionManager.processScopeNotifications(TransactionManager.java:5146)
at com.goldencode.p2j.util.TransactionManager.popScope(TransactionManager.java:2251)
at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6955)
at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:230)
at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:212)
at com.goldencode.testcases.list_widgets.W3.execute(W3.java:68)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at com.goldencode.p2j.util.Utils.invoke(Utils.java:1244)
at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1772)
at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1272)
at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:435)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:693)
at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NullPointerException
at com.goldencode.p2j.ui.LogicalTerminal.pushScreenDefInt(LogicalTerminal.java:10847)
at com.goldencode.p2j.ui.LogicalTerminal.pushScreenDefinition(LogicalTerminal.java:7991)
at com.goldencode.p2j.ui.GenericFrame.pushScreenDefinition(GenericFrame.java:7387)
at com.goldencode.p2j.ui.GenericFrame.pushScreenDefinition(GenericFrame.java:7318)
at com.goldencode.p2j.ui.GenericFrame.deleteDynamicWidget(GenericFrame.java:7758)
at com.goldencode.p2j.ui.GenericWidget.resourceDelete(GenericWidget.java:2960)
at com.goldencode.p2j.util.HandleChain.delete(HandleChain.java:238)
at com.goldencode.p2j.ui.GenericFrame.frameDelete(GenericFrame.java:7259)
at com.goldencode.p2j.ui.LogicalTerminal.deregisterFrameInt(LogicalTerminal.java:8811)
at com.goldencode.p2j.ui.LogicalTerminal.applyChanges(LogicalTerminal.java:9986)
at com.goldencode.p2j.net.Protocol.applyChanges(Protocol.java:309)
at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1161)
at com.goldencode.p2j.net.Queue.transact(Queue.java:585)
at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:223)
at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:163)
at com.goldencode.p2j.net.HighLevelObject.getKey(HighLevelObject.java:116)
at com.goldencode.p2j.net.RemoteObject$RemoteAccess.obtainRoutingKey(RemoteObject.java:1503)
at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1415)
at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:97)
at com.sun.proxy.$Proxy1.removeWindow(Unknown Source)
at com.goldencode.p2j.ui.LogicalTerminal.deregisterWindow(LogicalTerminal.java:11977)
at com.goldencode.p2j.ui.WindowWidget.resourceDelete(WindowWidget.java:680)
at com.goldencode.p2j.util.HandleChain.delete(HandleChain.java:238)
at com.goldencode.p2j.util.ProcedureManager.deleteResources(ProcedureManager.java:2041)
at com.goldencode.p2j.util.ProcedureManager.access$3400(ProcedureManager.java:128)
at com.goldencode.p2j.util.ProcedureManager$WorkArea.scopeFinished(ProcedureManager.java:2239)
at com.goldencode.p2j.util.TransactionManager.processScopeNotifications(TransactionManager.java:5146)
at com.goldencode.p2j.util.TransactionManager.popScope(TransactionManager.java:2251)
at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6955)
at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:230)
at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:212)
at com.goldencode.testcases.list_widgets.W3.execute(W3.java:68)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at com.goldencode.p2j.util.Utils.invoke(Utils.java:1244)
at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1772)
at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1272)
at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:435)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:693)
at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
at java.lang.Thread.run(Thread.java:745)

It seems that the ScrollContainer.ensureVisibility() method's implementation is not suitable for at least a RadioSet widget. Its it acceptable to add a separate implementation which will be used only when the argument is instance of the RadioSet?

Another note. The look of the RadioSet widget at runtime is different in 4GL and Java: in 4GL the focused item is highlighted by changing the background color of the all three "( )" (or "(X)") characters while in Java these chars are underscored which makes them poorly visible, especially when the widget is scrollable. Is it OK?

In regard to the NPE, I don't understand why we would be pushing screen definitions to the client during the processing that is associated with the applyChanges() that itself is being executed "inside" the return from the removeWindow() call to the client.

Constantin: this whole sequence looks wrong. Do you have any ideas?

Igor Skornyakov wrote:

It seems that the ScrollContainer.ensureVisibility() method's implementation is not suitable for at least a RadioSet widget. Its it acceptable to add a separate implementation which will be used only when the argument is instance of the RadioSet?

Please provide more specifics about what needs to be different.

Igor Skornyakov wrote:

Another note. The look of the RadioSet widget at runtime is different in 4GL and Java: in 4GL the focused item is highlighted by changing the background color of the all three "( )" (or "(X)") characters while in Java these chars are underscored which makes them poorly visible, especially when the widget is scrollable. Is it OK?

Generally, no it is not OK.

I do see that RadioButtonImpl.drawButton() does not honor the pf-color for the "X" or " " middle character. So that is possibly a bug. But the "(" and ")" seem to be honoring the pf-color. Before we decide to make changes, please check to see if this is caused by color configuration differences. Perhaps our defaults are not matching the 4GL defaults OR the 4GL test system has some customized protermcap values that cause the display to be different.

Greg Shah wrote:

Generally, no it is not OK.

I do see that RadioButtonImpl.drawButton() does not honor the pf-color for the "X" or " " middle character. So that is possibly a bug. But the "(" and ")" seem to be honoring the pf-color. Before we decide to make changes, please check to see if this is caused by color configuration differences. Perhaps our defaults are not matching the 4GL defaults OR the 4GL test system has some customized protermcap values that cause the display to be different.

OK - I will check it.

Greg Shah wrote:

Please provide more specifics about what needs to be different.

Well, first of all the whole implementation look a little bit strange for me. In fact there is a long if/elseif block where the direction and magnitude of the scroll is calculated (which is strange per se as the last argument of the ScrollEvent completely determines the operation, moreover the ScrollableContainer.processScroll() method honors only LEFT/RIGHT/UP and DOWN directions). Secondly the conditions that are checked are not mutually exclusive which makes the algorithm suspicious. In my tests this results in two scrolls in opposite direction just after two consecutive invocations of the method.
For a correct scrolling of the RadioSet one should take into account the position of the currently focused button and do not scroll if it is visible - these logic doesn't fit into the current algorithm even if we're leaving the above mentioned issues apart.

Igor Skornyakov wrote:

Greg Shah wrote:

Please provide more specifics about what needs to be different.

Well, first of all the whole implementation look a little bit strange for me. In fact there is a long if/elseif block where the direction and magnitude of the scroll is calculated (which is strange per se as the last argument of the ScrollEvent completely determines the operation, moreover the ScrollableContainer.processScroll() method honors only LEFT/RIGHT/UP and DOWN directions). Secondly the conditions that are checked are not mutually exclusive which makes the algorithm suspicious. In my tests this results in two scrolls in opposite direction just after two consecutive invocations of the method.
For a correct scrolling of the RadioSet one should take into account the position of the currently focused button and do not scroll if it is visible - these logic doesn't fit into the current algorithm even if we're leaving the above mentioned issues apart.

These are reasonable points. I agree that the implementation is strange. There is even a TODO that suggests the code will need changes to operate in GUI mode, so it is clear it is unfinished code.

Hynek has been working on scrolling improvements. I don't know if he has changes in that method. He also may have some thoughts and insight into a better approach.

I would prefer to resolve this by implementing a proper abstraction interface to allow widgets to provide feedback to the scroll container so that the scroll container can operate properly on all widget types without knowledge of any specific widget being hard coded.

I also would like to see the algorithm be both easier to understand and have a safer implementation. The point about mutual exclusion is an important one.

Before you propose changes in that area, let's hear what Hynek has to say.

I see. May I make a suggestion?
First of all I think that it is important for the scroll to be well-defined and idempotent. For the RadioSet widget the following abstraction seems to be adequate.
Let C, W, A be rectangles on the plane where C is container, W - a widget and A - a sub-rectangle of W with sides parallel to the coordinate axes. Then the objective of the scroll is to find a vector v of minimal length so that the shift of W by v results in the maximal square of intersection of A and C. The coordinates of the sub-rectangle A can be the widget callback Greg have mentioned. The default implementation will provide A == W. For the RadioSet A is the rectangle of the current button.

Igor Skornyakov wrote:

I see. May I make a suggestion?
First of all I think that it is important for the scroll to be well-defined and idempotent. For the RadioSet widget the following abstraction seems to be adequate.
Let C, W, A be rectangles on the plane where C is container, W - a widget and A - a sub-rectangle of W with sides parallel to the coordinate axes. Then the objective of the scroll is to find a vector v of minimal length so that the shift of W by v results in the maximal square of intersection of A and C. The coordinates of the sub-rectangle A can be the widget callback Greg have mentioned. The default implementation will provide A == W. For the RadioSet A is the rectangle of the current button.

I think the proposal makes sense. It may even have good results with widgets where A > W. I'm guessing that might happen with a ComboBox (the drop-down portion extends down).

My only concern is that the 4GL is rarely sensible. We may find "quirks" that we must match. But so long as we are compatible with the 4GL, I would be perfectly happy with this approach.

Hynek?

Igor Skornyakov wrote:

Greg Shah wrote:

Please provide more specifics about what needs to be different.

Well, first of all the whole implementation look a little bit strange for me. In fact there is a long if/elseif block where the direction and magnitude of the scroll is calculated (which is strange per se as the last argument of the ScrollEvent completely determines the operation, moreover the ScrollableContainer.processScroll() method honors only LEFT/RIGHT/UP and DOWN directions). Secondly the conditions that are checked are not mutually exclusive which makes the algorithm suspicious.

I am not sure why is the direction needed, I guess the idea was to provide scroll direction and absolute scroll value in separate fields. I think scroll offsets of the X and Y axis allowing negative values should be sufficient. Alternatively having scroll event without scroll offsets and ask scroll container for scroll positions. This would probably simplify cases with multiple levels of scroll containers. If offset values are left in the ScrollEvent class I would rather use a different type than Point, as this implies a location.

ScrollContainer.ensureVisibility will for sure need an overhaul I have also seen it firing scroll events with invalid scroll "limits".

Greg Shah wrote:

Igor Skornyakov wrote:

I see. May I make a suggestion?
First of all I think that it is important for the scroll to be well-defined and idempotent. For the RadioSet widget the following abstraction seems to be adequate.
Let C, W, A be rectangles on the plane where C is container, W - a widget and A - a sub-rectangle of W with sides parallel to the coordinate axes. Then the objective of the scroll is to find a vector v of minimal length so that the shift of W by v results in the maximal square of intersection of A and C. The coordinates of the sub-rectangle A can be the widget callback Greg have mentioned. The default implementation will provide A == W. For the RadioSet A is the rectangle of the current button.

I think the proposal makes sense. It may even have good results with widgets where A > W. I'm guessing that might happen with a ComboBox (the drop-down portion extends down).

My only concern is that the 4GL is rarely sensible. We may find "quirks" that we must match. But so long as we are compatible with the 4GL, I would be perfectly happy with this approach.

Hynek?

Igor's idea is fine I think.

Personally I would rather see the scroll event initiated by the widget receiving focus itself. That is, (1) combo box receives focus, (2) combo box asks for its position relative to visible area, (3) combo box calculates the scroll offset depending on its current state, (4) combo box sends scroll event to make itself "visible" (with sensible default behaviors implemented in the abstract classes). The advantages are that no new special API is needed to calculate the scroll offset by scroll container, no need to detect the state changes leading to focus change in the containers (just handle the updated focus and do not care how the focus got there), take advantage of the eventing mechanism itself like consuming events, etc and maybe more straightforward implementation for multiple levels of scroll containers.

Hynek Cihlar wrote:

Personally I would rather see the scroll event initiated by the widget receiving focus itself. That is, (1) combo box receives focus, (2) combo box asks for its position relative to visible area, (3) combo box calculates the scroll offset depending on its current state, (4) combo box sends scroll event to make itself "visible" (with sensible default behaviors implemented in the abstract classes). The advantages are that no new special API is needed to calculate the scroll offset by scroll container, no need to detect the state changes leading to focus change in the containers (just handle the updated focus and do not care how the focus got there), take advantage of the eventing mechanism itself like consuming events, etc and maybe more straightforward implementation for multiple levels of scroll containers.

I'm not sure that I understand this completely. However it seems that it means that a widget (e.g. combo box) should be aware that it is scrollable (and may be know some details about the enclosing container) which makes the scrolling logic spread across multiple widgets. What I'm suggesting is that the widget should simply report what part of it should be visible for as good as possible at any given moment.

Of course at this moment I have a limited vision of the overall picture and may overlook some important cases.

At the bottom line - what we're going to do right now? In particular should the scrolling fix be in the scope of the current task?

Igor Skornyakov wrote:

I'm not sure that I understand this completely. However it seems that it means that a widget (e.g. combo box) should be aware that it is scrollable (and may be know some details about the enclosing container) which makes the scrolling logic spread across multiple widgets. What I'm suggesting is that the widget should simply report what part of it should be visible for as good as possible at any given moment.

I think this is an implementation detail. The widget can be "decorated" with a scrolling behavior and the widget class itself (and its parents) don't need to have any knowledge about the scrolling details.

The point I am trying to make is that the template method approach may be a bit limiting. What if the behavior is more complex - for example the widget itself renders differently depending on the visible area size, or a message is posted to inform the user the widget won't fit, etc. That is why I like the idea of rather dummer scroll container providing basic services like position information and fulfilling scroll requests and keeping the logic closer to the individual components that may know better how they should behave.

This would also have the benefit of keeping each individual part (scroll container, widget, scrolling behavior) separate taking only the right amount of responsibility.

On the other hand, I admit that we may find out that no other special behavior than finding the widget visible area as good as possible at any given moment will be required and the template method will suffice.

At the bottom line - what we're going to do right now? In particular should the scrolling fix be in the scope of the current task?

I think it won't hurt if you start fixing the immediate pains - the unclear model of scroll event class, issues with the ensureVisibility method implementation and your proposed solution based on the template method pattern. We may always evolve the design if needed.

Hynek Cihlar wrote:

I think it won't hurt if you start fixing the immediate pains - the unclear model of scroll event class, issues with the ensureVisibility method implementation and your proposed solution based on the template method pattern. We may always evolve the design if needed

Thank you Hynek. I will start implementing my approach. I will test the ComboBox behavior and of course submit my version for a code review.

Constantin: please comment on note 72 and item 3 in note 85.

Igor: in regard to the scrolling problems, I'd like for you to finish the current task without adding more work to it. I don't want to lose these items, so we may have you create a new redmine task to track these.

You have also found an NPE that we don't want to forget.

What items are left open as required for this task?

Code Review ias_upd20150219a.zip

1. In RadioButtonImpl.draw(), why iterate through character by character instead of using the append(String, Color)?

2. In RadioSet.processKeyEvent(), which is the code for SE_ENTRY removed?

3. Please add javadoc to the new data members and methods of ScrollDirection.

Igor Skornyakov wrote:

3. After some recent changes (as far as I understand - introducing manageSystemActions() method) it becomes impossible to switch frames (SE_NEXT_FRAME/SE_PREV_FRAME keycodes are ignored). I can try to fix it as well. Shall I?

Please add Keyboard.SE_NEXT_FRAME and Keyboard.SE_PREV_FRAME cases to AbstractWidget.ignoreAction, after HELP and TAB are treated - it will fix the problem.

About the NPE in note 73 and the w3.p test. A widget can be determine as no longer alive (thus can be deleted) by both client and server sides:
1. on server-side, if the widget/frame is no longer visible, then static widgets will be deleted when it's instantiating-procedure is finished
2. on client-side, if it determines that a frame is now hidden and is no longer alive, it needs to inform the server-side that the frame was deleted from client-side (so server-side can cleanup after it, too).

I think the NPE is caused by the mixed usage of dynamic and static widgets. When a dynamic widget is deleted and is attached to a frame, it needs to be removed from the frame. But now the frame doesn't know that this deletion was decided by the client-side, so it tries to inform back the client-side to delete it, too.

What I think we need is a way to ignore LT.pushScreenDefinition if the caller is LT.applyChanges.

Greg Shah wrote:

Code Review ias_upd20150219a.zip

1. In RadioButtonImpl.draw(), why iterate through character by character instead of using the append(String, Color)?

The ChuiOutputManager sets the value of the cursorInvalid in the at method and doesn't reset it in the append(String). As the result the in the HORIZONTAL RADIO-SET the button was seen at the screen only when its start is visible while in 4GL this is not the fact - the button can be partially visible both at the beginning and at the end of the widget.

2. In RadioSet.processKeyEvent(), which is the code for SE_ENTRY removed?

Its my fault, sorry. I've restored it.

3. Please add javadoc to the new data members and methods of ScrollDirection.

Done.

Constantin Asofiei wrote:

Please add Keyboard.SE_NEXT_FRAME and Keyboard.SE_PREV_FRAME cases to AbstractWidget.ignoreAction, after HELP and TAB are treated - it will fix the problem.

Thank you Constantin - it works now.

The ChuiOutputManager sets the value of the cursorInvalid in the at method and doesn't reset it in the append(String). As the result the in the HORIZONTAL RADIO-SET the button was seen at the screen only when its start is visible while in 4GL this is not the fact - the button can be partially visible both at the beginning and at the end of the widget.

Please put a // comment in the code at that point to explain this. It is important that someone doesn't come along later and change this without knowing this information.

Code Review ias_upd20150220a.zip

The ScrollDirection constructor still needs Javadoc. Obviously, this doesn't invalidate any testing you do. So whatever passes testing can be checked in with the javadoc and other comment added.

There is about to be a check in from #1790. When that occurs, please merge up to the latest bzr revision and put that merged update back into testing. I agree that the problems are probably not due to your update but it is best to get a pretty clean run (or the combination of passing tests from multiple runs) before we accept the failures. Since there is a merge coming anyway, it is safest to re-run testing.

Code Review ias_upd20150220c.zip

It looks good.

There where 3 runs of the regression test.
The following test failed in in all 3 runs:

gso_tests
57 gso_187: failure in step 6: 'timeout before the specific screen buffer became available (Mismatched data at line 3, column 8. Expected ' ' (0x0020 at relative Y 3, relative X 8) and found '┌' (0x250C at relative Y 3, relative X 8).)'

63 gso_196: failure in step 4: 'timeout before the specific screen buffer became available (Mismatched data at line 3, column 8. Expected ' ' (0x0020 at relative Y 3, relative X 8) and found '┌' (0x250C at relative Y 3, relative X 8).)'

133 gso_281: failure in step 4: 'timeout before the specific screen buffer became available (Mismatched data at line 2, column 14. Expected '1' (0x0031 at relative Y 2, relative X 14) and found '2' (0x0032 at relative Y 2, relative X 14).)'

152 gso_307: failure in step 40: 'timeout before the specific screen buffer became available (Mismatched data at line 4, column 73. Expected '1' (0x0031 at relative Y 4, relative X 73) and found '0' (0x0030 at relative Y 4, relative X 73).)'

221 gso_394: failure in step 10: 'timeout before the specific screen buffer became available (Mismatched data at line 21, column 1. Expected '' (0x0000 at relative Y 21, relative X 1) and found 'E' (0x0045 at relative Y 21, relative X 1).)'

222 gso_395: failure in step 55: 'timeout before the specific screen buffer became available (Mismatched data at line 21, column 1. Expected '' (0x0000 at relative Y 21, relative X 1) and found 'E' (0x0045 at relative Y 21, relative X 1).)'

gc_tests
5 gc_62: failure in step 36: 'timeout before the specific screen buffer became available (Mismatched data at line 2, column 11. Expected '>' (0x003E at relative Y 2, relative X 11) and found ' ' (0x0020 at relative Y 2, relative X 11).)'

gso_rfq_tests
3 gso_301: failure in step 6: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 0. Expected '┌' (0x250C at relative Y 5, relative X 0) and found '' (0x0000 at relative Y 5, relative X 0).)'

4 gso_308: failure in step 69: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 0. Expected '┌' (0x250C at relative Y 5, relative X 0) and found '' (0x0000 at relative Y 5, relative X 0).)'

5 gso_333: failure in step 1: 'Timeout while waiting for event semaphore to be posted.'

6 gso_334: failure in step 1: 'Timeout while waiting for event semaphore to be posted.'

7 gso_343: failure in step 1: 'Timeout while waiting for event semaphore to be posted.'

tc_tests
67 tc_inquiry_inventory_017: failure in step 19: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 59. Expected 'H' (0x0048 at relative Y 5, relative X 59) and found 'A' (0x0041 at relative Y 5, relative X 59).)'

396 tc_job_002: failure in step 40: 'Unexpected EOF in actual at page # 1.'

505 tc_po_012: failure in step 28: 'timeout before the specific screen buffer became available (Mismatched data at line 21, column 1. Expected '' (0x0000 at relative Y 21, relative X 1) and found 'E' (0x0045 at relative Y 21, relative X 1).)'

gso_489_gc_63-64_tests
3 gc_63: failure in step 51: 'timeout before the specific screen buffer became available (Mismatched data at line 6, column 28. Expected '>' (0x003E at relative Y 6, relative X 28) and found ' ' (0x0020 at relative Y 6, relative X 28).)'

4 gc_64: failure in step 17: 'timeout before the specific screen buffer became available (Mismatched data at line 6, column 15. Expected 'C' (0x0043 at relative Y 6, relative X 15) and found '' (0x0000 at relative Y 6, relative X 15).)'

Only in gso_187 and gso_196 the mismatched screen contained RADIO-SET (actually it was the same screen - "CONTACT MASTER"). As far as I understand the problem was in the border position (top left corner) which seems to be unrelated to my changes.

Greg,
Can I commit my changes?
Thank you.

Although it seems unlikely that the problems are caused by your update, it is very unusual for the same list of tests to keep failing. Usually, the failures are smaller in number and vary, such that across multiple runs the results show that all tests pass.

Please watch devsrv01 for a period of lower utilization and run testing again then. If the same tests fail, then I would expect that there may be something in your update that is actually the cause.

Greg Shah wrote:

Although it seems unlikely that the problems are caused by your update, it is very unusual for the same list of tests to keep failing. Usually, the failures are smaller in number and vary, such that across multiple runs the results show that all tests pass.

Please watch devsrv01 for a period of lower utilization and run testing again then. If the same tests fail, then I would expect that there may be something in your update that is actually the cause.

OK, I will restart the test tomorrow morning (Moscow time) - typically the devsrv01 is not heavy loaded at this time. I a meantime I've started the regression w/o my changes. Please note however that the last 3 runs where on the weekend. The only reason I can imagine why my changes broke the test is that now I initialize HEIGHT_CHARS and WIDTH_CHARS (they where always zero before). This can be checked by adding new attributes for my purposes and re-testing.

Adding separate height/width fields doesn't help. Starting a detailed investigation. In particular I can see the the RADIO-SET widget on gso_187 is wider than expected.

OK, this makes sense. As you move forward, do keep in mind that you will likely need to merge your code up to newer versions. I'm going to allow other conflicting changes to go forward ahead of yours while you debug the problem.

Please do post any specific questions here and we will try to help.

Greg Shah wrote:

OK, this makes sense. As you move forward, do keep in mind that you will likely need to merge your code up to newer versions. I'm going to allow other conflicting changes to go forward ahead of yours while you debug the problem.

I understand. Actually I've already had to perform merges so it is not a problem.

Please do post any specific questions here and we will try to help.

Thank you. Actually it seems that I've localized at least one issue - it appears that converted syman/utils/contact.p (slightly simplified) provides a standalone example of the obvious misbehavior with my changes.

After fixes only the following tests failed in two runs:

3 gso_301: failure in step 6: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 0. Expected '┌' (0x250C at relative Y 5, relative X 0) and found '' (0x0000 at relative Y 5, relative X 0).)'

4 gso_308: failure in step 69: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 0. Expected '┌' (0x250C at relative Y 5, relative X 0) and found '' (0x0000 at relative Y 5, relative X 0).)'

5 gso_333: failure in step 1: 'Timeout while waiting for event semaphore to be posted.'

6 gso_334: failure in step 1: 'Timeout while waiting for event semaphore to be posted.'

7 gso_343: failure in step 1: 'Timeout while waiting for event semaphore to be posted.'

396 tc_job_002: failure in step 40: 'Unexpected EOF in actual at page # 1.' (expected)

Continue investigation of the gso_rfq_tests test plan

Both gsO301 and gso_308 failed at the same screen. As far as I understand the code for this screen is in syman/so/serv21.p.
The screen looks corrupted:

02/27/2015                      RFQ Customer Cards                      07:56:24
┌──────────────────────────────────────────────────────────────────────────────┐

confidential information redacted

└──────────────────────────────────────────────────────────────────────────────┘

┌──────────────────────────────────────────────────────────────────────────────┐
│  Sel W.B.C.   Oper Card Num             Description                          │
│  ────────────────────────────────────────────────────────────────────────────│
│ >                                                                            │
│                                                                              │
│                                                                              │
│                                                                              │
│                                                                              │
│                                                                              │
│                                [ Card #  ]                                   │
│                                                                              │
│                                                                              │
│                                                                              │
<G> Generate cards from existing Service Order/RFQ                             ┘

Should be:

02/23/2015                      RFQ Customer Cards                      15:00:57
┌──────────────────────────────────────────────────────────────────────────────┐

confidential information redacted

└──────────────────────────────────────────────────────────────────────────────┘
┌──────────────────────────────────────────────────────────────────────────────┐
│  Sel W.B.C.   Oper Card Num             Description                          │
│  ────────────────────────────────────────────────────────────────────────────│
│ >                                                                            │
│                                                                              │
│                                                                              │
│                                                                              │
│                                                                              │
│                                                                              │
│                                                                              │
│                                                                              │
│                                                                              │
│                                                                              │
│                                                                              │
└──────────────────────────────────────────────────────────────────────────────┘
<G> <B> <A> <D> <F> <U> <O> <C> <P> <N> <E> <S> <R> <K> <H> <M> <I>

<G> Generate cards from existing Service Order/RFQ

first of all there is an extra space between the header frame and the one which is below it. As a result the line <G> <B> <A> <D> <F> <U> <O> <C> <P> <N> <E> <S> <R> <K> <H> <M> <I> is not visible.

Secondly there is [ Card #] inside the lower frame which is created in syman/util/serv21-co-p which includes syman/util/GCnumbrws.i. I do not see any of list items in the code and do not understand how to debug this part. Can you please suggest how to deal with this problem?
Thank you.

Has anyone seen this kind of failure in the RFQ tests? Could we have checked something in that has broken RFQ?

Greg Shah wrote:

Has anyone seen this kind of failure in the RFQ tests? Could we have checked something in that has broken RFQ?

I don't think this is a regression in the latest bzr... my last run (from today) had no error in this.

But what I see when executing this manually is a "hickup" which shows the main (larger) frame one row lower and then it moves it one row upper. I think it might be related to devsrv01 load... it captured the screen at the wrong time.

Igor: do you have any other problems to solve or is this it?

Constantin, Hynek, Eric and I all have quite a few changes to try to get through testing. These are high priority. Please hold off on additional testing until we clear these. I hope when we are through, that will reduce the load for you and you can get a clean run.

Greg Shah wrote:

Igor: do you have any other problems to solve or is this it?

No. I do not have any new problems with this task.

Constantin, Hynek, Eric and I all have quite a few changes to try to get through testing. These are high priority. Please hold off on additional testing until we clear these. I hope when we are through, that will reduce the load for you and you can get a clean run.

OK. I have the regression test running now. Should I stop it?

No, let it finish.

Constantin Asofiei wrote:

Greg Shah wrote:

Has anyone seen this kind of failure in the RFQ tests? Could we have checked something in that has broken RFQ?

I don't think this is a regression in the latest bzr... my last run (from today) had no error in this.

I agree.

But what I see when executing this manually is a "hickup" which shows the main (larger) frame one row lower and then it moves it one row upper. I think it might be related to devsrv01 load... it captured the screen at the wrong time.

I've added a delay to gso_301 (more presisely, to common/find_rfq_so_8000356.xml), but the result is the same. Continue investigation.

Igor Skornyakov wrote:

I've added a delay to gso_301 (more presisely, to common/find_rfq_so_8000356.xml), but the result is the same. Continue investigation.

Igor if haven't already, try to run the test manually. That is, start the client, login, and follow the test steps.

Hynek Cihlar wrote:

Igor if haven't already, try to run the test manually. That is, start the client, login, and follow the test steps.

Thank you Hunek. I've performed the manual test. It also failed but the screen looks more informative than in automatic test's log:

03/02/2015                      RFQ Customer Cards                      11:03:46
┌──────────────────────────────────────────────────────────────────────────────┐
│        Order: 8000356          Customer: confidential info redacted          │
│Aircraft Type: B767            Work-type: HMV                                 │
└──────────────────────────────────────────────────────────────────────────────┘

                                                               ┌──────────────────────────────────────────────────────────────────────────────┐
                                                               │  Sel W.B.C.   Oper Card Num             Description                          │
                                                               │  ────────────────────────────────────────────────────────────────────────────│
                                                               │ >                                                                            │
                                                               │                                                                              │
                                                               │                                                                              │
                                                               │                                                                              │
                                                               │                                                                              │
                                                               │                                                                              │
                                 [ Card #  ]                   │                                                                              │
                                                               │                                                                              │
                                                               │                                                                              │
                                                               │                                                                              │
                                                               │                                                                              │
                                                               └──────────────────────────────────────────────────────────────────────────────┘
                                                               <G> <B> <A> <D> <F> <U> <O> <C> <P> <N> <E> <S> <R> <K> <H> <M> <I>

Trying to understand what exactly was broken.

Igor, is your terminal set to 80 cols by 24 rows? MAJIC layout will not be correct if the terminal is not set like this.

Constantin Asofiei wrote:

Igor, is your terminal set to 80 cols by 24 rows? MAJIC layout will not be correct if the terminal is not set like this.

Thank you Constantin. With 80x20 terminal the screen looks exactly like the one in the test report.

Remove your update and manually test again to determine if it is your update that causes this. I guess it is so, unless we have this broken in bzr.

Greg Shah wrote:

Remove your update and manually test again to determine if it is your update that causes this. I guess it is so, unless we have this broken in bzr.

I already did this and I'm sure that my changes caused the problem. Trying to understand what was wrong and already have a guess.

Just a note. If the screen size is ~ 132x43 then the failing screen with and without my changes look identical (manual testing):

03/02/2015                      RFQ Customer Cards                      16:24:07
┌──────────────────────────────────────────────────────────────────────────────┐
│        Order: 8000356          Customer: confidential info redacted          │
│Aircraft Type: B767            Work-type: HMV                                 │
└──────────────────────────────────────────────────────────────────────────────┘

                          ┌──────────────────────────────────────────────────────────────────────────────┐
                          │  Sel W.B.C.   Oper Card Num             Description                          │
                          │  ────────────────────────────────────────────────────────────────────────────│
                          │ >                                                                            │
                          │                                                                              │
                          │                                                                              │
                          │                                                                              │
                          │                                                                              │
                          │                                                                              │
                          │      [ Card #  ]                                                             │
                          │                                                                              │
                          │                                                                              │
                          │                                                                              │
                          │                                                                              │
                          └──────────────────────────────────────────────────────────────────────────────┘
                          <G> <B> <A> <D> <F> <U> <O> <C> <P> <N> <E> <S> <R> <K> <H> <M> <I>

However with screen size 80x24 they look different (see above).

Code Review ias_upd20150303c.zip

I think unwrapControlEntity() should be removed from handle. Rebuilding MAJIC will confirm this has no consequences. Conversion and runtime regression testing won't be needed for this change.

If you pass regression testing with ias_upd20150303c.zip, then it is a quick thing to remove this method and rebuild MAJIC. Report the results here. As long as everything is OK, you can check it in and distribute it.

Good work!

Greg Shah wrote:

I think unwrapControlEntity() should be removed from handle. Rebuilding MAJIC will confirm this has no consequences. Conversion and runtime regression testing won't be needed for this change.

If you pass regression testing with ias_upd20150303c.zip, then it is a quick thing to remove this method and rebuild MAJIC. Report the results here. As long as everything is OK, you can check it in and distribute it.

OK.

Good work!

Thank you.

The following tests failed identically in two runs:

4     ctrlc_11_session1     CTRL-C 11 (Session 1) - CL - GSO - RFQ.     FAILED     CONCURRENT     BACKOUT     Driver 0     356.169     

failure in step 18: 'Timeout while waiting for event semaphore to be posted.'

6     ctrlc_11_session4     CTRL-C 11 (Session 4) - CL - GSO - RFQ.     FAILED     CONCURRENT     BACKOUT     Driver 2     300.000     

failure in step 1: 'Timeout while waiting for event semaphore to be posted.'

396     tc_job_002     TIMCO TC-JOB-002 testcase.     FAILED     NONE     BACKOUT     Driver 10     68.038     

failure in step 40: 'Unexpected EOF in actual at page # 1.'

400     tc_job_clock_004     TIMCO TC-JOB-CLOCK-004 testcase.     FAILED     NONE     BACKOUT     Driver 8     240.550     

failure in step 3: 'Timeout while waiting for event semaphore to be posted.'

Test restarted.

In a third run all gso_ctrlc_3way_tests failed. tc_job_clock_004 passed. I've also noticed that the following test failed in all 3 runs:

509     tc_po_item_003     TIMCO TC-PO-ITEM-003 testcase.     FAILED     SEQUENTIAL     BACKOUT     Driver 11     198.239     

failure in step 26: 'timeout before the specific screen buffer became available (Mismatched data at line 20, column 51. Expected ' ' (0x0020 at relative Y 20, relative X 51) and found 'M' (0x004D at relative Y 20, relative X 51).)'

The test tc_po_item_004 was not run as it depends on tc_po_item_003.

gso_ctrlc_3way_tests restarted. I've extracted tc_po_item_003 and tc_po_item_004 and will re-run them.

Igor Skornyakov wrote:

gso_ctrlc_3way_tests restarted. I've extracted tc_po_item_003 and tc_po_item_004 and will re-run them.

With rev 10787 there were baseline changes. You should reconfigure your setup or do a "git pull origin staging" in your ~/testing/majic_baseline/ folder

Constantin Asofiei wrote:

With rev 10787 there were baseline changes. You should reconfigure your setup or do a "git pull origin staging" in your ~/testing/majic_baseline/ folder

Thank you Constantin.

Don't worry about the ctrl-c tests. The 3-way tests are known to fail and so long as all of them don't fail on all runs, we can accept this.

If the ctrl-c tests have gotten good enough results, you can stop running those (use main-regression target to only run the regular functional tests).

Hopefully 1 more run after you update the baselines will be enough.

Greg Shah wrote:

Don't worry about the ctrl-c tests. The 3-way tests are known to fail and so long as all of them don't fail on all runs, we can accept this.

If the ctrl-c tests have gotten good enough results, you can stop running those (use main-regression target to only run the regular functional tests).

Hopefully 1 more run after you update the baselines will be enough.

Actually in a separate run all gso_ctrlc_3way_tests passed. I'm waiting now for the end of extracted tc_po_item_* tests.

After updating majic_baseline tc_po_item_003 and tc_po_item_004. Majic rebuild passed OK.

Passed runtime regression testing and checked in as bzr rev 10793.

Igor, you have a regression when LIST-ITEMS and LIST-ITEM-PAIRS are set via the widget attribute, as in:

def var ch as char view-as combo-box.
form ch with frame f1.
ch:LIST-ITEMS IN FRAME f1 = "list1,list2,list3".
update ch with frame f1.

Please test setting both LIST-ITEMS and LIST-ITEM-PAIRS attributes at runtime, for COMBO-BOX and SELECTION-LIST.

Constantin Asofiei wrote:

Igor, you have a regression when LIST-ITEMS and LIST-ITEM-PAIRS are set via the widget attribute, as in:
[...]

Please test setting both LIST-ITEMS and LIST-ITEM-PAIRS attributes at runtime, for COMBO-BOX and SELECTION-LIST.

Constantin, this is indeed a bug, but it was introduced in #2508. I've found and fixed it. Should I re-run a full regression test?

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor, you have a regression when LIST-ITEMS and LIST-ITEM-PAIRS are set via the widget attribute, as in:
[...]

Please test setting both LIST-ITEMS and LIST-ITEM-PAIRS attributes at runtime, for COMBO-BOX and SELECTION-LIST.

Constantin, this is indeed a bug, but it was introduced in #2508. I've found and fixed it. Should I re-run a full regression test?

Have you tested both attributes with both widgets?

If so, go ahead and do another runtime testing. Please upload the update to #2508 and cross-reference the comment with notes 135/136/137 from this task.

Constantin Asofiei wrote:

Have you tested both attributes with both widgets?

The method is the same for both widgets. The LIST-ITEM_PAIRS attribute was re-tested as well.

If so, go ahead and do another runtime testing. Please upload the update to #2508 and cross-reference the comment with notes 135/136/137 from this task.

OK