FWD » Core Development » User Interface

Please add support for the following:

Options

WIDGET-ID - all widgets, see #1791 for some prior research, we don't need to support the full compatible -usewidgetid implementation but instead we just need to implement a fake approach that is safe for conversion and runtime
DEBLANK - fillin (conversion is there, but no runtime)
NO-TAB-STOP - browse, button, combo-box, editor, fillin, frame, radio-set, selection-list, slider, toggle-box (just set the tab-stop attribute to false and let the rest of the support be common with that attribute)
PASSWORD-FIELD - fillin

Attributes

AUTO-RESIZE
BLANK - fillin, text (option conversion is already there as is the config.blank setter, but attribute conversion is not there and the client-side runtime support is missing)
CONTEXT-HELP-ID - this only needs support for conversion and the server-side setting of the value into the configuration (or returning that value in the case of the getter, further help support will be implemented in #1827 and #1829.
DEBLANK - fillin (conversion is missing, runtime support is needed but is the same as the option above)
FRAME-COL
FRAME-ROW
FRAME-X
FRAME-Y
INDEX
MANUAL-HIGHLIGHT
MENU-MOUSE
PASSWORD-FIELD - fillin
SELECTABLE
TAB-STOP - browse, button, combo-box, editor, fillin, frame, radio-set, selection-list, slider, toggle-box
TOOLTIP - conversion is there and there is already runtime support implemented for buttons and images, this needs to be reworked into a more generic approach (perhaps done at the AbstractWidget level) instead of having to re-implement the support in each specific widget subclass
WIDGET-ENTER
WIDGET-ID - all widgets, see #1791 for some prior research, we don't need to support the full compatible -usewidgetid implementation but instead we just need to implement a fake approach that is safe for conversion and runtime
WIDGET-LEAVE
WINDOW

Methods

MOVE-TO-BOTTOM - browse, button, combo-box, editor, fillin, frame, image, literal, radio-set, rectangle, selection-list, slider, text, toggle-box, window (runtime only)
MOVE-TO-TOP - browse, button, combo-box, editor, fillin, frame, image, literal, radio-set, rectangle, selection-list, slider, text, toggle-box, window (runtime only)

It looks like there is very much in common between "Options" and "Attributes" sections in this task. Is there any difference between two e.g. WIDGET-ID subtasks?
Thank you.

I don't know the answer, but usually the option is just the static way (compile-time) to define a feature as part of a frame definition while the attribute is a dynamic/runtime way to access the same thing.

But each of the overlapping cases should be tested to confirm this assumption.

Greg Shah wrote:

I don't know the answer, but usually the option is just the static way (compile-time) to define a feature as part of a frame definition while the attribute is a dynamic/runtime way to access the same thing.

But each of the overlapping cases should be tested to confirm this assumption.

I see. Thank you.

Greg Shah wrote:

Basically, yes.

We need to do whatever the 4GL does when the command line parameter -usewidgetid is NOT used. It is a no-op as described in #1791. The P2J code needs to be compatible with how the 4GL does it. But we are NOT going to implement the -usewidgetid support.

I see. Sorry for misunderstanding. This is almost done. I will finish in a couple of hours.

I've finished with the WIDGET-ID.

Please create a task branch for this task (2567a) and commit all your current work. All subsequent work should be done in task branches. Thanks!

Tha task branch 2567a has been created. First updates are committed to the branch, revno: 10896

Igor Skornyakov wrote:

I've noticed that X and Y attributes' p2j support is not compatible with 4GL. In the attached test the values are (1,17) instead of (0,0). These attributes are related to ones I'm working on now. Should I fix this incomatility?
Thank you.

Hynek: Is this expected? I thought our X and Y support was complete.

Greg Shah wrote:

Igor Skornyakov wrote:

I've noticed that X and Y attributes' p2j support is not compatible with 4GL. In the attached test the values are (1,17) instead of (0,0). These attributes are related to ones I'm working on now. Should I fix this incomatility?
Thank you.

Hynek: Is this expected? I thought our X and Y support was complete.

There are two things:

- a bug in ZeroLayoutColumn positioning the selection-list widget at ROW 3 instead of 1. I think the logic allocates space for the widget's label.
- BaseEntity.getX() and BaseEntity.getY() adds 1 to the returned value, which is incorrect. X/Y are zero-based.

Hynek Cihlar wrote:

- a bug in ZeroLayoutColumn positioning the selection-list widget at ROW 3 instead of 1. I think the logic allocates space for the widget's label.

Constantin, if I remember correctly you had some WIP or planned changes in ZeroLayoutColumn so maybe it would make sense if you took this.

- BaseEntity.getX() and BaseEntity.getY() adds 1 to the returned value, which is incorrect. X/Y are zero-based.

Igor, I fixed this as part of 2424b.

Hynek Cihlar wrote:

Igor, I fixed this as part of 2424b.

Thank you Hynek.

Hynek Cihlar wrote:

Hynek Cihlar wrote:

- a bug in ZeroLayoutColumn positioning the selection-list widget at ROW 3 instead of 1. I think the logic allocates space for the widget's label.

Constantin, if I remember correctly you had some WIP or planned changes in ZeroLayoutColumn so maybe it would make sense if you took this.

Yes, I think this is related to the same issue when top-labels are used, P2J starts counting from the top-border and not from just below the top-label separator.

More, there is another issue: in 4GL, widget coordinates are relative to the parent. But the widget's parent is always a FIELD-GROUP, and sometimes (in case of down-frames and top-labels) a frame can have more than one field-group - in these cases, P2J will report incorrect coordinates for the widgets, as they will be relative to the frame. When there are side-labels and a single field-group (non-down or 1-down frames), the coordinates are reported OK, as the field-group is positioned on the frame's top-left corner.

Constantin Asofiei wrote:

More, there is another issue: in 4GL, widget coordinates are relative to the parent. But the widget's parent is always a FIELD-GROUP, and sometimes (in case of down-frames and top-labels) a frame can have more than one field-group - in these cases, P2J will report incorrect coordinates for the widgets, as they will be relative to the frame. When there are side-labels and a single field-group (non-down or 1-down frames), the coordinates are reported OK, as the field-group is positioned on the frame's top-left corner.

Would you have a test case showing this?

Rebased task branch 2567a from P2J trunk revision 10896 (new branch revision 10898).

Hynek Cihlar wrote:

Constantin Asofiei wrote:

More, there is another issue: in 4GL, widget coordinates are relative to the parent. But the widget's parent is always a FIELD-GROUP, and sometimes (in case of down-frames and top-labels) a frame can have more than one field-group - in these cases, P2J will report incorrect coordinates for the widgets, as they will be relative to the frame. When there are side-labels and a single field-group (non-down or 1-down frames), the coordinates are reported OK, as the field-group is positioned on the frame's top-left corner.

Would you have a test case showing this?

See uast/frame_layout/widget-hierarchy.p - it outputs the entire widget hierarchy into a file. You will see that the Y for field-group's is non-zero, while for the inner children (the fill-in's) they are zero. Also, if you change the widget's row coordinate for the first, second, etc frame line, it will output it relative to the parent field-group, not the frame.

Igor Skornyakov wrote:

I've used the attached test to understand the correlation between ROW/COLUMN and FRAME-ROW/FRAME-COL attributes. It seems however that values assigned to these attributes in p2j are ignored. This can be seen if one press 'Y': all lines will be the same while in P2J they reflect assigned values (the last for numbers are the values of the X, Y, COLUMN and ROW attributes respectively). The actual position of the widget on the screen is also not affected. In the debugger I see multiple invocations of the setLocation/setPhysicalLocation with identical arguments (0,5).

This is again a bug in ZeroColumnLayout. Although server logic correctly assigns the widget's position and this is stored in BaseConfig.column/BaseConfig.row, the stacked layout logic doesn't account for this. I would leave this again for Constantin.

Hynek Cihlar wrote:

This is again a bug in ZeroColumnLayout. Although server logic correctly assigns the widget's position and this is stored in BaseConfig.column/BaseConfig.row, the stacked layout logic doesn't account for this. I would leave this again for Constantin.

Thank you Hynek. However this is a showstopper for me, so in a meantime I'll try to fix it myself.

Igor Skornyakov wrote:

Hynek Cihlar wrote:

This is again a bug in ZeroColumnLayout. Although server logic correctly assigns the widget's position and this is stored in BaseConfig.column/BaseConfig.row, the stacked layout logic doesn't account for this. I would leave this again for Constantin.

Thank you Hynek. However this is a showstopper for me, so in a meantime I'll try to fix it myself.

Perhaps it makes sense to work on other attributes first? That would give Constantin time to fix this and take this out of your critical path.

Greg Shah wrote:

Perhaps it makes sense to work on other attributes first? That would give Constantin time to fix this and take this out of your critical path.

Thank you Greg. This makes sense. I will switch to other attributes.

Added PASSWORD-FIELD conversion support. Commited to branch 2567a. New revno 10900

Added PASSWORD-FIELD runtime support. Commited to branch 2567a. New revno 10901

Added BLANK conversion and runtime support. Commited to branch 2567a. New revno 10902

Added DEBLANK conversion and runtime support. Commited to branch 2567a. New revno 10903

Added CONTEXT-HELP-ID conversion and runtime support. Commited to branch 2567a. New revno 10904

Added NO-TAB-STOP option and TAB-STOP attribute conversion support. Commited to branch 2567a. New revno 10905.

Greg,
Is the implementation of the runtime semantics of the TAB-STOP attribute in the scope of this task?
Thank you.

Is the implementation of the runtime semantics of the TAB-STOP attribute in the scope of this task?

Yes.

Greg Shah wrote:

Is the implementation of the runtime semantics of the TAB-STOP attribute in the scope of this task?

Yes.

I see. Thank you.

About the FRAME-COL/ROW/X/Y coordinate attributes: the current BaseConfig.column/row/x/y fields are actually for the FRAME- attributes, as these are relative to the frame (the client-side is missing a field-group parent). I'll work on these (conversion, server and client-side support) part of the #1796 changes.

test please ignore

Added TAB-STOP attribute runtime support. Commited to branch 2567a. New revno 10906.

This took some time because the TAB/BACK-TAB processing logic is implemented in a slightly different way in different widgets.

Added INDEX and MANUAL-HIGHLIGHT conversion support. Commited to branch 2567a. New revno 10907

Rebased task branch 2567a from P2J trunk revision 10898 (new branch revision 10909).

Greg,
I've implemented the conversion and setters/getters for the MANUAL-HIGHLIGHT attribute, but the only thing I've found about it semantics in the documentation is Set the MANUAL-HIGHLIGHT attribute to TRUE to use a customized highlight design for selection of the widget. A FALSE value for this attribute specifies the ABL default highlight behavior for the selection of the widget.. This is too terse for me. Can you please give a clue in about what functionality is expected at the p2j side?
Thank you.

As far as I understand it, using this option should cause the widget to draw without a highlight. The programmer would then have to accept that "look" or would have to implement some other 4GL feature to cause the "look" to be different. We don't care about the other 4GL features that might be used. I just want your changes to implement the drawing the way that the 4GL does in this case. Whatever drawing differences are there, should be duplicated.

I imagine it will vary by widget and is documented to be supported for browse, button, combo-box, editor, fill-in, frame, image, literal, radio-set, rectangle, selection-list, slider, text and toggle-box.

In the GUI application we are working on, this is only set to true in 2 places (one is for EDITOR and the other is TOGGLE-BOX). I will send you the code separately, for your review.

At a minimum, we must support whatever drawing differences are implemented in the 4GL for GUI EDITOR and GUI TOGGLE-BOX.

For now, we can avoid support for ChUI and support for other GUI widgets.

Greg Shah wrote:

As far as I understand it, using this option should cause the widget to draw without a highlight. The programmer would then have to accept that "look" or would have to implement some other 4GL feature to cause the "look" to be different. We don't care about the other 4GL features that might be used. I just want your changes to implement the drawing the way that the 4GL does in this case. Whatever drawing differences are there, should be duplicated.

I imagine it will vary by widget and is documented to be supported for browse, button, combo-box, editor, fill-in, frame, image, literal, radio-set, rectangle, selection-list, slider, text and toggle-box.

In the GUI application we are working on, this is only set to true in 2 places (one is for EDITOR and the other is TOGGLE-BOX). I will send you the code separately, for your review.

At a minimum, we must support whatever drawing differences are implemented in the 4GL for GUI EDITOR and GUI TOGGLE-BOX.

For now, we can avoid support for ChUI and support for other GUI widgets.

Got it. Thank you.

I was unable to figure any noticeable semantics of the MANUAL-HIGHLIGHT attribute in the ChUI.

About the remaining attributes/methods.

1. I plan to finish with WIDGET-ENTER, WIDGET-LEAVE, and WINDOW attributes tomorrow.

2. As per Constantin the fixes which are required for the FRAME-COL, FRAME-ROW, FRAME-X, and FRAME-Y attributes are supposed to be fixed by him later. Will we postpone the implementation of the runtime support for these attributes or I should try to find a solution independently?

3. According to the 4GL documentation the SELECTABLE, AUTO-RESIZE, MENU-MOUSE, and TOOLTIP attributes are meaningful in GUI only. I was working only with ChUI before and it can take me a day or to to get familiar with GUI. The #2614 task assigned to me today is only about GUI, so I will have to do it anyway soon.

4. MOVE-TO-BOTTOM/MOVE-TO-TOP methods are also mostly about GUI, In ChUI they are meaningful only if the FRAME:KEEP-FRAME-Z-ORDER attribute is set to true. As I can see at this moment we have a conversion support for this attribute but it seem that its semantics is not implemented.

Keeping in mind that this task is supposed to be finished soon (even if not 100% ready) may be it makes sense to re-triage the remaining issues? I'm pretty sure that I will be able to finish with the FRAME-COL, FRAME-ROW, FRAME-X, and FRAME-Y attributes early next week.

May be it makes sense to move GUI-specific attributes/methods to a separate task?

Thank you.

Igor Skornyakov wrote:

2. As per Constantin the fixes which are required for the FRAME-COL, FRAME-ROW, FRAME-X, and FRAME-Y attributes are supposed to be fixed by him later. Will we postpone the implementation of the runtime support for these attributes or I should try to find a solution independently?
...
I'm pretty sure that I will be able to finish with the FRAME-COL, FRAME-ROW, FRAME-X, and FRAME-Y attributes early next week.

I'll take both the runtime and conversion support for these. So please don't spend time working on them.

Constantin Asofiei wrote:

I'll take both the runtime and conversion support for these. So please don't spend time working on them.

OK. Thank you Constantin.

Igor Skornyakov wrote:

I was unable to figure any noticeable semantics of the MANUAL-HIGHLIGHT attribute in the ChUI.

...

3. According to the 4GL documentation the SELECTABLE, AUTO-RESIZE, MENU-MOUSE, and TOOLTIP attributes are meaningful in GUI only. I was working only with ChUI before and it can take me a day or to to get familiar with GUI. The #2614 task assigned to me today is only about GUI, so I will have to do it anyway soon.

4. MOVE-TO-BOTTOM/MOVE-TO-TOP methods are also mostly about GUI, In ChUI they are meaningful only if the FRAME:KEEP-FRAME-Z-ORDER attribute is set to true. As I can see at this moment we have a conversion support for this attribute but it seem that its semantics is not implemented.

...

May be it makes sense to move GUI-specific attributes/methods to a separate task?

Please go ahead with the GUI implementation of all of these features as part of this task. Of course, if there is some ChUI behavior, please implement that too. I understand that you will need a little time to ramp-up on the GUI design. As you say, you are going to need it soon anyway.

Greg Shah wrote:

Please go ahead with the GUI implementation of all of these features as part of this task. Of course, if there is some ChUI behavior, please implement that too. I understand that you will need a little time to ramp-up on the GUI design. As you say, you are going to need it soon anyway.

OK. Thank you Greg.

There is a minor incompatibility between p2j and 4GL. If h is a handle with the 'unknown' value then the statement message h displays '?' in 4GL but generates error message ** Could not find widget handle for window from expression. (3138) in p2j.

Added conversion support for the WINDOW, WIDGET-ENTER, and WIDGET-LEAVE attributes.

Rebased task branch 2567a from P2J trunk revision 10900 (new branch revision 10912).

Fixed Javadoc, committed to the branch 2567a (new branch revision 10913)

I should have been more clear earlier: please implement KEEP-FRAME-Z-ORDER for the window widget so that your MOVE-TO-* work can be fully realized.

Greg Shah wrote:

I should have been more clear earlier: please implement KEEP-FRAME-Z-ORDER for the window widget so that your MOVE-TO-* work can be fully realized.

Got it. Thank you.

Partial runtime support for the WIDGET-ENTER and WIDGET-LEAVE attributes. Committed to the branch 2567a, revno 10914.

Rebased task branch 2567a from P2J trunk revision 10901 (new branch revision 10915).

The logic of the FocusManager.focusChange() method should be changed as within the ON LEAVE trigger it should be possible to provide a correct value of the WIDGET-ENTRY attribute.

p2j generates incompatible error message when referencing to the attributes of the handle which has 'unknown' value.
Should be: "Lead attributes in a chained-attribute expression (a:b:c) must be type HANDLE or a user-defined type and valid (not UNKNOWN). (10068)"

Igor Skornyakov wrote:

p2j generates incompatible error message when referencing to the attributes of the handle which has 'unknown' value.
Should be: "Lead attributes in a chained-attribute expression (a:b:c) must be type HANDLE or a user-defined type and valid (not UNKNOWN). (10068)"

When you find cases like this, please document both the expected behavior and the P2J behavior. And eventually a simple test case.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor Skornyakov wrote:

p2j generates incompatible error message when referencing to the attributes of the handle which has 'unknown' value.
Should be: "Lead attributes in a chained-attribute expression (a:b:c) must be type HANDLE or a user-defined type and valid (not UNKNOWN). (10068)"

When you find cases like this, please document both the expected behavior and the P2J behavior. And eventually a simple test case.

Sorry.
When running the attached program with 4GL four (10068) messages are generated at startup while p2j generates two:
Invalid handle. Not initialized or points to a deleted object. (3135)
Cannot access the NAME attribute because the widget does not exist. (3140)

For this task, please change your test to protect against unknown handle usage. So, instead of doing LAST-EVENT:WIDGET-LEAVE:NAME you need to do something like (if valid-handle(last-event:widget-leave) then last-event:widget-leave:name else "?"). Otherwise, the error messages you are seeing will hide other info about the handles you are adding support for.

Constantin Asofiei wrote:

For this task, please change your test to protect against unknown handle usage. So, instead of doing LAST-EVENT:WIDGET-LEAVE:NAME you need to do something like (if valid-handle(last-event:widget-leave) then last-event:widget-leave:name else "?"). Otherwise, the error messages you are seeing will hide other info about the handles you are adding support for.

Of course I will do that. I've mentioned this incompatibility just for memory.

Thanks for posting the unknown value processing/messages problems (notes 46 and 56). I have created task #2629 for these issues. You don't have to fix them as part of the current task.

Igor Skornyakov wrote:

The logic of the FocusManager.focusChange() method should be changed as within the ON LEAVE trigger it should be possible to provide a correct value of the WIDGET-ENTRY attribute.

Thanks for the heads-up on this. I assume you are working on this as part of the current task. Focus processing is tricky. Let us know if you have any questions.

Greg Shah wrote:

Thanks for the heads-up on this. I assume you are working on this as part of the current task. Focus processing is tricky. Let us know if you have any questions.

Yes, I do. Actually I think I've found how to deal with it, albeit it is a little bit tricky as you said. I've also noticed another minor incompatibility which I'm goind to fix: if there are no another widget to gain focus (e.g. after TAB) then 4GL doesn't activate ON LEAVE trigger while p2j currently does.

Igor Skornyakov wrote:

Greg Shah wrote:

Thanks for the heads-up on this. I assume you are working on this as part of the current task. Focus processing is tricky. Let us know if you have any questions.

Yes, I do. Actually I think I've found how to deal with it, albeit it is a little bit tricky as you said. I've also noticed another minor incompatibility which I'm goind to fix: if there are no another widget to gain focus (e.g. after TAB) then 4GL doesn't activate ON LEAVE trigger while p2j currently does.

When you find incompatibilities like this, please confirm 4GL test behaves the some, both in ChUI and GUI.

Constantin Asofiei wrote:

When you find incompatibilities like this, please confirm 4GL test behaves the some, both in ChUI and GUI.

OK. Can you please be more specific regarding 'like this'? Does it mean 'not explicitely ChUI specific'?
Thank you.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

When you find incompatibilities like this, please confirm 4GL test behaves the some, both in ChUI and GUI.

OK. Can you please be more specific regarding 'like this'? Does it mean 'not explicitely ChUI specific'?

I mean when you find something which is related to the core focus or event processing. We need to make sure ChUI and GUI behave the same, before changing the P2J code. Although I hope we will not find any differences between ChUI and GUI, in 4GL.

Constantin Asofiei wrote:

I mean when you find something which is related to the core focus or event processing. We need to make sure ChUI and GUI behave the same, before changing the P2J code. Although I hope we will not find any differences between ChUI and GUI, in 4GL.

Got it. Thank you.

Partial runtime support for the WIDGET-ENTER and WIDGET-LEAVE attributes. Committed to the branch 2567a, revno 10916.

Finished implementation of the runtime support for the WIDGET-ENTER and WIDGET-LEAVE attributes. Committed to the branch 2567a, revno 10917.

The bug mentioned in the note #50 still exists.

Other than the bug in note 50, am I correct that only the GUI related features are left?

That would be this list:

Attributes
SELECTABLE
AUTO-RESIZE
MENU-MOUSE
TOOLTIP
FRAME:KEEP-FRAME-Z-ORDER

Methods
MOVE-TO-BOTTOM
MOVE-TO-TOP

Am I missing anything?

Greg Shah wrote:

Other than the bug in note 50, am I correct that only the GUI related features are left?

That would be this list:

Attributes
SELECTABLE
AUTO-RESIZE
MENU-MOUSE
TOOLTIP
FRAME:KEEP-FRAME-Z-ORDER

Methods
MOVE-TO-BOTTOM
MOVE-TO-TOP

Am I missing anything?

Yes, this is correct.

OK, good. Then we have 3 options on moving forward:

1. Review and regression test the current 2567a branch. Then fix the note 50 bug and add the GUI features in branch 2567b.

2. Fix note 50 in 2567a. Review and regression test the current 2567a branch. Then add the GUI features in branch 2567b.

3. Fix note 50 and add GUI features in 2567a and then review and regression test.

What is your preference? I'm leaning toward option 2, but I'm open to your thoughts.

Greg Shah wrote:

OK, good. Then we have 3 options on moving forward:

1. Review and regression test the current 2567a branch. Then fix the note 50 bug and add the GUI features in branch 2567b.

2. Fix note 50 in 2567a. Review and regression test the current 2567a branch. Then add the GUI features in branch 2567b.

3. Fix note 50 and add GUI features in 2567a and then review and regression test.

What is your preference? I'm leaning toward option 2, but I'm open to your thoughts.

Well, suggest to take the route 2. If this will not be done today and at the weekend, then revert to the route 1.
It it OK?
Thank you,

Well, suggest to take the route 2. If this will not be done today and at the weekend, then revert to the route 1.
It it OK?

Hynek's 2424b update is being tested next. I hope it will be in regression testing Monday. Your update will be next. If Hynek's update has issues we can push yours first as needed, but generally it is OK if you are ready for testing on Tuesday.

Greg Shah wrote:

Well, suggest to take the route 2. If this will not be done today and at the weekend, then revert to the route 1.
It it OK?

Hynek's 2424b update is being tested next. I hope it will be in regression testing Monday. Your update will be next. If Hynek's update has issues we can push yours first as needed, but generally it is OK if you are ready for testing on Tuesday.

I see. I will submit for review at Monday.

Fixed the bug with the ON LEAVE trigger activation (note #50). Committed to the branch 2567a, revno 10918.

That is great!

Now may be a good time to rebase. Do you need any more changes/cleanup before code review?

Greg Shah wrote:

That is great!

Now may be a good time to rebase. Do you need any more changes/cleanup before code review?

I'm rebasing right now. After that I will add Javadocs and submit for a code review

Rebased task branch 2567a from P2J trunk revision 10906 (new branch revision 10923).

Added Javadocs. Committed to the branch 2567a, revno 10924.
Ready for the code review

Code Review Task Branch 2567a Revision 10924

This is quite good. Most issues are minor, but issue 4 is a more serious one.

1. FillInWidget.getScreenValue() needs javadoc.

2. LogicalTerminal.getWidgetHnadleById() should probably be named getWidgetHandleById().

3. SelectionListImpl.nextFocus()/previousFocus() need javadoc.

4. The FocusManager.focusChange() modification to move the LEAVE generation to after the focus change is probably incorrect. I know it looks wrong, but the LEAVE must be processed while the focus is still on the original widget. During the LEAVE processing, a return NO-APPLY will cancel the focus change. If the LEAVE is not canceled, then we do a "temporary" focus change and then back to the original widget. I can't remember exactly why this extra "change and back" is needed. I seem to recall that perhaps it was about ensuring that the focus change would work, but I'm not sure. It was always something I wanted to get rid of, but I know it was needed. Then the ENTRY is generated and during the ENTRY processing a return NO-APPLY rejects the focus change. A successful ENTRY causes the focus to change for good. I'm pretty sure your changes are will break things... If the change is needed for the tab-stop cases, perhaps it should only be active then. Please review the documentation in ui/chui/package.html under "Progress Focus Management". There may be some really useful info there.

5. Please remove any references to CHARVA in KeyInput. That is no longer valid reference (we don't use CHARVA anymore).

6. Please add the new interfaces to HandleCommon.

Greg Shah wrote:

Code Review Task Branch 2567a Revision 10924

This is quite good. Most issues are minor, but issue 4 is a more serious one.

1. FillInWidget.getScreenValue() needs javadoc.

2. LogicalTerminal.getWidgetHnadleById() should probably be named getWidgetHandleById().

3. SelectionListImpl.nextFocus()/previousFocus() need javadoc.

4. The FocusManager.focusChange() modification to move the LEAVE generation to after the focus change is probably incorrect. I know it looks wrong, but the LEAVE must be processed while the focus is still on the original widget. During the LEAVE processing, a return NO-APPLY will cancel the focus change. If the LEAVE is not canceled, then we do a "temporary" focus change and then back to the original widget. I can't remember exactly why this extra "change and back" is needed. I seem to recall that perhaps it was about ensuring that the focus change would work, but I'm not sure. It was always something I wanted to get rid of, but I know it was needed. Then the ENTRY is generated and during the ENTRY processing a return NO-APPLY rejects the focus change. A successful ENTRY causes the focus to change for good. I'm pretty sure your changes are will break things... If the change is needed for the tab-stop cases, perhaps it should only be active then. Please review the documentation in ui/chui/package.html under "Progress Focus Management". There may be some really useful info there.

5. Please remove any references to CHARVA in KeyInput. That is no longer valid reference (we don't use CHARVA anymore).

6. Please add the new interfaces to HandleCommon.

Thank you Greg,
I will review my changes in FocusManager more thoroughly. I understand that a cleaner way is to find the next focus candidate w/o actually chaging focus (parent().nextFocus()/parent.prevFocus() invocation), even temporary. I was confused with the comment about temporary change and didn't realized that it has side effects.

Fixed issues 1,2,3,6 mentioned in the code review. Committed to the branch 2567a, revno 10925.

Regarding the issue 5. The CHARVA is mentioned in Javadoc only which is actually a copy/paste of the Javadoc of other method. CHARVA is mentioned in many places in this class. Should I remove them all?
Thank you.

CHARVA is mentioned in many places in this class. Should I remove them all?

Yes, please do.

Fixed issues 5 mentioned in the code review.

I've tested several scenarios: no widget for the new focus, NO-APPLY in the ON-LEAVE trigger and NO-APPLY in the ON-ENTER trigger. I've found and fixed an issue with incorrect trigger activation when the potential new focus has TAB-STOP = false. The rest seems working OK apart from the following issue: if ON-LEAVE/ON-ENTER trigger returns NO-APPLY then there is no cursor in the initial widget. BTW: the same problem is with the original version of the FocusManager, but only for NO-APPLY in the ON-ENTER trigger.

It seems that the trick with temporary focus change and restoring it afterwards needs some elaboration. Some traces a remained and after FocusManager.focusChange() is ended the widget which temporary gained focus becomes partially activated (in the case of the SelectionList a current item becomes highlighted) and in the widget which retained focus due to NO-APPLY has focus but cursor is not visible.

Committed to the branch 2567a, revno 10926.

The side effect of the temporary focus change where the events posted in the AbstractWidget.requestFocus() method. I've found the workaround.

Committed to the branch 2567a, revno 10927.

Do you believe this is the full solution to the focus issues? (Is it ready for final review?)

Greg Shah wrote:

Do you believe this is the full solution to the focus issues? (Is it ready for final review?)

I hope so. Of couse I cannot be 100% sure as I still do not completely understand the logic of the focus management. However my workaround at least fixes the issue that existed before my changes.

Igor, please rebase to trunk rev 10909, your branch is built on top of trunk rev 10906.

Rebased task branch 2567a from P2J trunk revision 10909 (new branch revision 10930).

1. methods_attributes.rules:
   - line 376: you've changed the name of the getter; the correct one is getFrameColumn
2. HandleCommon:
   - add the new interfaces in alphabetical order
3. Trigger.INFO
   - in P2J we do not use ThreadLocal variables, as the same state can be accessed from multiple threads. For this, we have the typed ContextLocal class. Also, static fields (unless they are also final) are named in lowercase, not uppercase.
   - more, the way you use this in LogicalTrigger.trigger, this state is better kept as an instance field for the LogicalTerminal class, exposing apropriate APIs to set/interogate it.
4. EventInfo, BlankInterface, DeblankInterface, IndexedInterface, PasswordFieldInterface
   - class-level javadoc is missing
   - the correct history header is (the JPRM column is obsolete and we don't add it to new files):
   

   ** -#- -I- --Date-- ---------------------------------Description----------------------------------
   ** 001 IAS 20150801 Created initial version.
   

   
   - the instance fields need to be separated by a new-line
   - there is an extra empty line at the end of the file, before last curly-brace
5. KeyInput
   - line 137: there should be an empty line between constructors
6. TextImpl
   - line 82: you have a double display = display
7. Widget
   - line 650: the javadoc for return needs to start on the same line as the tag
8. AbstractWidget
   - line 1872: the javadoc for return needs to start on the same line as the tag
   - this is a client-side only class, so no ThreadLocal or ContextLocal vars are required
   - the state kept in TEMP_FOCUS_CHANGE should be in FocusManager as an instance boolean field (and the associated APIs)
   - as a side note, the static methods enter/leaveTempFocusChange are not in the correct place
9. SelectionListImpl
   - the header entry number is incorrect; please double check all files for the header numbers to be correct
10. CommonWidget
    - put getFrameRow back in the previous place
    - remove the getFrameCol API you added
11. FillInConfig
    - line 96/97: remove an extra empty line
12. FillinWidget
    - setPasswordField(boolean), setBlank(boolean) is missing pushScreenDefinition
    - setDeblan(logical) ends up calling pushScreenDefinition twice - leave it only in the worker setDeblank(boolean) - where the config attribute is actually set
13. GenericWidget
    - getWidgetId - is it correct for this to return new integer() (which is 4GL unknown value)? Why not return the actual widget ID?
    - getFrameCol() - remove this API
    - setContextHelpId(int id) - pushScreenDefinition is missing
    - setManualHighlight(boolean) - pushScreenDefinition is missing
    - please ensure all attribute setters you've added end up calling pushScreenDefinition (in all classes)
14. FocusManager
    - the AbstractWidget.enterTempFocusChange(); and AbstractWidget.leaveTempFocusChange(); calls need to be changed with FocusManager instance field/APIs
    - you've moved this code from focusChange somewhere later in the method:
    

             KeyInput leave = new KeyInput(widget,
                                           EventType.KEY_PRESSED,
                                           Keyboard.SE_LEAVE);
    
             tc.processProgressEvent(leave);
    
             // check to see if the trigger returned no-apply
             if (leave.isConsumed())
                return false;
    

    
    Altough you note that your NO-APPLY tests are working, this is a very sensitive area. So if MAJIC testing fails, this might be a cause.

1. update_focus*.p

2. update_editing*.p

3. waitfor_focus*.p

4. wait-for_focus_run.p (which I think runs all the

   wait-for_*focus*.p

   programs)

Ideally, even if MAJIC testing passes with your changes, all of these should be checked manually. At least, convert the wait-for_focus_run.p suit and see how P2J behaves.

Thank you Constatin.
I will change the code as you advised.

Constantin Asofiei wrote:s in alphabetical order

1. Trigger.INFO
   - in P2J we do not use ThreadLocal variables, as the same state can be accessed from multiple threads. For this, we have the typed ContextLocal class. Also, static fields (unless they are also final) are named in lowercase, not uppercase.
   - more, the way you use this in LogicalTrigger.trigger, this state is better kept as an instance field for the LogicalTerminal class, exposing apropriate APIs to set/interogate it.

Constantin. Can you please explain me this point in more details. I haven't seen any standard precautions regarding multithreading in the code and as far as I remember Greg explained me that the code is essentially a single threaded. I've used ThreadLocal just to avoid proparating additional data through the long call chain. As far as I can see in me case all happends in a single thread.
In any case I will replace ThreadLocal with ContextLocal as you recommend. My question is just for better understanding.
Thank you.

Igor Skornyakov wrote:

Constantin Asofiei wrote:s in alphabetical order

1. Trigger.INFO
   - in P2J we do not use ThreadLocal variables, as the same state can be accessed from multiple threads. For this, we have the typed ContextLocal class. Also, static fields (unless they are also final) are named in lowercase, not uppercase.
   - more, the way you use this in LogicalTrigger.trigger, this state is better kept as an instance field for the LogicalTerminal class, exposing apropriate APIs to set/interogate it.

Constantin. Can you please explain me this point in more details. I haven't seen any standard precautions regarding multithreading in the code and as far as I remember Greg explained me that the code is essentially a single threaded. I've used ThreadLocal just to avoid proparating additional data through the long call chain. As far as I can see in me case all happends in a single thread.

The key part here is this: although 4GL is single-threaded, some processing is done asynchronously. An example is CTRL-C which sends a STOP condition to the program; this can be pressed by the user at any time, regardless which side has control: client-side, server-side, or even a remote server, if the server-side needs some processing done on a remote, secondary server (i.e. database access). So, in this case, when CTRL-C is pressed, if the control is not on the client-side (for example, waiting for a key press), this is sent to the server-side asynchronously. And, as the user's Conversation thread can't handle it (as is busy either waiting for the client-side to return control to it or for a remote server to finish work), the request will be handled by one of the available Dispatcher threads - this will ensure that the user's context is properly set before handling the request.

For these reasons, we can't use ThreadLocal, as this isolates the state to a single thread. Instead, we are using our own ContextLocal implementation: this stores the data in the authenticated user's context. So, regardless of the thread which handles a request, this ensures that the data is available. More details can be found in the javadoc for ContextLocal, SecurityContext, SecurityManager (the authenticateLocal API which performs user authentication is responsible for creating and assigning the user's SecurityContext).

In any case I will replace ThreadLocal with ContextLocal as you recommend. My question is just for better understanding.

You don't need to replace it, you need to move this field to LogicalTerminal as an instance boolean field, as I noted above. The instance of this class is already user-specific, so no other protection is needed.

Another point: it is not required that all sessions use the conversation model (where a dedicated thread executes the logic for the user's session). It is valid for user sessions to use the Dispacher approach where there is a thread pool and any incoming request is handled by the next available thread. In such a case, the threads context is set before it starts executing logic. This is another place where the thread-local approach would break.

Thank you gentlemen. I understand the point. Although in this particular case using of a ThreadLocal is just a shortcut for propagating an additional argument through a long call chain it can be possible that in the future this call chain will be replaced with chained queues in a SEDA style.

Fixed all issues from the Constantin's code review except the one regarding the Trigger.INFO. The getWidhetId() returns 'unknown' as so does 4GL in the absense of the -usewidgetid command line option. The support of this option was not in the scope of this task.

Committed to the branch 2567a, revno 10931.

Got rid of Trigger.INFO.

Committed to the branch 2567a, revno 10932.

Code Review Task Branch 2567a Revision 10932

I made a few minor additions or removals of whitespace in some files. Those changes are committed to the branch.

1. Please add javadoc to the new methods in LogicalTerminal.

2. The only real concern I have is with making the LogicalTerminal.locate() into a public method. I think we don't want to do that, at least not just for the purpose of KeyReader usage. Please add this:

   public static EventInfo obtainEventInfo()
   {
      return locate().getEventInfo();
   }

Then please make the locate() private again. Today we don't have a way to access the LT instance externally, so we know that we don't ever have to worry about stale instances when we reset the client session (e.g. in a CTRL-C scenario). That is why we don't directly access the instance outside of the LT itself.

3. Your protection logic regarding the temporary focus change seem interesting. If it works well, I think it will be a nice improvement. Please report on your manual testing of the testcases listed by Constantin in note 89.

Greg Shah wrote:

Code Review Task Branch 2567a Revision 10932

I made a few minor additions or removals of whitespace in some files. Those changes are committed to the branch.

1. Please add javadoc to the new methods in LogicalTerminal.

2. The only real concern I have is with making the LogicalTerminal.locate() into a public method. I think we don't want to do that, at least not just for the purpose of KeyReader usage. Please add this:

[...]

Then please make the locate() private again. Today we don't have a way to access the LT instance externally, so we know that we don't ever have to worry about stale instances when we reset the client session (e.g. in a CTRL-C scenario). That is why we don't directly access the instance outside of the LT itself.

3. Your protection logic regarding the temporary focus change seem interesting. If it works well, I think it will be a nice improvement. Please report on your manual testing of the testcases listed by Constantin in note 89.

Thank uou Greg. I've made the changes you've mentioned.

Committed to the branch 2567a, revno 10934.

I'm running the tests Constantin wrote about now.

There are several errors in compilation of the converted testcases like this:

[javac] /home/ias/p2j/testcases/uast/src/com/goldencode/testcases/ui/list_widgets/UpdateEditingFocusFrame0.java:11: error: getX() in UpdateEditingFocusFrame0 clashes with getX() in CommonFrame
[javac] public character getX();


Should I try to fix them?
Thank you.

Some tests in wait-for_focus_run.p fail:


Invalid handle. Not initialized or points to a deleted object. (3135)
Cannot access the NAME attribute because the widget does not exist. (3140)


Investigating

Exactly the same error with the previous version of the FocusManager.focusChange()

Continue investigation.

Igor Skornyakov wrote:

There are several errors in compilation of the converted testcases like this:

[javac] /home/ias/p2j/testcases/uast/src/com/goldencode/testcases/ui/list_widgets/UpdateEditingFocusFrame0.java:11: error: getX() in UpdateEditingFocusFrame0 clashes with getX() in CommonFrame
[javac] public character getX();


Should I try to fix them?

No, we will work those problems in #2558. Please just modify the 4GL source code to bypass the issue.

Greg Shah wrote:

No, we will work those problems in #2558. Please just modify the 4GL source code to bypass the issue.

OK, thank you.

Exactly the same error with the previous version of the FocusManager.focusChange()

OK, you don't need to fix this problem.

Please create a new task, make it related to this one and put enough details in to allow us to recreate the problem.

Greg Shah wrote:

Please create a new task, make it related to this one and put enough details in to allow us to recreate the problem.

Do you mean that I have to stop the investigation and attempts to fix this issue?

When you said "previous version of FocusManager", did you mean that that problem already exists in the trunk of P2J?

Greg Shah wrote:

When you said "previous version of FocusManager", did you mean that that problem already exists in the trunk of P2J?

Well, actually I can say that this problem exists with the version of the FocusManager.focusChange() method from the p2j revision 10909 (w/o the tricks I've introduced for the WIDGET-LEAVE/WIDGET-ENTRY support).

The problem is that locate().client.getFocus() returns -1. Test wait-for_restore_focus1.p line 39.

If you are close to a resolution, then go ahead and fix it now. If not, please open a new task and make it related to this one. If you open a new task, please record whatever findings you have.

Greg Shah wrote:

If you are close to a resolution, then go ahead and fix it now. If not, please open a new task and make it related to this one. If you open a new task, please record whatever findings you have.

I hope that I will fix the issue in the next couple of hours. It doesn't look complicated and I have a clear route in mind to its solution.

Sounds good, thanks.

Rebased task branch 2567a from P2J trunk revision 10916 (new branch revision 10941).

Started regression testing of the branch 2567a

Code Review Task Branch 2567a Revision 10941

The changes look fine. At some point we will need to move the private methods in LT to the bottom of the file, but I think the problem is much larger than just your additions, so we will defer that.

I know you're working on the focus issue. Did you finish testing all of the focus testcases?

Greg Shah wrote:

I know you're working on the focus issue. Did you finish testing all of the focus testcases?

Not yet. I've spent some time on rebasing. Now I've resumed working on the problem I've described before.

It looks that the problem is due to the race condition. If there is a breakpoint on the second line of

apply "entry" to var2 in frame f.
message "Focus on entry: " + focus:name.

(more precisely its p2j conversion) then the problem disapears.

In fact this is understandable as apply is processed in a separate thread, but at this moment I do not see how to fix it.

Igor, the wait-for tests (and generally all tests which require testing of focus/event processing) need to be ran from the terminal, not from the 4GL procedure editor; this is because the 4GL procedure editor is a 4GL program itself, and the behaviour is altered by this fact. You need to run the program using:

pro -p wait-for_restore_focus1.p

If you run it like this, you will see that 4GL displays the NAME-related error, too.

Constantin Asofiei wrote:

Igor, the wait-for tests (and generally all tests which require testing of focus/event processing) need to be ran from the terminal, not from the 4GL procedure editor; this is because the 4GL procedure editor is a 4GL program itself, and the behaviour is altered by this fact. You need to run the program using:
[...]

If you run it like this, you will see that 4GL displays the NAME-related error, too.

Oh, I see. Thank you Constantin.

In fact this is understandable as apply is processed in a separate thread, but at this moment I do not see how to fix it.

I don't understand this statement. The apply is not executed on a separate thread. On the server, the apply will ultimately resolve down to calling the ThinClient.apply(). It will block while that call executes. That call may generate an event that fires a trigger on the server, but that would be a kind of recursion using the same Conversation thread that is executing the apply.

I'm unclear on whether this issue still needs to be worked. Does running the testcase from the command line (as suggested in note 117) eliminate this issue?

Greg Shah wrote:

I don't understand this statement. The apply is not executed on a separate thread. On the server, the apply will ultimately resolve down to calling the ThinClient.apply(). It will block while that call executes. That call may generate an event that fires a trigger on the server, but that would be a kind of recursion using the same Conversation thread that is executing the apply.

I'm unclear on whether this issue still needs to be worked. Does running the testcase from the command line (as suggested in note 117) eliminate this issue?

Well, I may be wrong about the separate thead. I will double check, but the fact that with a breakpoint there are no errors is still correct. Running the testcase from the command line does indeed reproduces the error message about the NAME attribute with 4GL, but p2j in addition generates a message (3135) as described in #2629 and the results are still different even with an 'old' version of the FocusManager.focusChange(). However this can be due to a fact that (3135) breaks the normal execution path (as far as I understand).

I'm analyzing now the results of the regression testing.

There where 4 runs of the regression tests (3 on devsrv01 and 1 on my local machine).

The following tests failed in all runs:

1. ctrlc_11_session1 - failed in the same way in all 4 runs. failure in step 18: 'Timeout while waiting for event semaphore to be posted.'
2. ctrlc_11_session3 - failed in all 4 runs but each time in a different way.
3. ctrlc_11_session4 - failed in the same way in all 4 runs. failure in step 28: timeout before the specific screen buffer became available (Mismatched data at line 0, column 0. Expected ' ' (0x0020 at relative Y 0, relative X 0) and found '┌' (0x250C at relative Y 0, relative X 0), template: screens/navigation/syman_menu.txt.)

From the main part the following tests failed in the same way in all 4 runs:

gso_195
gso_281
gso_289
gso_307
gso_394
gso_395
gso_405
tc_inquiry_inventory_017
tc_po_012
gc_64

Igor Skornyakov wrote:

From the main part the following tests failed in the same way in all 4 runs:

gso_195
gso_281
gso_289
gso_307
gso_394
gso_395
gso_405
tc_inquiry_inventory_017
tc_po_012
gc_64

Please investigate these, as they seem likely to be regressions.

BTW, before deciding to do another round of regression testing, is worth to check some of the failed tests manually - if the problem is still there, then you've found a regression in your changes, which needs to be fixed.

It appears the the failure of at least one test (gso_195) is is caused by a new "protection logic". Another workaround for the 'clean' state restore after the temporary focus chanage is implemented.

Committed to the branch 2567a, revno 10942.

Regression testing restarted.

Igor, just a heads up: the antivirus scanner was in progress (I've killed it now) and when this runs (sundays) this slows down a lot devsrv01 - so your harness testing might have lots of false negatives.

So, check the previous failures from note 121 first, once the testing completes.

BTW, have you checked the scenarios from note 121 with rev 10942 before starting testing?

Constantin Asofiei wrote:

Igor, just a heads up: the antivirus scanner was in progress (I've killed it now) and when this runs (sundays) this slows down a lot devsrv01 - so your harness testing might have lots of false negatives.

So, check the previous failures from note 121 first, once the testing completes.

BTW, have you checked the scenarios from note 121 with rev 10942 before starting testing?

Constantin,
I've started the regression testing both on devsrv01 and my machine and the results where almost identical and not good. I've checked the standalone scanarios before testing and there where no noticeable difference with what I saw before. The manual testing of gso_195 passed with my fixed.

After this fail I decided to try to suppress only the "protection logic". In this case my changes are just a reordering of an existed logic and this reordering only affects the case when the ON-LEAVE trigger returns NO-APPLY. The gso_195 and standalone scanarios passed fine and I've started regression testing (again both on devsrv01 and my machine). Unfortunately the results do not look encouraging.

In all these runs many tests failed in a similar way: a mismatched template screen has a bottom line like Enter data or press F4 to end. while in a test run is is something like Select if this clock to shift time variation is approved. (in gso_195) or vise versa.

I'm trying to understand this now.

Igor, please post the 4GL test which required the changes in FocusManager

Status line update is performed in the processing of FOCUS_GAINED/FOCUS_LOST events. These event are not fired in the processKeyEvent(entry) so an explicit firing is added.

Committed to the branch 2567a, revno 10944.

Regression testing restarted.

Igor Skornyakov wrote:

Status line update is performed in the processing of FOCUS_GAINED/FOCUS_LOST events. These event are not fired in the processKeyEvent(entry) so an explicit firing is added.

Committed to the branch 2567a, revno 10944.

I'm OK with the change as long as it doesn't duplicate trigger invocations.

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Status line update is performed in the processing of FOCUS_GAINED/FOCUS_LOST events. These event are not fired in the processKeyEvent(entry) so an explicit firing is added.

Committed to the branch 2567a, revno 10944.

I'm OK with the change as long as it doesn't duplicate trigger invocations.

According to my test they don't

Igor, if you are not rebasing at this time, I want to commit some cleanups/pushScreenDefinition changes I've made to branch 2567a - please let me know if this is OK.

Constantin Asofiei wrote:

Igor, if you are not rebasing at this time, I want to commit some cleanups/pushScreenDefinition changes I've made to branch 2567a - please let me know if this is OK.

Constantin,
I do not plan to rebase in the next few hours.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor, if you are not rebasing at this time, I want to commit some cleanups/pushScreenDefinition changes I've made to branch 2567a - please let me know if this is OK.

Constantin,
I do not plan to rebase in the next few hours.

The changes are in rev 10945.

After several runs of the regression tests some tests are still failing. I'm analyzing them now and at least some failures look unusual. In particular gso_281 fails at step 43 with the message timeout occurred before all output could be read (null bytes read). This is inside of a sequence of 18 VK_ENTERs w/o buffer validation. The manual run passed OK.

Some tests (gso_307, tc_inquiry_inventory_017) failed due to a different data. I guess this happened because some previous tests failed.

gso_394 and gso_395 failed because a message was not cleared after VK_ENTER.

Continue investigation.

gso_289 failed in the same way as gso_281 but manual run passed OK.

Regarding gso_307
In the template screen the bottom line is Press any key for corrections or ENTER to print
In the manual run I can see this line for a short time and after that another message is shown:
@ * ORDER NOT PRINTED *

Enter a valid Service Order
@

which causes the test failure

Rebased task branch 2567a from P2J trunk revision 10918 (new branch revision 10946).

Rebased task branch 2567a from P2J trunk revision 10919 (new branch revision 10947).

Rebased task branch 2567a from P2J trunk revision 10920 (new branch revision 10948).

Something strange is going on with the test gso_307
With branch 2567a tt consistently fails on step 40 with message 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), template: screens/gso/gso_307/stock_order_selection_screen_step11.txt.)

The screen in question is:
wait = true; millis = '180000'; failing screen =

08/11/2015                      INVENTORY INQUIRY                       17:09:23
┌──────────────────────────────── Item Master ─────────────────────────────────┐
│Item: 0A0001ERN       Mfg P/N: PS870B1/2-6OZ            On Hand:    1,112.000 │
│ Description: PS870B1/2-6OZ SEALANT 6 OZ #654 SEMKIT        U/M: EA           │
│   2nd Desc.: 5512694999 BMS5-95TY1CLB1-2 MIL-PRF81733 On Order:      240.000 │
│    Alt Item: BMS5┌─────── Stock Order Information ───────┐ Job:        0.000 │
│Product Code: PGC │                                       │ Qty:    1,112.000 │
│        Type: M (M│ Order #:                    Stat:     │tock:    1,188.000 │
│ Drawing Nbr:     │     S/O: 125329  Oper: 1     Rtg:     │able: no           │
│Hazmat Types: #$FH│ Qty Required:           1.00 EA       │Time:    6  days   │
│Current Site: GSO │ Pickup Local: H2   HANGAR 2           │otes: yes          │
└──────────────────│                                       │───────────────────┘
              ┌────└───────────────────────────────────────┘─────┐
              │Site Fac  Location             On Hand Expires MRB│
              │──── ──── ─────────────── ──────────── ─────── ───│
              │                                                  │
              │GSO       IC                     0.000         Yes│
              │GSO  A    STOCK                  0.000         Yes│
              │GSO  B    STOCK                  0.000         Yes│
              └──────────────────────────────────────────────────┘

 ***  ORDER NOT PRINTED  *** 

Enter a valid Service Order

In my manual tests however this value can be 240.000, 241.000, 242.000 and even 243.000.

Actually this value appears on the screen for the fisrt time at at step 22, but in this and several next step there is an exclude clause:

   <send-text value="1" special="VK_ENTER" />
   <check-screen-buffer filename="gso_307/stock_order_selection_screen_step8.txt" named-rectangle="screen" >
      <exclude named-rectangle="top_frame_date" />
      <exclude named-rectangle="top_frame_time" />
      <exclude left="73" right="73" top="4" bottom="4" />
   </check-screen-buffer>

Actually the failing step is the first one when this clause is not present.

What looks most strange is that this test passes with the trunk and fails consistently with 2567a, My guess is that this is due to some tests which failed before as in some manual runs of this test with a trunk the value is 240.000 as well.
However the only tests which failed before are gso_281 and gso_289 described earlier (these tests pass OK in a manual run, but fail consistently in an automated one).

Try to run single GSO 307 test after DB restore. What is the result?

In general if we are not sure the test does not change the DB we have to assume it does. So even if you run the single test(not included in some predefined test sequence) you need to restore DB again to have the clean result.

Eugenie Lyzenko wrote:

Try to run single GSO 307 test after DB restore. What is the result?

In general if we are not sure the test does not change the DB we have to assume it does. So even if you run the single test(not included in some predefined test sequence) you need to restore DB again to have the clean result.

Thank you Eugenie. With a "clean" DB the value is 240.000. Actually it seems that the problem is indeed a side effect of the failed gso_281. At least this test operates with the same item 0A0001ERN

It seems that there is a misprint in the com.goldencode.harness.test.SendKey. The format specification SPEC3 is:

   /** Another format specification for drain mode. */
   private static final String SPEC3 = "timeout occurred before all output " +
                                       "could be read (%d bytes read)";

and it is used as:

                  reason = String.format(SPEC3, reason, newCount);

which results in a confusing error message. I suggest to modify the format like this:

   private static final String SPEC3 = "timeout occurred before all output " +
                                       "could be read [%s] (%d bytes read)";

Agreed.

Please create a new task in the Harness project. Post this information, create a task branch and get this change tested. You can check it in and archive the branch as soon as you have successfully used it once.

Greg Shah wrote:

Agreed.

Please create a new task in the Harness project. Post this information, create a task branch and get this change tested. You can check it in and archive the branch as soon as you have successfully used it once.

OK. Actually I'm testing it now.

This is really strange but after (I'm not sure that because of) the harness test change (#2639) the results of the regression tests improved significantly. All tests which consistently failed before have passed now.
The failed tests now (in addition to the expected tc_job_002) are:
gso_418, tc_job_clock_002 and tc_job_clock_005.

As far as I remember the last two typically fail when runned too early in the day and the first one passed before.

I've started the regression test again.

Before you run testing again, perhaps it is a good time to rebase? I think if the next run goes well (and doesn't overlap the last run), then you can merge to trunk. But you need to have a test run that is on the rebased version. Best to stop your current run, rebase and restart testing.

Greg Shah wrote:

Before you run testing again, perhaps it is a good time to rebase? I think if the next run goes well (and doesn't overlap the last run), then you can merge to trunk. But you need to have a test run that is on the rebased version. Best to stop your current run, rebase and restart testing.

OK. Rebasing now.

Rebased task branch 2567a from P2J trunk revision 10921 (new branch revision 10949).

Regression tests restarted

Well, in this run there are no miracles. The tests which failed before failed again. However with the new version of the harness the situation with gso_281, gso_289 and gso_405 becomes clear. The new message is timeout occurred before all output could be read [null] (40 bytes read). The step settings are timeout = 15000; minimum = 40;. This is actually what I've expected. With the new "protection logic" less screen updates are generated on focus change. In the manual run it was not visible. I suggest to reduce the minimum value to 20. I will do it now and restart tests. If this will help the tests in the repository should be updated before merging 2567a.

I suggest to reduce the minimum value to 20. I will do it now and restart tests. If this will help the tests in the repository should be updated before merging 2567a.

Interesting. Yes, this seems like a reasonable idea. I hope that you are right!

Eugenie: can you see any concern with making this change?

After last changes in tests' parameters all main tests (except tc_job_002) passed OK. However in gso_ctrlc_3way_tests section all ctrlc_11_session* failed. They passed before, but I will try to run this part again.

I've noticed that NPE is frequently happens in the FillIn line 1211:

      @SuppressWarnings("unchecked")
      Frame<O> frame = (Frame<O>) UiUtils.locateFrame(this);

      if (frame.isInChoose(this))
      {
         frame.processKeyEvent(ke);
         return;
      }

The frame variable is used many times later in this method as well. May be it makes sense to add a check that it is not null?

ctrl-c tests passed. Can I commit 2567a and updated tests?
Thank you.

Rebased task branch 2567a from P2J trunk revision 10923 (new branch revision 10951).

Ready to merge, however the updated definition of the gso_281, gso_289, and gso_405 should be committed first.

Igor Skornyakov wrote:

I've noticed that NPE is frequently happens in the FillIn line 1211:

[...]

The frame variable is used many times later in this method as well. May be it makes sense to add a check that it is not null?

Do you have a stacktrace or a test which shows this?

Because a FILL-IN should never live on its own, it always must be attached to a frame.

Igor Skornyakov wrote:

Please find the log attached

And where did you see this? In MAJIC testing or somewhere else?

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Please find the log attached

And where did you see this? In MAJIC testing or somewhere else?

This was a standard MAJIC regression testing.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Please find the log attached

And where did you see this? In MAJIC testing or somewhere else?

This was a standard MAJIC regression testing.

Were there some tests failed with this issue? Is it from the CTRL-C part? Because I don't recall seeing this NPE before in the MAJIC testing.

Constantin Asofiei wrote:

Were there some tests failed with this issue? Is it from the CTRL-C part? Because I don't recall seeing this NPE before in the MAJIC testing.

I'm not sure. In this run tc_job_002 and all three all ctrlc_11_session* tests failes. In the next run all ctrl-c tests passed. I do not mean that NPE happens in every test run, but I see it frequently.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Were there some tests failed with this issue? Is it from the CTRL-C part? Because I don't recall seeing this NPE before in the MAJIC testing.

I'm not sure. In this run tc_job_002 and all three all ctrlc_11_session* tests failes. In the next run all ctrl-c tests passed. I do not mean that NPE happens in every test run, but I see it frequently.

Have you seen it before testing branch 2567b? Because looks like a regression related to some focus issues - it might use a widget for which it's frame has already been destroyed.

Constantin Asofiei wrote:

Have you seen it before testing branch 2567b? Because looks like a regression related to some focus issues - it might use a widget for which it's frame has already been destroyed

I don't remember. And in fact all tests eventually passed. Please note also the the logic of the widget choice was not changed at least by me, only the sequence of events.

Igor, please change the TC.processEventsWorker:14596 so that the NPE is re-thrown, not consumed. After this, re-start testing - this will allow you to find which test generated the NPE. Currently, the NPE was consumed thus the test managed to pass... but this NPE should never have happened.

         catch (NullPointerException npe)
         {
            throw npe;
         }

         catch (Exception e)
         {
            e.printStackTrace(System.err);
            continue;
            // throw new RuntimeException("")
         }

Constantin Asofiei wrote:

Igor, please change the TC.processEventsWorker:14596 so that the NPE is re-thrown, not consumed. After this, re-start testing - this will allow you to find which test generated the NPE. Currently, the NPE was consumed thus the test managed to pass... but this NPE should never have happened.
[...]

OK. I will to this. However the tests should be started later on today as some tests usually fail if started too early in the day which will distort the picture.

Added NPE rethrow as per Constantin's suggestion.

Committed to the branch 2567a, revno 10952.

Regression test restarted

The failed test is gso_216. The Browse.nextFocus() detaches current focus which is not correct for a temporary focus change. A "protection logic" was added.

Committed to the branch 2567a, revno 10953.

Regression test restarted

Igor Skornyakov wrote:

Committed to the branch 2567a, revno 10953.

1. Browse.java:
   - you need to merge the history entries so that only H110 remains, i.e.:
   

   ** 110 IAS 20150801          TAB-STOP attribute support
                                Added protection logic for temp focus change.
   

   
   - nextFocus: when the IF condition doesn't fit the 98 width, we left-align the conditions and also right-align the operators:
   

         if (isEditPossible()          && 
             getActiveEditor() != null && 
             !ThinClient.getInstance().getFocusManager().isTempFocusChange())
   

Stanislav, this change is related to browse widget in editing mode; can you suggest a test to check if editing browse works OK?

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Committed to the branch 2567a, revno 10953.

Review rev 10953:

1. Browse.java:
   - you need to merge the history entries so that only H110 remains, i.e.:
   [...]
   - nextFocus: when the IF condition doesn't fit the 98 width, we left-align the conditions and also right-align the operators:
   [...]

Fixed.
Committed to the branch 2567a, revno 10954.

Stanislav, this change is related to browse widget in editing mode; can you suggest a test to check if editing browse works OK?

For example:

def temp-table tt
               field f1 as integer
               field f2 as character.

def var i as integer.
repeat i = 1 to 11:
   create tt. tt.f1 = i. tt.f2 = string(i * 10). 
end.

def buffer xtt for tt.

def query q for tt.
def browse brws query q
     display tt.f1 
             tt.f2
     enable all
             with 6 down.
def button btn1.

on "x" anywhere do:
  message "update" update val as integer.
end.

open query q for each tt.
display brws btn1 with frame fr.
enable brws btn1 with frame fr.

wait-for close of current-window.

I've checked, it works.

A "protection logic" added to the Browse.nextFocus() was too rude. A more delicate version was implemented.

Committed to the branch 2567a, revno 10955.

Regression test restarted

Igor Skornyakov wrote:

A "protection logic" added to the Browse.nextFocus() was too rude. A more delicate version was implemented.

Committed to the branch 2567a, revno 10955.

Regression test restarted

I'm not sure this is the best approach. The call path to reach stopEdit - where you made the change - is like this, I think:

nextFocus()
goDown(boolean)
goDown(boolean, boolean)
leaveRow()
releaseCurrentRow()
releaseCurrentRow(boolean)
stopEdit(boolean)

When the is made while browse editing mode on, goDown will end calling startEdit, which will create a new widget, but stopEdit will not de-register the widget. So we might have a widget leak.

If this is a temporary focus change, why not just ignore the goDown when nextFocus is called; or you need some logic in goDown to be executed?

      if (isEditPossible() && getActiveEditor() != null)
      {
         if (!ThinClient.getInstance().getFocusManager().isTempFocusChange())
         {
            goDown(true);
            repaint();
         }
      }
      else
         parent().nextFocus();

Constantin Asofiei wrote:

Igor Skornyakov wrote:

A "protection logic" added to the Browse.nextFocus() was too rude. A more delicate version was implemented.

Committed to the branch 2567a, revno 10955.

Regression test restarted

I'm not sure this is the best approach. The call path to reach stopEdit - where you made the change - is like this, I think:
[...]

When the is made while browse editing mode on, goDown will end calling startEdit, which will create a new widget, but stopEdit will not de-register the widget. So we might have a widget leak.

If this is a temporary focus change, why not just ignore the goDown when nextFocus is called; or you need some logic in goDown to be executed?
[...]

Unfortunatelly w/o down() the test gso_216 fails. This was the reason for tha last change. I was thinking about destroying the widget, but I had a hope that it is done on a focus lost as well (not only on nextFocus()). I will think how to deal with that.

Well, actually my last change appears to be wrong as well. gso_216 fails anyway but at a later step. So I decided to limit the "protection logic" to just suppressing assigning null to the parent field. After all it is all about NPE.

Committed to the branch 2567a, revno 10956.

Regression test restarted

Igor Skornyakov wrote:

Well, actually my last change appears to be wrong as well. gso_216 fails anyway but at a later step. So I decided to limit the "protection logic" to just suppressing assigning null to the parent field. After all it is all about NPE.

Committed to the branch 2567a, revno 10956.

Regression test restarted

The protection logic doesn't seem OK... I wonder if we really know the root cause for this yet.

For editing browse, as I can tell the FILL-IN widgets are created and destroyed as the user navigates the browse. I'm looking into GSO 216 to see if I can find something.

Constantin Asofiei wrote:

Igor Skornyakov wrote:
The protection logic doesn't seem OK... I wonder if we really know the root cause for this yet.

For editing browse, as I can tell the FILL-IN widgets are created and destroyed as the user navigates the browse. I'm looking into GSO 216 to see if I can find something.

I completely agree with you. I understand your point about creating/destroying FILL-IN widgets and my last change (hopefully) do not break this. However gso_216 still fails. I'm also investigating.

1. it performs a temporary focus change - for browse case, this destroys the current editing fill-in (oldFocus) and creates a new one (inFocus)
2. later on, after the temp focus change, focus is requested by inFocus
3. lastly, oldFocus is being sent a LEAVE event, but at this time the oldFocus FILL-IN (for the editing browse case) has already been destroyed. The difference from the previous code is that LEAVE was being sent BEFORE the focus change, thus the widget was still alive when LEAVE was being processed.
4. another issue with Browse.goDown is that stopEdit and startEdit destroy and re-create a widget with the same config ID (this is because the browse model has an editing fill-ins per each column, no matter how many rows can be edited - I think this is because is assumed only a certain row can be in "editing mode" at a time, so there is no need to have "per-row" editing fill-ins). For this reason, we can't use ThinClient.getWidget(oldFocus.id) to determine if the widget is still alive or not. Instead, we need to check if the widget is still attached to a parent in FocusManager.focusChange:1286:
   

         // for browse editing fill-in, the oldFocus fill-in gets destroyed during temporary focus 
         // change by Browse.goDown.  As the browse uses the same fill-in ID for each row during 
         // editing, we need to check if the oldFocus is still attached to a parent; TC.getWidget
         // can not be used because it will return inFocus (which has the same config ID as oldFocus).
         if (oldFocus != null && oldFocus.parent() != null)
   

   
   I think is OK to bypass this LEAVE event as AFAIK it is not possible to have 4GL triggers for the browse editing fill-in - Stanislav, please confirm this.

I've added this change to branch 2567a rev 10957. Please do a runtime testing with it.

Constantin Asofiei wrote:

Some notes about how FocusManager.focusChange works:

1. it performs a temporary focus change - for browse case, this destroys the current editing fill-in (oldFocus) and creates a new one (inFocus)
2. later on, after the temp focus change, focus is requested by inFocus
3. lastly, oldFocus is being sent a LEAVE event, but at this time the oldFocus FILL-IN (for the editing browse case) has already been destroyed. The difference from the previous code is that LEAVE was being sent BEFORE the focus change, thus the widget was still alive when LEAVE was being processed.
4. another issue with Browse.goDown is that stopEdit and startEdit destroy and re-create a widget with the same config ID (this is because the browse model has an editing fill-ins per each column, no matter how many rows can be edited - I think this is because is assumed only a certain row can be in "editing mode" at a time, so there is no need to have "per-row" editing fill-ins). For this reason, we can't use ThinClient.getWidget(oldFocus.id) to determine if the widget is still alive or not. Instead, we need to check if the widget is still attached to a parent in FocusManager.focusChange:1286:
   [...]
   I think is OK to bypass this LEAVE event as AFAIK it is not possible to have 4GL triggers for the browse editing fill-in - Stanislav, please confirm this.

I've added this change to branch 2567a rev 10957. Please do a runtime testing with it.

Thank you Constantin. Unfortunately it seems that NPE still happens (in a manual test run). Investigating.

Well, it seems that I did something wrong in a hurry. The manual test passed OK.
Restarted regression test.

In this run all gso_tests passed. Some tc_tests failed (tc_gl_rpts_007, tc_codes_employees_010, tc_codes_employees_021, tc_job_002, tc_job_clock_005, tc_job_clock_002).
Restarted regression test.

All regression tests except tc_job_002 passed (not all in one run, but the intersection of sets of failed tests in 3 runs is only tc_job_002).

Igor Skornyakov wrote:

All regression tests except tc_job_002 passed (not all in one run, but the intersection of sets of failed tests in 3 runs is only tc_job_002).

Please rebase and I'll do a quick review after that.

Rebased task branch 2567a from P2J trunk revision 10926 (new branch revision 10960).

I assume the conversion testing has passed? If so, you can merge branch 2567a and also commit the baseline changes - make sure to commit them to the "staging" tag and also do a git push origin staging.

Merged to trunk rev. 10927

Created task branch 2567b

Rebased task branch 2567b from P2J trunk revision 10930 (new branch revision 10931).

Please make a list of the remaining work on this task and an estimate of when it can be completed.

Greg Shah wrote:

Please make a list of the remaining work on this task and an estimate of when it can be completed.

Sure. I will do this today.

Remaining tasks:

1. the support for the WINDOW:FRAME-Z-ORDER attribute and MOVE-TO-BOTTOM/MOVE-TO-TOP methods - browse, button, combo-box, editor, fillin, frame, image, literal, radio-set, rectangle, selection-list, slider, text, toggle-box, window (runtime only) - will be finished at the weekend at the latest

2. SELECTABLE, AUTO-RESIZE, MENU-MOUSE attributes - will be finished by the end (hopefully by the middle) of the next week.

I assume you are writing many testcases to check behavior. Please make sure you are checking in your testcases to testcases/uast/<some_path>/.

Thank you very much Greg!

Please check in your testcases. This will help me understand the research you are doing.

When do you expect to have the rules for z-order determined? Please do document those rules here.

Greg Shah wrote:

Please check in your testcases. This will help me understand the research you are doing.

When do you expect to have the rules for z-order determined? Please do document those rules here.

In fact at this moment I'm about to finish runtime support for the case with frames. I understand it is the main one. It will be committed to the task brank in about an hour. I plan at least to have a full test set and document the behavior tomorrow. Hopefully the runtime support will be done as well.

In fact at this moment I'm about to finish runtime support for the case with frames. I understand it is the main one.

It may be that originally, frames were the most important feature. But the application code we are converting has been changed to heavily use features like rectangles and images. I believe we need to handle all classes of widgets in a very comprehensive manner.

Greg Shah wrote:

It may be that originally, frames were the most important feature. But the application code we are converting has been changed to heavily use features like rectangles and images. I believe we need to handle all classes of widgets in a very comprehensive manner.

I understand that. Actually what I mean to say is that it is the most important case for me to find all places in the code which must be changed and to understand how exactly this should be done.

I understand that.

OK, sorry I misunderstood.

Sorry, please disregard the previous post.

Implemented MOVE-TO-TOP()/MOVE-TO-BOTTOM() for frames.
Committed to branch 2567b revno 10932

Added MOVE-TO-TOP/MOVE-TO-BOTTOM for frames to testcases/uast/frame-z-order. revno 1316

With 4GL the frame fr1 (see attached sample) is not VISIBLE (both in ChUI and Windows modes) while with p2j it is.

Please describe the recreate steps.

Greg Shah wrote:

With 4GL the frame fr1 (see attached sample) is not VISIBLE (both in ChUI and Windows modes) while with p2j it is.

Please describe the recreate steps.Just start the program. At the first step the fr1 will be visible but after the fist pause it will dissapear (with 4GL). With p2j it will be always visible.

It looks that the value of the FRAME:TOP-ONLY attribute affects only the inital visibility of the frame. Neither switching between overlapping frames nor MOVE-TO-TOP()/MOVE-TO-BOTTOM() methods are affected.

Igor Skornyakov wrote:

It looks that the value of the FRAME:TOP-ONLY attribute affects only the inital visibility of the frame. Neither switching between overlapping frames nor MOVE-TO-TOP()/MOVE-TO-BOTTOM() methods are affected.

1. Is this true for ChUI and GUI?

2. Is this the cause of the issue reported in note 204?

In the z2.p program, the a, b, c, x, y and z ANYWHERE triggers are not working. It is hard to really test how things work without those triggers.

Greg Shah wrote:

1. Is this true for ChUI and GUI?

Yes, it is.

2. Is this the cause of the issue reported in note 204?

No, the issue is that with p2j the TOP-ONLY does not affect the initial visibility of the frame.

Greg Shah wrote:

In the z2.p program, the a, b, c, x, y and z ANYWHERE triggers are not working. It is hard to really test how things work without those triggers.

They are working, the triggers use uppercase letters, A, B, ...

the triggers use uppercase letters

Good point, thanks.

No, the issue is that with p2j the TOP-ONLY does not affect the initial visibility of the frame.

Please do fix this as part of this task.

Greg Shah wrote:

No, the issue is that with p2j the TOP-ONLY does not affect the initial visibility of the frame.

Please do fix this as part of this task.

Thank you. I will fix it. It should not be difficult.

Added conversion support for the OVERLAY and TOP-ONLY attributes (was for the corresponding options only).

Committed to branch 2567b revno 10934

Have you tested your updated tooltips implementation with all the current widget types? If so, does it work generically (except for the custom code for browse and frame)?

Greg Shah wrote:

Have you tested your updated tooltips implementation with all the current widget types? If so, does it work generically (except for the custom code for browse and frame)?

I cannot say that I've extensively tested it with all widgets. I planned to do it in parallel with regression testing.

I planned to do it in parallel with regression testing

OK.

I have a question. What is the right interface for the TOP-ONLY/OVERLAY attributes support? I've added the corresponding method to the CommonFrame but is seems to be not exactly correct.
Thank you.

OVERLAY should be in FrameInterface.

TOP-ONLY will need a new interface that can contain attrs and methods that are only shared by WindowWidget and FrameWidget.

I was thinking about the FrameInterface but the GenericFrame where setTopOnly and setOverlay methods are implemented now does not implement it. I will try to understand the GenericFrame/FrameWidget construction better.

I think that the similar problem with e.g. FRAME:LINE attribute (currently not supported).

Revised conversion support for the OVERLAY and TOP-ONLY attributes.

Committed to branch 2567b revno 10935

Actually the 4GL behavior is a a little bit weird. If we have two overlapping frames fr1 and fr2 then fr1 is visible not onle if it has an OVERLAY option set but even when is has not but fr2 has. This looks counterintuitive for me.

This looks counterintuitive for me.

I think this is simply the fact that by default the 4GL does auto-hiding of a frame when another frame is made visible in the same space (or any overlap).

OVERLAY disables this auto-hiding behavior.

Greg Shah wrote:

This looks counterintuitive for me.

I think this is simply the fact that by default the 4GL does auto-hiding of a frame when another frame is made visible in the same space (or any overlap).

OVERLAY disables this auto-hiding behavior.

Yes, but I expected that this logic is based on the value of the OVERLAY attribute of the frame which is overlapped, not the one which overlaps.

I see.

OVERLAY means that the frame with OVERLAY option or OVERLAY attribute set true "can overlay" other frames.

Greg Shah wrote:

I see.

OVERLAY means that the frame with OVERLAY option or OVERLAY attribute set true "can overlay" other frames.

Exactly! Thanks a lot. I was confused in a very stupid way. Thank you!

Rebased task branch 2567b from P2J trunk revision 10935 (new branch revision 10940).

Fixed the issue with OVERLAY/TOP-ONLY combination (see note 204 above).

Committed to branch 2567b revno 10941

You have been working on z-order since Aug 27th but I only see 1 testcase in the testcases/uast/frame-z-order/ directory. You must have a large number of testcases at this point. Please check them all into testcases/uast/ so that I can see the details of your research.

I would also like you to please document the results here.

We MUST get this task done ASAP. It is months behind. When do you estimate to have everything done?

Greg Shah wrote:

You have been working on z-order since Aug 27th but I only see 1 testcase in the testcases/uast/frame-z-order/ directory. You must have a large number of testcases at this point. Please check them all into testcases/uast/ so that I can see the details of your research.

I would also like you to please document the results here.

We MUST get this task done ASAP. It is months behind. When do you estimate to have everything done?

Greg,
I'm working on this right now. I will commit all z-order tests and the final version of the code tomorrow.

1. Commited tests uast/frame-z-order/z2.p and uast/frame-z-order/z3.p revno 1323/
2. When running converted z2.p in gui_swing mode there are multiple error messages in the client log loke the following:
Received mouse event for zombie source -50: java.awt.event.MouseEvent[MOUSE_MOVED,(70,62),absolute(999,264),clickCount=0] on com.goldencode.p2j.ui.client.gui.driver.swing.SwingEmulatedWindow$1[,0,0,408x540,alignmentX=0.0,alignmentY=0.0,border=,flags=0,maximumSize=java.awt.Dimension[width=408,height=540],minimumSize=java.awt.Dimension[width=408,height=540],preferredSize=java.awt.Dimension[width=408,height=540]]

The investigation shows that the problem is with the instance of the LabelGuiImpl. As far as I understand such a widgets are never registered in the WidgetRegistry. Assuming that this was done on purpose I've changes a bit the logic of the search for valid parent in the TitledWidow.processEvent() method. After this the error message have disappeared and the converted app started working.

Committed to the branch 2567b revno 10942
3. However there is an exception java.lang.RuntimeException: Invalid frame ID 8 on the exit with the converted z2.p in the gui_swing mode. This happens on the MOVE_TO_TOP()/MOVE_TO_BOTTOM() in z3.p. Investigating.

Fixed problems with Invalid frame ID. There where 2 different reasons for z2.p and z3.p.

Committed to the branch 2567b revno 10943

Commited test uast/frame-z-order/z4.p revno 1324 - RECTANCLE and IMAGE

The MOVE-TO-TOP()/MOVE-TO-BOTTOM() methods when applied to widgets other than FRAME and WINDOW their z-order is changed within the frame. The RECTANGLE and IMAGE widgets are always stay behind other widgets and the methods change their z-order only within the class of these widgets. This behavior doesn't depend on the value of the KEEP-FRAME-Z-ORDER attribute of the enclosing windows - it affects frames only.
The MOVE-TO-TOP()/MOVE-TO-BOTTOM() methods when applied to widgets other than FRAME and WINDOW doesn't affect the z-order of the enclosing frame. When widgets' z-order is changed via MOVE-TO-TOP()/MOVE-TO-BOTTOM() methods the widgets' labels are not affected.

Greg,
I suggest to add a 'marker interface' or annotation to explicitely distinguish between those p2j widgets which are direct counterparts to 4GL ones and p2j internals. This will obviously not affect the functionality but will greatly simplify the code navigation especially with IDE. This can also can be useful in the implemetation of the logic where different kinds of widgets are processed (for this purpose the interface is more convenient).
What do you think about this?
Igor.

Igor Skornyakov wrote:

Greg,
I suggest to add a 'marker interface' or annotation to explicitely distinguish between those p2j widgets which are direct counterparts to 4GL ones and p2j internals. This will obviously not affect the functionality but will greatly simplify the code navigation especially with IDE. This can also can be useful in the implemetation of the logic where different kinds of widgets are processed (for this purpose the interface is more convenient).
What do you think about this?
Igor.

1. for server-side widgets, they are placed in the com.goldencode.p2j.ui package. The widget classes which have a 4GL counterpart have their name ending with the Widget suffix and also have a LegacyResource annotation. They are all derived from the GenericWidget class. GenericFrame is a special class which contains frame-related APIs and is used by the server-side runtime to define a frame, which accesses GenericFrame via an anonymous proxy. The frame widget is defined in the FrameWidget class.
2. the widget configuration classes all end with the Config and are all derived from the WidgetConfig class.
3. for client-side, the base classes for widgets reside in the com.goldencode.p2j.ui.client package. They are all derived from the AbstractWidget class. The implementation classes for GUI follow the <widget>GuiImpl format (i.e. ButtonGuiImpl), and for ChUI either <widget>Impl or <widget>ChuiImpl (this format must be followed for all new ChUI widget classes).
4. for GUI, the implementation classes reside in the p2j.ui.client.gui package
5. for ChUI, the implementation classes reside either in the p2j.ui.chui package or (for new widgets) in p2j.ui.client.chui package. No new classes can be added to p2j.ui.chui, we expect to move all the ChUI specific implementations from there to p2j.ui.client.chui.

Hope this helps.

Constantin Asofiei wrote:

There are some name conventions we have for the widgets:

1. for server-side widgets, they are placed in the com.goldencode.p2j.ui package. The widget classes which have a 4GL counterpart have their name ending with the Widget suffix and also have a LegacyResource annotation. They are all derived from the GenericWidget class. GenericFrame is a special class which contains frame-related APIs and is used by the server-side runtime to define a frame, which accesses GenericFrame via an anonymous proxy. The frame widget is defined in the FrameWidget class.
2. the widget configuration classes all end with the Config and are all derived from the WidgetConfig class.
3. for client-side, the base classes for widgets reside in the com.goldencode.p2j.ui.client package. They are all derived from the AbstractWidget class. The implementation classes for GUI follow the <widget>GuiImpl format (i.e. ButtonGuiImpl), and for ChUI either <widget>Impl or <widget>ChuiImpl (this format must be followed for all new ChUI widget classes).
4. for GUI, the implementation classes reside in the p2j.ui.client.gui package
5. for ChUI, the implementation classes reside either in the p2j.ui.chui package or (for new widgets) in p2j.ui.client.chui package. No new classes can be added to p2j.ui.chui, we expect to move all the ChUI specific implementations from there to p2j.ui.client.chui.

Hope this helps.

Thank you Constantin. I think I understand these conventions. However there are many classes in the packages you've mentioned which are not 4GL counterparts. When there is a need to quickly look at all counterparts it is still takes time for me. However it is not that important of course.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

There are some name conventions we have for the widgets:

1. for server-side widgets, they are placed in the com.goldencode.p2j.ui package. The widget classes which have a 4GL counterpart have their name ending with the Widget suffix and also have a LegacyResource annotation. They are all derived from the GenericWidget class. GenericFrame is a special class which contains frame-related APIs and is used by the server-side runtime to define a frame, which accesses GenericFrame via an anonymous proxy. The frame widget is defined in the FrameWidget class.
2. the widget configuration classes all end with the Config and are all derived from the WidgetConfig class.
3. for client-side, the base classes for widgets reside in the com.goldencode.p2j.ui.client package. They are all derived from the AbstractWidget class. The implementation classes for GUI follow the <widget>GuiImpl format (i.e. ButtonGuiImpl), and for ChUI either <widget>Impl or <widget>ChuiImpl (this format must be followed for all new ChUI widget classes).
4. for GUI, the implementation classes reside in the p2j.ui.client.gui package
5. for ChUI, the implementation classes reside either in the p2j.ui.chui package or (for new widgets) in p2j.ui.client.chui package. No new classes can be added to p2j.ui.chui, we expect to move all the ChUI specific implementations from there to p2j.ui.client.chui.

Hope this helps.

Thank you Constantin. I think I understand these conventions. However there are many classes in the packages you've mentioned which are not 4GL counterparts. When there is a need to quickly look at all counterparts it is still takes time for me. However it is not that important of course.

If you use Eclipse, there is the "Type Hierarchy" view (via F4 on an interface/class) and it shows you the entire class tree derived from your selection. Just make sure you don't select Object :).

Constantin Asofiei wrote:

If you use Eclipse, there is the "Type Hierarchy" view (via F4 on an interface/class) and it shows you the entire class tree derived from your selection. Just make sure you don't select Object :).

Yes, I use Eclipse and of course I use F4 (and CTRL-T) extensively. However this doesn't help if I'm looking for 4GL counterparts only.
With the marker interface F4 will give immediate result.

Are the problems with Invalid frame ID the same as those reported in #2700?

When widgets' z-order is changed via MOVE-TO-TOP/MOVE-TO-BOTTOM methods the widgets' labels are not affected.

Please elaborate. Do you mean that the labels do not change z-order? Or do you mean that the labels "stay with" their respective widgets (e.g. they match the z-order changes of the widget to which they are attached)?

Igor,

When widgets' z-order is changed via MOVE-TO-TOP/MOVE-TO-BOTTOM methods the widgets' labels are not affected.

If you want to access the side-labels, use the SIDE-LABEL-HANDLE attribute at the widget.

Greg Shah wrote:

Are the problems with Invalid frame ID the same as those reported in #2700?

Yes, it looks like that.

I understand that both bugs do not fit naturally into the scope of this task. Should I create 2 separate independent tasks?

Yes, please do.

Greg Shah wrote:

When widgets' z-order is changed via MOVE-TO-TOP/MOVE-TO-BOTTOM methods the widgets' labels are not affected.

Please elaborate. Do you mean that the labels do not change z-order? Or do you mean that the labels "stay with" their respective widgets (e.g. they match the z-order changes of the widget to which they are attached)?

I mean that if e.g. FILL-IN g initially stays atop of FILL-IN f then only the label of the first FILL-IN is visible. When f is moved on the top of g then there is still label of g which is visible.

Greg Shah wrote:

I understand that both bugs do not fit naturally into the scope of this task. Should I create 2 separate independent tasks?

Yes, please do.

Thank you. I will do it later today as I want to create simple tests illustrating these issues. The initail z4.p contains too many unneeded and distracting code.

In regard to the TitledWindow.processEvents() change in revision 10942, does the zombie event message actually stop you from executing the testcase? Or does it cause other problems?

If so, then it is OK to use this as a temporary fix for testing. But those messages are an indication of an issue elsewhere, so we really don't want to permanently suppress them with your change. We want to leave the logging there because if we see an zombie source messages, it may indicate a leak in label (de)registration.

Greg Shah wrote:

In regard to the TitledWindow.processEvents() change in revision 10942, does the zombie event message actually stop you from executing the testcase? Or does it cause other problems?

If so, then it is OK to use this as a temporary fix for testing. But those messages are an indication of an issue elsewhere, so we really don't want to permanently suppress them with your change. We want to leave the logging there because if we see an zombie source messages, it may indicate a leak in label (de)registration.

I understand that this not the final solution. But it prevented my test from running normally.

Implemented MOVE-TO-TOP()/MOVE-TO-BOTTOM() for all widgets except WINDOW

Committed to the branch 2567b revno 10944

Igor, forgot to mention something: there is a change related to moveToTop/Bottom/BeforeTab in 1811q WindowGuiImpl - I've just overridden the base APIs so that the call is dispatched to workspace.getContentPane(), as the widgets in GUI are in the workspace, not directly attached to the window (as ChUI does). Please take a look and see if it helps your implementation.

Constantin Asofiei wrote:

Igor, forgot to mention something: there is a change related to moveToTop/Bottom/BeforeTab in 1811q WindowGuiImpl - I've just overridden the base APIs so that the call is dispatched to workspace.getContentPane(), as the widgets in GUI are in the workspace, not directly attached to the window (as ChUI does). Please take a look and see if it helps your implementation.

Thank you Constantin. Where can I find your changes?

Fixed minor bug in MOVE-TO-TOP()/MOVE-TO-BOTTOM().
Committed to branch 2567b revno 10945

The existing test window_parenting/waitfor_2wnd.p doesn't start.

java.lang.RuntimeException: No renderer is registered for id = 12
    at com.goldencode.p2j.ui.client.gui.driver.GuiPrimitivesImpl.getWindowEmulator(GuiPrimitivesImpl.java:203)
    at com.goldencode.p2j.ui.client.gui.driver.GuiPrimitivesImpl.selectWindow(GuiPrimitivesImpl.java:270)
    at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.selectWindow(AbstractGuiDriver.java:1353)
    at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.stackWindows(AbstractGuiDriver.java:1573)
    at com.goldencode.p2j.ui.client.gui.WindowGuiImpl.hide(WindowGuiImpl.java:244)
    at com.goldencode.p2j.ui.client.WindowManager.clearWindowList(WindowManager.java:230)
    at com.goldencode.p2j.ui.chui.ThinClient.terminate(ThinClient.java:2846)
    at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:280)
    at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:100)
    at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:201)
    at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:398)
    at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:95)
    at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:267)

Should I try to fix that?
Thank you.

Modified test frame-z-order/z4.p, added frame-z-order/zw.p - for WINDOW:(MOVE-TO-TOP()/MOVE-TO-BOTTOM()).

Commited to testcases, revno 1326

Igor Skornyakov wrote:

The existing test window_parenting/waitfor_2wnd.p doesn't start.

[...]

Should I try to fix that?
Thank you.

No. Please create a new task.

Greg Shah wrote:

Igor Skornyakov wrote:

The existing test window_parenting/waitfor_2wnd.p doesn't start.

[...]

Should I try to fix that?
Thank you.

No. Please create a new task.

Thank you. I will. However it is a stopper for my work on WINDOWS:(MOVE-TO-TOP()/MOVE-TO-BOTTOM(). I'm working on other attributes now.

However it is a stopper for my work on WINDOWS:(MOVE-TO-TOP/MOVE-TO-BOTTOM.

OK, in that case you must fix this as part of this task. Don't bother creating a new one.

Greg Shah wrote:

However it is a stopper for my work on WINDOWS:(MOVE-TO-TOP/MOVE-TO-BOTTOM.

OK, in that case you must fix this as part of this task. Don't bother creating a new one.

I see. But I've already created the task (#2716). Please delete or close it.
Thank you.

Added support for the SELECTABLE attribute conversion.

Committed to branch 2567b revno 10946

Igor Skornyakov wrote:

The existing test window_parenting/waitfor_2wnd.p doesn't start.

[...]

Should I try to fix that?
Thank you.

In fact the Exception happens only at the program termination. At the startup I see only one window with "Procedure complete. Press space bar to continue." at the bottom.

Investigating.

Rebased task branch 2567a from P2J trunk revision 10936 (new branch revision 10948).

I've found a workaround for the issue mentioned in the note 263. If there are enabled frame(s) in the default window everything works. I've added a new test uast/frame-z-order/zw1.p.
Now I can resume working on the implementation of the WINDOW:(MOVE-TO-TOP/MOVE-TO-BOTTOM).
I will try to fix the issue with window_parenting/waitfor_2wnd.p after finishing with the the intitial scope of the task.
Is it OK?

I will try to fix the issue with window_parenting/waitfor_2wnd.p after finishing with the the intitial scope of the task.

Don't worry about this bug. We will resolve it later in #2716.

Greg Shah wrote:

I will try to fix the issue with window_parenting/waitfor_2wnd.p after finishing with the the intitial scope of the task.

Don't worry about this bug. We will resolve it later in #2716.

Thank you Greg.

Please make a list of the remaining work in this task and an estimate of when it can be finished.

The remaining work for this task
1. WINDOW:MOVE-TO-TOP/MOVE-TO-BOTTOM - will finish today (hopefullly in a couple of hours)
2. SELECTABLE attribute. The conversion support is done. Should not take much time if I understand the semantics correctly, I hope to finish with it today
4. MENU-MOUSE - looks simple, should be finished tomorrow.
4. AUTO-RESIZE - Should be simple if the default behavior is already implemented. In this case it will be done by the end of the week at latest.

Of course I will work at the weekend if required.

Please note that the WINDOW:TITLE attribute is ignored now. It is always P2J GUI Client.

Igor Skornyakov wrote:

Please note that the WINDOW:TITLE attribute is ignored now. It is always P2J GUI Client.

This is a known issue. See #2706.

Greg Shah wrote:

Igor Skornyakov wrote:

Please note that the WINDOW:TITLE attribute is ignored now. It is always P2J GUI Client.

This is a known issue. See #2706.

I see. Sorry.

No apologies are needed. I would rather know about problems and if we have duplicate reports that is OK.

I do not see any data structure which reflects windows' z-order. The only place where references to all windows are kept is GuiPrimitivesImpl.emulators which is Map. Have I missed something?

Thank you.

On the WINDOW:MOVE-TO-TOP invocation 4GL redraws only the affected window (the z-order of other windows with respect to othe OS windows remains the same).
On the WINDOW:MOVE-TO-BOTTOM invocation 4GL redraws all application's windows and they stay atop of the other OS windows which looks natural.

Well, at least part of the support is in WindowManager.moveToTop(), WindowManager.moveToBottom() and the WindowManager.WorkArea.windows list (where index 0 is the bottom of the z-order).

The problem I see is that these methods just edit the list, but they don't make the changes visible to the user.

For the driver level, the implementation differs. There is no data structure to edit. But in the trunk revision 10935, there is EmulatedWindowState.moveToTop() and AbstractGuiDriver.stackWindows().

In task branch 1811q we have changed this approach to eliminate EmulatedWindowState.moveToTop() (actually it is still there for the SwingEmulatedWindow subclass but wasn't needed for the web client. We also modified how stackWindows() works such that there is a driver-specific worker named restackWindows() that handles the implementation details.

Swing doesn't directly expose the operating system level control over z-order. So our SwingGuiDriver must "play some games" to manage the z-order. That means that the restackWindows() is very inefficient for Swing so I prefer not to use it if we can implement a more specific moveToTop() and moveToBottom() that can be implemented efficiently.

Task branch 1811q is meant to be merged to trunk before 2567b. For now, I think you should only call the WindowManager.moveToTop() and WindowManager.moveToBottom() as your "workers".

In 1811q, we will update WindowManager.moveToTop() and WindowManager.moveToBottom() to make the changes visible.

Constantin/Hynek: what do you think about this?

Greg Shah wrote:

Well, at least part of the support is in WindowManager.moveToTop(), WindowManager.moveToBottom() and the WindowManager.WorkArea.windows list (where index 0 is the bottom of the z-order).

The problem I see is that these methods just edit the list, but they don't make the changes visible to the user.

For the driver level, the implementation differs. There is no data structure to edit. But in the trunk revision 10935, there is EmulatedWindowState.moveToTop() and AbstractGuiDriver.stackWindows().

In task branch 1811q we have changed this approach to eliminate EmulatedWindowState.moveToTop() (actually it is still there for the SwingEmulatedWindow subclass but wasn't needed for the web client. We also modified how stackWindows() works such that there is a driver-specific worker named restackWindows() that handles the implementation details.

Swing doesn't directly expose the operating system level control over z-order. So our SwingGuiDriver must "play some games" to manage the z-order. That means that the restackWindows() is very inefficient for Swing so I prefer not to use it if we can implement a more specific moveToTop() and moveToBottom() that can be implemented efficiently.

Task branch 1811q is meant to be merged to trunk before 2567b. For now, I think you should only call the WindowManager.moveToTop() and WindowManager.moveToBottom() as your "workers".

In 1811q, we will update WindowManager.moveToTop() and WindowManager.moveToBottom() to make the changes visible.

I see. Thank you Greg.

Implemented WINDOW:MOVE-TO-(TOP/BOTTOM) (by invoking invoking corresponding methods of the WindowManager). See note 276 above.
Committed to branch 2567b revno 10949

Greg,
I do not completely understand the semantics of the SELECTABLE attribute at this moment but it seems that this semantics is related to the SELECTED attribute. However the SELECTED attribute is currently only partially supported (the corresponding setters/getters are implemented for BaseEntity and FrameWidget only and no semantics is implemented). Does it makes sense to continue working on the SELECTABLE attribute runtime support?

Thank you,
Igor.

Does it makes sense to continue working on the SELECTABLE attribute runtime support?

No.

But I want do you to document your findings so far.

Greg Shah wrote:

Does it makes sense to continue working on the SELECTABLE attribute runtime support?

No.

But I want do you to document your findings so far.

OK. Thank you.

Added conversion support for the MENU-KEY and MENU-MOUSE attributes.

Committed to the 2567b branch revno 10950

Greg,
Can you please provide more information about what I'm supposed to do regarding the AUTO-RESIZE attribute? A I can see the conversion support is already in place and some runtime support is also implemented.
Thank you,
Igor.

Write testcases to confirm that the behavior works fully. List any deviations and resolve them.

Fixed setters for the MENU-MOUSE attribute.

Committed to the 2567b branch revno 10951

Greg Shah wrote:

Write testcases to confirm that the behavior works fully. List any deviations and resolve them.

OK. Thank you.

1. Added testcases/uast/menu/menu_mouse.p revno 1330
2. Finished runtime support for the MENU-MOUSE attribute.
Committed to 2567b branch revno 10952

Added testcase/uasr/selectable/s.p test for the SELECTABLE attribute. revno 1331.

As far as I can see the SELECTABLE attribute affects only firing SELECTED and DESELECTED events on widgets with SELECTABLE = yes. These events are activated via SELECT (left button) and EXTEND (ctrl + left button) mouse clicks. The default result of these events is setting the SELECTED attribute of the widget to true or false respectively. As mentioned in the documentation this assignment happens after the trigger invocation.

Added testcases/uast/auto_resize/arf.p test for FILL-IN:AUTO-RESIZE runtime support, revno 1332.

The following incompatibilities are found:
1. The widget is not resized on the FONT change regardless the value of the AUTO-RESIZE
2. The widget is resized on the FORMAT change regardless the value of the AUTO-RESIZE

Fixed incompatibility in the FILL-IN:AUTO-RESIZE runtime support

Committed to branch 2567b revno 10953

Added testcases/uast/auto_resize/arbtn.p test for FILL-BUTTON:AUTO-RESIZE runtime support, revno 1333.

The following incompatibilities are found:
1. The widget is not resized on the FONT change regardless the value of the AUTO-RESIZE

It looks that the value of the FONT attribute is ignored by most widgets. I'm working now on refactoring the runtime support of it in FILL-IN for wider reuse.

I've found another incompatibility. When the BUTTON:LABEL attribute is changed neither WIDHT-CHARS nor WIDHT-PIXELS attributes are updated. See (updated) uast/auto_resize/arbtn.p test (testcases revno 1334).

I suggest to add abstract updateSize(beforeUpdate) method to the AbstractWidget class and invoke it in the updateSizeWorker() method. The concrete implementations should check if FORMAT/FONT/LABEL attributes (when applicable) have been changed and AUTO-RESIZE is true and recalculate widget size. As far as I can see all widgets invoke afterConfigUpdateBase() method at the beginning of afterConfigUpdate so the size recalculation will be guaranteed.
The FillIn.afterConfigUpdate and FillIn.updateSize() methods should be changed according to the new logic.

Constantin, what do you think about that?
Thank you.

Igor Skornyakov wrote:

I suggest to add abstract updateSize(beforeUpdate) method to the AbstractWidget class and invoke it in the updateSizeWorker() method. The concrete implementations should check if FORMAT/FONT/LABEL attributes (when applicable) have been changed and AUTO-RESIZE is true and recalculate widget size. As far as I can see all widgets invoke afterConfigUpdateBase() method at the beginning of afterConfigUpdate so the size recalculation will be guaranteed.
The FillIn.afterConfigUpdate and FillIn.updateSize() methods should be changed according to the new logic.

Constantin, what do you think about that?

It looks good. BTW, is frame layout/size affected (if FRAME:AUTO-RESIZE is true) when a widget's font/format/label is changed? The position of the other widgets remain unaffected if only one widget is auto-resized?

Constantin Asofiei wrote:

Igor Skornyakov wrote:

I suggest to add abstract updateSize(beforeUpdate) method to the AbstractWidget class and invoke it in the updateSizeWorker() method. The concrete implementations should check if FORMAT/FONT/LABEL attributes (when applicable) have been changed and AUTO-RESIZE is true and recalculate widget size. As far as I can see all widgets invoke afterConfigUpdateBase() method at the beginning of afterConfigUpdate so the size recalculation will be guaranteed.
The FillIn.afterConfigUpdate and FillIn.updateSize() methods should be changed according to the new logic.

Constantin, what do you think about that?

It looks good. BTW, is frame layout/size affected (if FRAME:AUTO-RESIZE is true) when a widget's font/format/label is changed? The position of the other widgets remain unaffected if only one widget is auto-resized?

Thank you Constantin. The FRAME widget doesn't have the AUTO-RESIZE attribute. I haven't analyzed yet if frame layout is changed after some of its widgets' size was changed. Please note however that the default value of the AUTO-RESIZE attribute is true so I assume that the frame behavior is not in the scope of this particular task.

Is the issue found in #2567-263, still a problem?

Greg Shah wrote:

Is the issue found in #2567-263, still a problem?

Yes, it is still a problem. As you may remember I've found a workaround to continue my work and you've reopened #2716

As you may remember I've found a workaround to continue my work and you've reopened #2716

OK, thanks. Somehow I thought there were two separate issues (note 263 and #2716). I'm glad to know I was wrong. :)

Please note however that the default value of the AUTO-RESIZE attribute is true so I assume that the frame behavior is not in the scope of this particular task.

Agreed. We will test this as part of #2038. If any "re-layout" issues are found we can consider and fix them as part of that work.

Igor Skornyakov wrote:

I've found another incompatibility. When the BUTTON:LABEL attribute is changed neither WIDHT-CHARS nor WIDHT-PIXELS attributes are updated. See (updated) uast/auto_resize/arbtn.p test (testcases revno 1334).

Does your proposed auto-resize design resolve this issue too?

Greg Shah wrote:

Igor Skornyakov wrote:

I've found another incompatibility. When the BUTTON:LABEL attribute is changed neither WIDHT-CHARS nor WIDHT-PIXELS attributes are updated. See (updated) uast/auto_resize/arbtn.p test (testcases revno 1334).

Does your proposed auto-resize design resolve this issue too?

Yes, it does.

Excellent. When do you plan to have this finished?

Greg Shah wrote:

Excellent. When do you plan to have this finished?

I hope to finish it today. Tomorrow at latest.

I have a question. The BROWSE (column) widget has an AUTO-RESIZE attribute. However the BrowseConfig doesn't extend BaseConfig and doesn't have a field for the FONT attribute. The first means that the updateSizeWorker method is not invoked in the afterConfigUpdateBase. The second one is just a note.
Should I try to fix these issues?

I don't know why BrowseConfig doesn't extend BaseConfig, but I'm not very happy with this fact. So I can solve it.

It is a little bit inconvenient that afterConfigUpdateBase method is in the AbstractWidget class which does not implement WidgetWithConfig interface. I suggest to add an additional abstract class which will extend AbstractWidget and implement WidgetWithConfig interface. All classes which implement WidgetWithConfig will extend a new class instead of AbstractWidget. This class will contain afterConfigUpdateBase method. For the backward compatibility the AbstractWidget will have a noop afterConfigUpdateBase method.
Is it OK?
Thank you.

I suggest to add an additional abstract class which will extend AbstractWidget and implement WidgetWithConfig interface. All classes which implement WidgetWithConfig will extend a new class instead of AbstractWidget.

I'm not sure I understand how this would work. The WidgetWithConfig is generic and is implemented at a lower level in the hierarchy for two reasons (if I recall properly):

1. It is generic and the more specific implementation point means that the specific *Config class can be parameterized properly.
2. Not all widgets support this.

The second item (probably) can be accounted for by your approach, but the first item doesn't seem to match your idea.

Greg Shah wrote:

1. It is generic and the more specific implementation point means that the specific *Config class can be parameterized properly.
2. Not all widgets support this.

The second item (probably) can be accounted for by your approach, but the first item doesn't seem to match your idea.

The idea was that the type parameter of the WidgetWithConfig will become an additional type parameter of the new class. This will allow to make the argument type of the afterConfigUpdateBase will be generic. After all this method makes sense only for widgets with config. Of course only widgets which now implement WidgetWithConfig will extend this new class (instead of AbstractWidget).

However I've found a less intrusive solution to my initial problem to have a correct argument type for updateSize(beforeUpdate) method in relevant widgets. Simply be adding a type parameter to afterConfigUpdateBase and updateSizeWorker methods.

Simply be adding a type parameter to afterConfigUpdateBase and updateSizeWorker methods.

Good. That seems a better solution.

Committed to the task branch 2567b revno 10955

It is not possible to run an application which references to a FONT attribute in the ChUI mode.

java.lang.reflect.InvocationTargetException
        at sun.reflect.GeneratedMethodAccessor13.invoke(Unknown Source)
        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:1252)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
        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.client.FontManager$EnvironmentFontTable.access$700(FontManager.java:881)
        at com.goldencode.p2j.ui.client.FontManager.fontTableSize(FontManager.java:487)
        at com.goldencode.p2j.ui.chui.ThinClient.fontTableSize(ThinClient.java:5797)
        at sun.reflect.GeneratedMethodAccessor14.invoke(Unknown Source)
        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.waitMessage(Conversation.java:257)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1128)
        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.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1424)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:97)
        at com.sun.proxy.$Proxy4.standardEntry(Unknown Source)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:277)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:100)
        at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:201)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:398)
        at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:95)
        at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:267)

EDITOR:(WIDTH-CHAR/WIDTH-PIXELS) are always 0.

It is not possible to run an application which references to a FONT attribute in the ChUI mode.

What does this same program do in 4GL ChUI?

As far as I understand it, fonts are not supported in ChUI. Although our failure is not correct, I'm surprised that the programs actually function properly in the 4GL ChUI environment.

Greg Shah wrote:

What does this same program do in 4GL ChUI?

It reports an error

**Progress does not support the FONT attribute for the  EDITOR f1 in this display environment. (4088)

As far as I understand it, fonts are not supported in ChUI. Although our failure is not correct, I'm surprised that the programs actually function properly in the 4GL ChUI environment.

The FONT attribute is indeed not supported in ChUI but the program which accesses it at least runs.

Please go ahead and fix this. I think it is best to just detect that the system is in ChUI mode (on the server side) when the attribute is accessed and generate this error/return unknown. That should avoid the NPE and behave the same way as the 4GL.

Greg Shah wrote:

Please go ahead and fix this. I think it is best to just detect that the system is in ChUI mode (on the server side) when the attribute is accessed and generate this error/return unknown. That should avoid the NPE and behave the same way as the 4GL.

OK, I will do it.

EDITOR:(WIDTH-CHAR/WIDTH-PIXELS) are always 0.

What testcase in testcases/uast/ can be used to see this?

Greg Shah wrote:

EDITOR:(WIDTH-CHAR/WIDTH-PIXELS) are always 0.

What testcase in testcases/uast/ can be used to see this?

It is uast/auto-resize/are.p. I plan to fix it as a part of the EDITOR:AUTO-RESIZE support. I'm working on it right now.

Something strange is going on with the EDITOR widget. With the uast/auto_resize/are.p in ChUI mode both HEIGHT-CHARS and WIDTH-CHARS have correct values but when I type the cursor is mooving but nothing is visible until the widget redraw (after A is pressed), In GUI mode both HEIGHT-CHARS and WIDTH-CHARS are always zero (as mentioned above). Moreover with the debugger I see that physicalDimension() returns (0, 0) and when I type nothing is visible even after redraw. It possible of course (albeit unlikely) that this is a result of my changes. Investigating.

Well, at least now HEIGHT-CHARS and WIDTH-CHARS are non-zero in GUI mode (however the values are a little greater than with 4GL) and I can see what I'm typing after redraw.

The SELECTION-LIST:AUTO-RESIZE support is finished.
Committed to branch 2567b revno 10957.

I've noticed the following issues in the default behavior of the SELECTION-LIST in GUI mode (see uast/auto_resize/arsl.p)

1. Calculated WIDTH-CHARS/WIDTH-PIXELS values are not the same as in 4GL
2. When the FONT attribute is changed the WIDTH-CHARS attribute's value of the widget remains the same. As the result(?) the border has drawn incorrectly.

Should I fix these issues in the scope of this task?

Thank you.

Igor Skornyakov wrote:

Greg Shah wrote:

1. It is generic and the more specific implementation point means that the specific *Config class can be parameterized properly.
2. Not all widgets support this.

The second item (probably) can be accounted for by your approach, but the first item doesn't seem to match your idea.

The idea was that the type parameter of the WidgetWithConfig will become an additional type parameter of the new class. This will allow to make the argument type of the afterConfigUpdateBase will be generic. After all this method makes sense only for widgets with config. Of course only widgets which now implement WidgetWithConfig will extend this new class (instead of AbstractWidget).

I agree the afterConfigUpdateBase() implemented in the class not implementing WidgetWithConfig may not seem very intuitive. It is needed to have a base implementation that covers the behavior common to all widgets with a valid runtime config. The WidgetWithConfig interface provides strongly typed interface for widgets that are known at compile-time to hold a configuration class and also a marker to let the surrounding logic handle the required config agenda. You can consider this as a kind of extension on top of the pure runtime nature of the base config support (Widget.config()).

I don't think the WidgetWithConfig abstract class would work also for another reason, some containers do require config and some not.

Hynek Cihlar wrote:

I agree the afterConfigUpdateBase() implemented in the class not implementing WidgetWithConfig may not seem very intuitive. It is needed to have a base implementation that covers the behavior common to all widgets with a valid runtime config. The WidgetWithConfig interface provides strongly typed interface for widgets that are known at compile-time to hold a configuration class and also a marker to let the surrounding logic handle the required config agenda. You can consider this as a kind of extension on top of the pure runtime nature of the base config support (Widget.config()).

I don't think the WidgetWithConfig abstract class would work also for another reason, some containers do require config and some not.

Currently we distinguish these three cases, widget has no config and doesn't

Thank you for your comments Hynek. I've found a simpler solution to my problem by adding generic config type to the relevant methods. I needed it as I don't want to rely on inheritance of the *Config classes in the program logic.

Igor Skornyakov wrote:

The SELECTION-LIST:AUTO-RESIZE support is finished.
Committed to branch 2567b revno 10957.

I've noticed the following issues in the default behavior of the SELECTION-LIST in GUI mode (see uast/auto_resize/arsl.p)

1. Calculated WIDTH-CHARS/WIDTH-PIXELS values are not the same as in 4GL
2. When the FONT attribute is changed the WIDTH-CHARS attribute's value of the widget remains the same. As the result(?) the border has drawn incorrectly.

Should I fix these issues in the scope of this task?

It depends:

1. How long do you estimate it will take?
2. What else still needs to be done on this task?

Greg Shah wrote:

Igor Skornyakov wrote:

The SELECTION-LIST:AUTO-RESIZE support is finished.
Committed to branch 2567b revno 10957.

I've noticed the following issues in the default behavior of the SELECTION-LIST in GUI mode (see uast/auto_resize/arsl.p)

1. Calculated WIDTH-CHARS/WIDTH-PIXELS values are not the same as in 4GL
2. When the FONT attribute is changed the WIDTH-CHARS attribute's value of the widget remains the same. As the result(?) the border has drawn incorrectly.

Should I fix these issues in the scope of this task?

It depends:

1. How long do you estimate it will take?

I hope that I can do it in one day, but I'm not 100% sure

2. What else still needs to be done on this task?

The remaining widgets are BUTTON, COMBO-BOX (working on now) and EDITOR. There is also small thing to add setting AUTO-RESIZE to false on explicit size set.

The remaining widgets are BUTTON, COMBO-BOX (working on now) and EDITOR. There is also small thing to add setting AUTO-RESIZE to false on explicit size set.

Finish these as your top priority. How much time do you expect to need?

1811q is in regression testing. 2567b is next. If 1811q is merged to trunk today, then I want to move forward with 2567b immediately. That would mean deferring the SELECTION-LIST issues.

However, if 1811q has issues it may open a window for the SELECTION-LIST issues to be included in 2567b, so long as the other features go in quickly.

Greg Shah wrote:

The remaining widgets are BUTTON, COMBO-BOX (working on now) and EDITOR. There is also small thing to add setting AUTO-RESIZE to false on explicit size set.

Finish these as your top priority. How much time do you expect to need?

1811q is in regression testing. 2567b is next. If 1811q is merged to trunk today, then I want to move forward with 2567b immediately. That would mean deferring the SELECTION-LIST issues.

However, if 1811q has issues it may open a window for the SELECTION-LIST issues to be included in 2567b, so long as the other features go in quickly.

I planned to finish yesterday but it appears to be more tricky then I expected. I'll do my best to finish today. However the situation with EDITOR looks pretty complicated. The BUTTON also doesn't adjust its size properly (at all actually) but the widget itself is much simpler.

On 24.09.2015 18:26, ias wrote:

Constantin,
I'm working now on AUTO-RESIZE support and have discovered that there are many places where GUI version of widgets do not take FONT into consideration. From the other side I see several (apparently) different ways to do it in the our code (the most complicated one is in FillInGuiImpl). Can you please advise me what is the `right` way?
Thank you,
Igor.

On 09/24/2015 12:57 PM, Constantin Asofiei wrote:

Igor,

Are the other widgets using a "common" approach? Anyway, if the FONT attribute is changed to a different value, I would expect GuiFontResolver (i.e. FillInGuiImpl.gf) to invalidate its cache, so that the next GuiFontResolver.font() call will resolve the new font.

I suspect for all widgets this will require re-computation of its implicit dimension (if an explicit one is not set); do you have an approach here? I would suggest using "afterConfigUpdate" and if the FONT has changed, re-calculate the implicit dimension (if needed).

Also, can you list some places where FONT is not used?

Thanks,
Constantin

Thank you Constantin.
It seems that I was not clear enough, sorry. Actually I was asking about re-computation of dimensions. See e.g. ComboBoxGuiImpl.width() method (ironically the height() method considers FONT).
Thank you,
Igor.

Igor Skornyakov wrote:

It seems that I was not clear enough, sorry. Actually I was asking about re-computation of dimensions. See e.g. ComboBoxGuiImpl.width() method (ironically the height() method considers FONT).

I think the combo's width is computed in ComboBoxGuiImpl.calcLimit().

Constantin Asofiei wrote:

I think the combo's width is computed in ComboBoxGuiImpl.calcLimit().

I was thinking about that. However it doesn't change the fact that in GUI mode the FONT value doesn't affect the size of the COMBO-BOX.

Eugenie, can you provide some details when the combo's width is computed in ComboBoxGuiImpl?

Constantin Asofiei wrote:

Eugenie, can you provide some details when the combo's width is computed in ComboBoxGuiImpl?

Actually I've found some solution. I do not like it though.

Implemented COMBO-BOX:AUTO-RESIZE support.

Committed to branch 2567b revno 10958

Constantin Asofiei wrote:

Eugenie, can you provide some details when the combo's width is computed in ComboBoxGuiImpl?

You are right, it is in calcLimit().

Implemented BUTTON:AUTO-RESIZE support.

Committed to branch 2567b revno 10959

I'm very confused.

With the uast/auto_resize/are.p the EDITOR widget seems to doesn't `see` the FONT attribute change. With the debugger I see that both font id = 0 and 7 resolve to ms sans serif,8,false,false,false. However when I run uast/auto_resize/arbnt.p the font id = 0 resolves to courier new,8,false,false,false and font id = 7 to ms sans serif,8,false,false,false

In all my tests from uast/auto_resize only this uast/auto_resize/are.p has this problem.

How this can happen?

Thank you.

Igor Skornyakov wrote:

I'm very confused.

With the uast/auto_resize/are.p the EDITOR widget seems to doesn't `see` the FONT attribute change. With the debugger I see that both font id = 0 and 7 resolve to ms sans serif,8,false,false,false. However when I run uast/auto_resize/arbnt.p the font id = 0 resolves to courier new,8,false,false,false and font id = 7 to ms sans serif,8,false,false,false

In all my tests from uast/auto_resize only this uast/auto_resize/are.p has this problem.

How this can happen?

Thank you.

Well, I think I understand the reason.

BTW: I've noticed that in many cases the GuiFontResolver constructor is invoked with the null window argument. Is it normal?

Thank you.

EDITOR:AUTO-RESIZE support is finished.

Comitted to branch 2567b revno 10960.

Now the behavior of the widget is more close to one in 4GL but it is still not exactly the same. In particular calculated dimension is different. This is actually a common issue with most GUI widget.

Added JavaDocs

Comitted to branch 2567b revno 10961.

Added AUTO-RESIZE reset on the size attributes' assignment

Comitted to branch 2567b revno 10962.

Ready for the code review

To make GUI widgets' code more standardized I suggest to refactor them to simulate GUI `mixin`.

Something like this:

public abstract class GuiMixin<T extends GuiWidget>
{
   private T widget();
// common operations and fields for GUI widgets
// uses GuiWdget methods to access widget-specific fields/methods via widget()
.....
}

public interface GuiWidget
{
//  widget-specific methods for GuiMixin
}

public class SomeWidgetGuiImpl 
extends ...
implements GuiWidget<SomeWidgetGuiImpl>, .....
{
  private final GuiMixin mixin = new GuiMixin<SomeWidgetGuiImpl>()
  {
     public SomeWidgetGuiImpl widget()
     {
        return this;
     }
     // GuiWidget methods' implementation
     ...
     // Other methods
     ...
  }
}

First of all, please rebase and upgrade your system to Java 8.

Second, about the EDITOR widget and all widgets which require an explicit size, and don't have an implicit size, as the widget's SIZE option (if a static widget) or it's explicit width and height must always be set before realizing this widget. So, changing the FONT will just mean that new size is computed using this font; but what are the rules to compute the new size? What is used as reference - the size in pixels, in character units, or something else? For example, if you have a SIZE 20 BY 20 and change fonts, both the pixels and character values are changed... Also, what happens if for a widget it's pixels dimensions are set, and is set as AUTO-RESIZE? An idea is that the widget's size is computed by determining it's pixel-unit size using the new font AND the old character-unit size, and after that the character-unit size is re-computed by dividing the pixel-size with the session:pixels-per-column/row values.

Third, same for widgets with auto-resize and their explicit width smaller/larger than the default width: how is the new width computed if FONT/FORMAT is changed? Does it fall back to calculating an implicit size using the new font/format, or the explicit size is "translated" to the new font, in a similar way as for the widgets which can't have an implicitly computed size?

1. You've set the Autoresizable interface only for the GUI widgets: are the ChUI widgets in scope for this task? I would expect them to behave in a similar way as the GUI widgets.
2. frame_generator.xml - please expand the history, if there was an explicit reason why the UNKNOWN was added. (as per Ovidiu request) gives no information because the reader can't know why Ovidiu requested this.
3. ControlEntity.setAutoResize(logical) - it needs to have a return statement if the value is unknown, as in:
   

         if (autoResize.isUnknown())
         {
            ErrorManager.recordOrShowError(4083, String.format(
                  "Unable to assign UNKNOWN value to attribute AUTO-RESIZE on %s %s.", type(),
                     frame.getName(this)), false);
            return;
         }
   

4. FrameInterface history comment says that it contains support for TOP-ONLY, but there are no changes related to this attribute.
5. GenericWidget.moveToTop/Bottom - please do not use handle.unwrap APIs within the P2J internal runtime; instead, get the resource directly via handle.getResource(), cast it (if necessary) and use it directly. Also, do not use new logical(...).equals(...), instead use e.g. isKeepFrameZOrder().booleanValue().
6. GenericWidget - you need a space before a left-paranthesys, as in while ( instead of while(.
7. GenericWidget.getWindow() - you can access the widget's frame directly via GenericWidget.frame.asWidget(), you don't need to walk the parent chain to find the frame. Please change the code.
8. FocusPlacementManager.hideFrames needs javadoc for the topOnly parameter
9. ThinClient.moveToBottom and moveToTop - these APIs are called directly from the server-side; you are executing a repaint() call at the end of the method, but this repaint() will have no event processing loop to process it, so it will be discarded. Currently, the repaint() at lines 17849 and 17809 are no-ops. If you need to ensure drawing is performed, call the w.moveToTop/Bottom() APIs in a TC.independentDrawingBracket
10. Browse.startEdit - you've added a TC.setupTooltip call, but this API is for internal browse edit... I doubt you can specify an explicit tooltip for this editable browse cell. Or is it inherited from the Browse?
11. FillIn.java has no functional change... please discard the changes
12. ButtonGuiImpl.width() - the comments in this code mention COMBO-BOX widget, not BUTTON... also, this code is a copy-paste from ComboBoxGuiImpl.calcLimit - this is something specific for combo-box, not button!
13. ButtonGuiImpl.updateSize() - has a // TODO Auto-generated method stub comment and an empty line at the end
14. Editor.width() and height() - these break the contract of the width/height setters and also of the setDimension API... I think there is a good chance these will break things. Why did you need them?
15. EditorGuiImpl.BORDER_SIZE comment mentions selection-list; also, static fields need to be placed before the instance fields,
16. EditorGuiImpl.adjustSize is copy-pasted code from the SelectionListGuiImpl.adjustSizeImpl - the comments even mention selection-list, not editor.
17. The changes in EditorContent.width() and width() are not OK - you can't compute dimensions relative to the Editor; if there are scrollbars (which are not included in the EditorContent size, but in some parent class), this will get incorrect results, as the Editor contains the full size, including scrollbars. I think this logic should remain as it is.
18. ImageGuiImpl.getZOrderClass needs javadoc.
19. RectangleGuiImpl.getZOrderClass javadoc: the return tag comment needs to be on the same line as the tag.
20. AbstractWidget.moveToTopInClass:633 should be ((np size) || (np >= pos)) not ( (np size) || (np >= pos) ).
21. AbstractContainer.normalizeZOrder and ZKey class need javadoc. Also, there are formatting issues for ZKey class.
22. AbstractWidget.getMenuMouse needs javadoc
23. TitledWindow.getZOrderClass needs javadoc
24. Widget.ZOrderClass formatting is problematic
25. StringHelper.equal needs javadoc
26. TopOnlyInterface - you have empty lines after the last method of the interface, please remove them.
27. WidgetConfig.readExternal - empty line before end of method, remove it.
28. SelectionList class open curly brace is on the wrong line...

Thank you Constantin.

First of all, please rebase and upgrade your system to Java 8.

As per Greg's instruction I have to rebase only after fixing all issues with the first code review and re-testing.

Second, about the EDITOR widget and all widgets which require an explicit size, and don't have an implicit size, as the widget's SIZE option (if a static widget) or it's explicit width and height must always be set before realizing this widget. So, changing the FONT will just mean that new size is computed using this font; but what are the rules to compute the new size? What is used as reference - the size in pixels, in character units, or something else? For example, if you have a SIZE 20 BY 20 and change fonts, both the pixels and character values are changed... Also, what happens if for a widget it's pixels dimensions are set, and is set as AUTO-RESIZE? An idea is that the widget's size is computed by determining it's pixel-unit size using the new font AND the old character-unit size, and after that the character-unit size is re-computed by dividing the pixel-size with the session:pixels-per-column/row values.

The documentation states that explicit setting of WIDTH-CHARS, HEIGTH-CHARS, WIDTH-PIXELS or HEIGHT-PIXELS sets AUTO-RESIZE to false

Third, same for widgets with auto-resize and their explicit width smaller/larger than the default width: how is the new width computed if FONT/FORMAT is changed? Does it fall back to calculating an implicit size using the new font/format, or the explicit size is "translated" to the new font, in a similar way as for the widgets which can't have an implicitly computed size?

Sorry, I'm not sute that completely understand the question. Does my response to the previous question is an answer for this one as well?

Finally, here is the code review for branch 2567b rev 10962; major issues are that at least EDITOR and BUTTON auto-resize is likely incorrect/incomplete.

Can you please be a little more specific on this regard? What issues you can mention?

1. You've set the Autoresizable interface only for the GUI widgets: are the ChUI widgets in scope for this task? I would expect them to behave in a similar way as the GUI widgets.

The documentation explicitely states that AUTO-RESIZE semantics if for GUI only.

I'm working on the Javadoc/formatting issues you've mentioned now.

Well, I think I understand the reason.

In regard to note 338, please share your findings. It is great that you understand the reason, but no one that reads this task history will understand it unless you provide the details.

The documentation explicitely states that AUTO-RESIZE semantics if for GUI only.

Please put enough comments into the code such that readers can understand this. Explain that it is not just 4GL documentation (which is frequently wrong), but that you have tested AUTO-RESIZE and it does not work in ChUI.

Greg Shah wrote:

Well, I think I understand the reason.

In regard to note 338, please share your findings. It is great that you understand the reason, but no one that reads this task history will understand it unless you provide the details.

The reason was that GuiFontResolver constructor was invoked with the null first (window) argument

The reason was that GuiFontResolver constructor was invoked with the null first (window) argument

Hmmm. The two ways we get this argument is though Window.resolveWindow(this) or in looking up the window using the parent/ancestor. Which of these are failing in your case? And do you know why?

Greg Shah wrote:

Hmmm. The two ways we get this argument is though Window.resolveWindow(this) or in looking up the window using the parent/ancestor. Which of these are failing in your case? And do you know why?

null was returned by the ancestor() method.

Greg Shah wrote:

Please put enough comments into the code such that readers can understand this. Explain that it is not just 4GL documentation (which is frequently wrong), but that you have tested AUTO-RESIZE and it does not work in ChUI.

Well, actually I was based on the documentation. For most widgets AUTO-RESIZE semantics is for the FONT changes. However I've just checked the FILL-IN and COMBO-BOX widget and found that AUTO-RESIZE affects resizing in the FORMAT change. It also affects resizing of BUTTON and TOGGLE-BOX on the LABEL change. I will fix this now.

Are you talking about ChUI or GUI?

If you haven't tested it on ChUI, please do so and record your results.

Greg Shah wrote:

Are you talking about ChUI or GUI?

If you haven't tested it on ChUI, please do so and record your results.

In the previous note I was talking about recent tests in the ChUI mode. Sorry but I was confused by the documentation. I will fix p2j ChUI shortly.

Igor Skornyakov wrote:

Greg Shah wrote:

Are you talking about ChUI or GUI?

If you haven't tested it on ChUI, please do so and record your results.

In the previous note I was talking about recent tests in the ChUI mode. Sorry but I was confused by the documentation. I will fix p2j ChUI shortly.

Please don't ever trust the 4GL documentation.

Igor Skornyakov wrote:

Greg Shah wrote:

Hmmm. The two ways we get this argument is though Window.resolveWindow(this) or in looking up the window using the parent/ancestor. Which of these are failing in your case? And do you know why?

null was returned by the ancestor() method.

This doesn't seem to be a normal case. By the time initialize() is called, the parent hierarchy should be valid.

Is the problem seen in FrameGuiImpl, ScrollBarGuiImpl or ScrollableListGuiImpl? (In the trunk, EditorGuiImpl also uses ancestor(), but that is not in your branch yet.

Greg Shah wrote:

Please don't ever trust the 4GL documentation.

I understand but in this case it is written in bold at the very beginning of the attributes' description.

Greg Shah wrote:

This doesn't seem to be a normal case. By the time initialize() is called, the parent hierarchy should be valid.

Is the problem seen in FrameGuiImpl, ScrollBarGuiImpl or ScrollableListGuiImpl? (In the trunk, EditorGuiImpl also uses ancestor(), but that is not in your branch yet.

Af far as I remember the ancestor() was invoked from the inner class. I've just replaced this call with the EditotGuiImpl.this.

You've been fooled by a font. :)

We have found out the hard way on many, many occasions that we really can never trust the 4GL documentation.

Greg Shah wrote:

You've been fooled by a font. :)

We have found out the hard way on many, many occasions that we really can never trust the 4GL documentation.

You're right. Now I know. Fortunately in this particular case the main part was the GUI one. I hope to finish the ChUI support in about an hour.

Implemented AUTO-RESIZE support for ChUI widgets (where applicable).

Comitted to branch 2567b revno 10963

Igor Skornyakov wrote:

Second, about the EDITOR widget and all widgets which require an explicit size, and don't have an implicit size, as the widget's SIZE option (if a static widget) or it's explicit width and height must always be set before realizing this widget. So, changing the FONT will just mean that new size is computed using this font; but what are the rules to compute the new size? What is used as reference - the size in pixels, in character units, or something else? For example, if you have a SIZE 20 BY 20 and change fonts, both the pixels and character values are changed... Also, what happens if for a widget it's pixels dimensions are set, and is set as AUTO-RESIZE? An idea is that the widget's size is computed by determining it's pixel-unit size using the new font AND the old character-unit size, and after that the character-unit size is re-computed by dividing the pixel-size with the session:pixels-per-column/row values.

The documentation states that explicit setting of WIDTH-CHARS, HEIGTH-CHARS, WIDTH-PIXELS or HEIGHT-PIXELS sets AUTO-RESIZE to false

Please confirm this through testing, if you haven't already. But, what if AUTO-RESIZE is set AFTER these attributes are set (and also in case the SIZE option is used), and only after that the FONT or FORMAT is changed - how is the widget resized?

For EDITOR case (static widget), its size can be set only via the SIZE option (and after that the size attributes can be used, too). What I would like to see is what rules 4GL uses to re-size the EDITOR (and other similar widgets which require the SIZE option) - how is the new size computed.

Third, same for widgets with auto-resize and their explicit width smaller/larger than the default width: how is the new width computed if FONT/FORMAT is changed? Does it fall back to calculating an implicit size using the new font/format, or the explicit size is "translated" to the new font, in a similar way as for the widgets which can't have an implicitly computed size?

Sorry, I'm not sute that completely understand the question. Does my response to the previous question is an answer for this one as well?

Same as above, AUTO-RESIZE can be set back on after the widget's size is changed via the attributes. So in a sequence like this, where h is a widget:

h:width-chars = something.
h:auto-resize = yes.
h:font = some-other-font.

Finally, here is the code review for branch 2567b rev 10962; major issues are that at least EDITOR and BUTTON auto-resize is likely incorrect/incomplete.

Can you please be a little more specific on this regard? What issues you can mention?

What I don't like for BUTTON (for example) is the size adjustment code is just the size adjustment for COMBO-BOX... this doesn't look right, especially this line: int ntlimit = ((int)wTxt) * wFont + 40; - where are the 40 pixels coming for button? Also, for BUTTON: what if it is using an image? Does resize still work?

Thank you Constantin. I will review my code.

Strangely enough today in my tests (ar*.p) triggers are not invoked. Actullay these apps seem not to react on most keyboard events at all. They exit on ESC though.
Can anybody suggest a solution?
Thank you.

Igor Skornyakov wrote:

Strangely enough today in my tests (ar*.p) triggers are not invoked. Actullay these apps seem not to react on most keyboard events at all. They exit on ESC though.
Can anybody suggest a solution?
Thank you.

Sorry. It was because the keyboard locale somehow switched to russian.

Constantin Asofiei wrote:

Please confirm this through testing, if you haven't already. But, what if AUTO-RESIZE is set AFTER these attributes are set (and also in case the SIZE option is used), and only after that the FONT or FORMAT is changed - how is the widget resized?

Yes, AUTO-RESIZE can be set to true after is was set to false after explicit sizing. I do not understand the problem with resizing after that. If the resize happens because of the FORMAT or LABEL change the new size is calculated based on new values of these attributes. On the FONT change it is recalculated based on the existing dimentions.
Please nore however that resizing is the default behavior so I didn't consider the detailed examination of resizing logic as a part of this task.

For EDITOR case (static widget), its size can be set only via the SIZE option (and after that the size attributes can be used, too). What I would like to see is what rules 4GL uses to re-size the EDITOR (and other similar widgets which require the SIZE option) - how is the new size computed.

Third, same for widgets with auto-resize and their explicit width smaller/larger than the default width: how is the new width computed if FONT/FORMAT is changed? Does it fall back to calculating an implicit size using the new font/format, or the explicit size is "translated" to the new font, in a similar way as for the widgets which can't have an implicitly computed size?

Please see my response regading the resize logic above. Should I consider a detailed analysis of the dimensions' recalculation as a part of this task?

Thank you.

To make GUI widgets' code more standardized I suggest to refactor them to simulate GUI `mixin`.

Something like this:

...

I'm open to this idea. It certainly has some promise. It has definitely been an objective to use more common code for GUI implementation. Of course, we would want as much code as possible to be implemented in the GuiMixin class itself so that the widget-specific subclass (which is contained as an inner class) has a minimized implementation. I presume that this contained instance would be used as a helper class to implement certain operations.

Please post ideas of what operations can be placed into that common code.

One concern I have is that we are on a very tight deadline and refactoring right now is potentially time consuming. It is also risky (it opens greater possibility of regressions). We may defer this idea a bit.

Constantin/Hynek: any thoughts?

Greg Shah wrote:

I'm open to this idea. It certainly has some promise. It has definitely been an objective to use more common code for GUI implementation. Of course, we would want as much code as possible to be implemented in the GuiMixin class itself so that the widget-specific subclass (which is contained as an inner class) has a minimized implementation. I presume that this contained instance would be used as a helper class to implement certain operations.

Please post ideas of what operations can be placed into that common code.

One concern I have is that we are on a very tight deadline and refactoring right now is potentially time consuming. It is also risky (it opens greater possibility of regressions). We may defer this idea a bit.

Constantin/Hynek: any thoughts?

I understand that this is not to be implemented right now. I've decided to share my thoughts. When I will finish with urgent task I'll try to prepare more detailed suggestion.

Greg Shah wrote:

To make GUI widgets' code more standardized I suggest to refactor them to simulate GUI `mixin`.

Something like this:

...

I'm open to this idea. It certainly has some promise. It has definitely been an objective to use more common code for GUI implementation. Of course, we would want as much code as possible to be implemented in the GuiMixin class itself so that the widget-specific subclass (which is contained as an inner class) has a minimized implementation. I presume that this contained instance would be used as a helper class to implement certain operations.

Please post ideas of what operations can be placed into that common code.

One concern I have is that we are on a very tight deadline and refactoring right now is potentially time consuming. It is also risky (it opens greater possibility of regressions). We may defer this idea a bit.

Constantin/Hynek: any thoughts?

I think this is relatively elegant way of class composition in Java (but miles away from Scala :-)). The most important part however will be to come up with a good composition scheme, especially to be careful not to do the code even more complicated.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Please confirm this through testing, if you haven't already. But, what if AUTO-RESIZE is set AFTER these attributes are set (and also in case the SIZE option is used), and only after that the FONT or FORMAT is changed - how is the widget resized?

Yes, AUTO-RESIZE can be set to true after is was set to false after explicit sizing. I do not understand the problem with resizing after that. If the resize happens because of the FORMAT or LABEL change the new size is calculated based on new values of these attributes. On the FONT change it is recalculated based on the existing dimentions.
Please nore however that resizing is the default behavior so I didn't consider the detailed examination of resizing logic as a part of this task.

OK, I understand, but at least I missed checking the auto-resizing behaviour for the widgets I've been implementing (FILL-IN, EDITOR, LABEL, TEXT, FRAME). So I can't confirm they are working properly when the FONT is changed - the other (LABEL, FORMAT) don't affect the "reference font", so they should be OK. Also, I think the BaseConfig.fixedWidth/Height fields are a "replacement" for the AUTO-RESIZE state, as until now we were using these to determine if the widget's size attributes were explicitly set...

For EDITOR case (static widget), its size can be set only via the SIZE option (and after that the size attributes can be used, too). What I would like to see is what rules 4GL uses to re-size the EDITOR (and other similar widgets which require the SIZE option) - how is the new size computed.

This is not correct. The EDITOR size can be specified via INNER-CHARS/INNER-LINES options.

I think you mean it can be specified via INNER-CHARS/INNER-LINES options, too. :) The following code produces an EDITOR with the exact 20 by 20 size:

DEF VAR ch AS CHAR VIEW-AS EDITOR SIZE 20 BY 20.

DISPLAY ch WITH FRAME f1.

MESSAGE ch:WIDTH-CHARS ch:HEIGHT-CHARS.

Please see my response regading the resize logic above. Should I consider a detailed analysis of the dimensions' recalculation as a part of this task?

Yes, I think this is needed. Focus on changing the size by assigning width/height attributes (pixels or char), change the FONT and see how the new size is computed...

Hynek Cihlar wrote:

I think this is relatively elegant way of class composition in Java (but miles away from Scala :-)). The most important part however will be to come up with a good composition scheme, especially to be careful not to do the code even more complicated.

Of course one cannot implement mixin pattern in Java as elegant as it is in Scala. I also agree that this should be done with care. However my main concern was about unification. At this moment GUI widgets share almost nothing GUI specific.

Constantin Asofiei wrote:

OK, I understand, but at least I missed checking the auto-resizing behaviour for the widgets I've been implementing (FILL-IN, EDITOR, LABEL, TEXT, FRAME). So I can't confirm they are working properly when the FONT is changed - the other (LABEL, FORMAT) don't affect the "reference font", so they should be OK. Also, I think the BaseConfig.fixedWidth/Height fields are a "replacement" for the AUTO-RESIZE state, as until now we were using these to determine if the widget's size attributes were explicitly set...

Yes, I also have an impression that BaseConfig.fixedWidth/Height fields should be revised with AUTO-RESIZE semantics in mind. I'm not sure however that now it is a right time for this.

Yes, I think this is needed. Focus on changing the size by assigning width/height attributes (pixels or char), change the FONT and see how the new size is computed...

OK, but it will take some time.

Igor Skornyakov wrote:

Hynek Cihlar wrote:

I think this is relatively elegant way of class composition in Java (but miles away from Scala :-)). The most important part however will be to come up with a good composition scheme, especially to be careful not to do the code even more complicated.

Of course one cannot implement mixin pattern in Java as elegant as it is in Scala.

My lament wasn't targeted to your idea but towards the limitations of Java language itself.

I also agree that this should be done with care. However my main concern was about unification. At this moment GUI widgets share almost nothing GUI specific.

I think the original idea was to minimize the GUI specific code.

I also agree that this should be done with care. However my main concern was about unification. At this moment GUI widgets share almost nothing GUI specific.

I think the original idea was to minimize the GUI specific code.

Yes. The common 4GL behavior of the widget is more important than the GUI-specific portions. That is why we inherit from a base class that is neither ChUI or GUI.

I completely agree about the limitations of Java. I also think we will move to Scala for some usage in the future, though on initial use it may be for a reimplemention of TRPL. I see great value in using it for converted code too, but this must be done very carefully. The biggest problem with Scala is that some features can actually make the code very hard to reason about and/or read/understand. I think they tried to do a bit too much with the language and added some features that are a "net negative" in my opinion. For example, I believe their co-variant/contra-variant syntax and behavior is so complicated that it makes the resulting code hard to reason about. If and when we move to Scala, we will probably take the approach to use only "the good parts" and to keep the code closer to Java. I think this will also help Java programmers pick up the code and make changes without breaking things.

Anyway, Scala is something for the future.

I appreciate all the ideas. We will come back to this and leverage it later.

1. for FILL-IN, the format
2. for EDITOR, the specified (or computed) INNER-LINES and INNER-CHARS
3. for BUTTON - I guess the width of its label
4. etc

So my assumption that the initial size set at the SIZE option gets preserved is incorrect. Please finish the review notes, do some more testing with the existing widgets if the AUTO-RESIZE is working properly and post all the issues here. We will defer any complex issues you find.

Constantin Asofiei wrote:

Igor, I did some testing and I think you are somehow correct: when AUTO-RESIZE is ON and the FONT gets changed, 4GL uses as reference (regardless of the current set size):

1. for FILL-IN, the format
2. for EDITOR, the specified (or computed) INNER-LINES and INNER-CHARS
3. for BUTTON - I guess the width of its label
4. etc

So my assumption that the initial size set at the SIZE option gets preserved is incorrect. Please finish the review notes, do some more testing with the existing widgets if the AUTO-RESIZE is working properly and post all the issues here. We will defer any complex issues you find.

Thank you Constantin.
Actually I've done with most of the issues you've mentioned in the code review. Only those ones related to EDITOR are have to be addressed.

Igor Skornyakov wrote:

Actually I've done with most of the issues you've mentioned in the code review. Only those ones related to EDITOR are have to be addressed.

I'll review 2567b rev 10963 shortly. Please point me to a test for EDITOR/BUTTON/COMBO-BOX you were using for AUTO-RESIZE.

I'll commit any issues directly to 2567b, so (hopefully) you'll be able to rebase next.

Constantin Asofiei wrote:

I'll review 2567b rev 10963 shortly. Please point me to a test for EDITOR/BUTTON/COMBO-BOX you were using for AUTO-RESIZE.

The tests are are.p arbtn.p and arcb.p on uast/auto_resize.

I'll commit any issues directly to 2567b, so (hopefully) you'll be able to rebase next.

I will commit the updated code right now.

Resolved the issues mentioned in the code review except ones related to the EDITOR widgets (sections 14-17).

Committed to branch 2567b revno 10964.

• fixed - AutoResizable interface is only for client-side; it has no usage on server-side, and AutoResizable.resetAutoResize will never be executed from server-side. So BaseEntity.resetAutoresize needs to be removed.
• fixed - TopOnlyInterface.isTopOnly and also isOverlay should return a logical value, not boolean.
• ButtonImpl, ComboBoxImpl, FillInImpl, ToggleBoxImpl require history entry
• ButtonImpl.updateSize: are you sure this test is correct?
  

        if (StringHelper.equal(config.label, beforeUpdate.label))
        {
           config.widthChars = width();
           config.heightChars = height();
        }
  

  
  Shouldn't we update only if the labels are different? Same for updateSize in FillinImpl, ButtonGuiImpl: it checks for equality...
• BaseConfig.fixedWidth/fixedHeight: I'm pretty sure these need to be set to false if AUTO-RESIZE is set back on, and then the size changed via FONT/FORMAT/etc
• runnig the demo/demo_widgets.p results in an NullPointerException with your changes. Please check all the tests in testcaes/uast/demo and see if they work. There are some existing issues, so report any findings which don't seem related to your changes before attempting to fix them.

• the EDITOR:BOX attribute manages if there is a box or not...
• EditorContent: this is the exact area where the content is displayed; it is computed without any scrollbars or border. So the code in there should remain unchanged. I suspect that beside changing the size of the EditorGuiImpl instance, the child widgets (scrollpane, etc) should be resized, too.
• beside this, the logic in adjustSize makes sense, as it relies on inner-chars/inner-lines to compute the editor's size. But, if we have scrollbars at play, things may work differently... as I recall, scrollbars are not included in inner-chars/lines, but are included in EDITOR's full size.

Anyway, please re-test with more complex GUI tests to determine if anything else is broken. Place a TODO in Editor.adjustSize that the logic may be incomplete, we will revise it later.

Constantin Asofiei wrote:

Anyway, please re-test with more complex GUI tests to determine if anything else is broken. Place a TODO in Editor.adjustSize that the logic may be incomplete, we will revise it later.

Thank you Constantin.

Created server-side counterpart of the AutoResizable interface.

Committed to branch 2567b revno 10966

Finished with resolving issues mentioned in the code review and re-tested.

Committed to the branch 2567a revno 10968.

Started rebase.

Rebased task branch 2567b from P2J trunk revision 10941 (new branch revision 10973).

Started re-testing

After rebase:
1. COMBO-BOX redraw on the FORMAT change is incorrect in the GUI mode.
2. EDITOR ignores BGCOLOR attribute in the GUI mode
3. FILL-IN redraw on the FORMAT change is incorrect in the GUI mode.
4. SELECTION-LIST redraw on the FONT change is still incorrect (see #2741). Moreover intitial draw is now incorrect as well.
5. TEXT redraw on the FORMAT change is incorrect in the GUI mode.
6. TOGGLE-BOX doesn't resize on the FONT change.

Which of these problems are new?

Greg Shah wrote:

Which of these problems are new?

I don't remember exactly but it seems that only the issue with the TOGGLE-BOX is not new in the sense that I had fixed it before. However they can be a result of my changes merged with the trunk.

Here is the plan:

1. Add these items to #2741. They can be fixed together.
2. Constantin will do a code review when he gets in tomorrow.
3. Please start regression testing (both conversion and runtime). This can be started before the code review is done.

Greg Shah wrote:

Here is the plan:

1. Add these items to #2741. They can be fixed together.
2. Constantin will do a code review when he gets in tomorrow.
3. Please start regression testing (both conversion and runtime). This can be started before the code review is done.

I've updated #2741.
However when I login to the devsrv01 I see a message *** System restart required ***.
Can I start regression testing?
Thank you.

However when I login to the devsrv01 I see a message * System restart required *.
Can I start regression testing?

Yes, please do. I don't know when we will schedule the reboot, but it is not needed for functional reasons.

Greg Shah wrote:

Yes, please do. I don't know when we will schedule the reboot, but it is not needed for functional reasons.

Thank you.
I've started the conversion on devsrv01 and configuration on my local machine. As it is late in Moscow I will continue tomorrow.

1. you need to rebase again
2. AutoResizableWidget, TopOnlyInterface, AutoResizable need class javadoc
3. ButtonWidget,ButtonImpl,ComboBox,ComboBoxImpl,ButtonImpl,ToggleBoxImpl,FillInImpl,EditorImpl require history entry
4. FillInImpl.updateSize - is it correct to check for equality:
   

         if (StringHelper.equal(config.format, beforeUpdate.format))
         {
            updateSize();
         }
   

   
   Same in ButtonImpl.updateSize:
   

         if (StringHelper.equal(config.label, beforeUpdate.label))
         {
            config.widthChars = width();
            config.heightChars = height();
         }
   

   
   Same for ButtonGuiImpl.updateSize:
   

         if (StringHelper.equal(config.label, beforeUpdate.label) ||
             config.font != beforeUpdate.font)
         {
            config.widthChars = width();
            config.heightChars = height();
         }
   

   
   Same for FillinGuiImpl.updateSize:
   

        if (StringHelper.equal(config.format, beforeUpdate.format ) || 
             config.font !=  beforeUpdate.font)
         {
            updateSize();
         }
   

Thank you Constantin.
I will fix it. Of course the equality check is incorrect. Strangely the behaviour was correct in my tests. I will double check.

Fixed issues mentioned in the code review (except rebase).

Committed to branch 2567b revno 10974.

Did conversion regression testing pass?

Have you started runtime regression testing yet?

Greg Shah wrote:

Did conversion regression testing pass?

Have you started runtime regression testing yet?

The conversion passed OK. The runtime regression is still running (both locally and on devsrv01)

Igor Skornyakov wrote:

Fixed issues mentioned in the code review (except rebase).

Committed to branch 2567b revno 10974.

I'm OK with the changes. rev 10974 contains a small formatting fix.

Rebased task branch 2567b from P2J trunk revision 10942 (new branch revision 10976).

Regression test on devsrv01 ended. The numeber of failed tests is reasonable for the first run.
However there a plenty of exceptions in client-side logs like this:

ct 02, 2015 5:46:19 AM Dispatcher.processInbound() 
SEVERE: {main} Unexpected throwable.
java.lang.reflect.InvocationTargetException
        at sun.reflect.GeneratedMethodAccessor22.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:703)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:257)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1128)
        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.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1425)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:97)
        at com.sun.proxy.$Proxy4.standardEntry(Unknown Source)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:277)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:100)
        at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:201)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:398)
        at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:95)
        at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:267)
Caused by: java.io.EOFException
        at com.goldencode.p2j.util.Stream.readLineCleanup(Stream.java:5120)
        at com.goldencode.p2j.util.FileStream.readLn(FileStream.java:517)
        at com.goldencode.p2j.util.StreamDaemon.readLn(StreamDaemon.java:615)
        ... 20 more

Oct 02, 2015 5:46:19 AM Dispatcher.processInbound() 
SEVERE: {main} Unexpected throwable.
java.lang.reflect.InvocationTargetException
        at sun.reflect.GeneratedMethodAccessor22.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:703)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:257)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1128)
        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.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1425)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:97)
        at com.sun.proxy.$Proxy4.standardEntry(Unknown Source)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:277)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:100)
        at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:201)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:398)
        at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:95)
        at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:267)
Caused by: java.io.EOFException
        at com.goldencode.p2j.util.Stream.readLineCleanup(Stream.java:5120)
        at com.goldencode.p2j.util.FileStream.readLn(FileStream.java:517)
        at com.goldencode.p2j.util.StreamDaemon.readLn(StreamDaemon.java:615)
        ... 20 more

Please note that client logs are polluted with warnings like this:

DRAW: Ignoring event com.goldencode.p2j.ui.client.event.PaintEvent 9 raised while drawing widget com.goldencode.p2j.ui.client.widget.Viewport with id [-1].

May be it makes sense to make possible suppressing these warning?

The EOFException instances are expected. They are not a problem. We need to add EOFException.class to the Dispacher.criteria array and those will no longer be reported.

The PaintEvent logging is for debugging purposes. It isn't a problem with your update. Just leave it for now.

Please document the regression testing status here.

The following tests failed identically in 3 runs:

133 gso_281 GSO 281 - Column error message in PO add items. FAILED NONE BACKOUT Driver 5 181.651

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), template: screens/gso/gso_281/po_add_step1.txt.)'

152 gso_307 GSO 307 - Screen does not refresh at Stock Order Selection Screen. FAILED NONE BACKOUT Driver 0 221.880

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), template: screens/gso/gso_307/stock_order_selection_screen_step11.txt.)'

221 gso_394 GSO 394 - User returned to Inv Inquiry when F4 selected at Maint Stock Order Status screen. FAILED NONE BACKOUT Driver 6 182.226

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), template: screens/gso/gso_394/maint_stock_order_status_step4.txt.)'

222 gso_395 GSO 395 - Abend at MAINTENANCE STOCK ORDER REQUEST screen Proxeee Playback 021 Exception 3. FAILED NONE BACKOUT Driver 0 247.477

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), template: screens/gso/gso_395/maintenance_stock_order_step17.txt.)'

5 gc_62 GC 62 - Client invocation abend on selection list call. FAILED NONE BACKOUT Driver 0 188.808

failure in step 34: 'timeout before the specific screen buffer became available (Mismatched data at line 1, column 10. Expected '┌' (0x250C at relative Y 1, relative X 10) and found ' ' (0x0000 at relative Y 1, relative X 10), template: screens/gc/gc_62/po_f7_abend_step17.txt.)'

67 tc_inquiry_inventory_017 TIMCO TC-INQUIRY-INVENTORY-017 testcase. FAILED SEQUENTIAL BACKOUT Driver 10 189.615

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), template: screens/tc/inquiry/inventory/017/inquiry_inventory_017_step3.txt.)'

396 tc_job_002 TIMCO TC-JOB-002 testcase. FAILED NONE BACKOUT Driver 1 62.656

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

505 tc_po_012 TIMCO TC-PO-012 testcase. FAILED SEQUENTIAL BACKOUT Driver 3 193.334

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), template: screens/tc/po/012/po_012_step12.txt.)'

509 tc_po_item_003 TIMCO TC-PO-ITEM-003 testcase. FAILED SEQUENTIAL BACKOUT Driver 2 188.222

failure in step 19: '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), template: screens/tc/po/item/003/po_item_003_step5.txt.)'

510 tc_po_item_004 TIMCO TC-PO-ITEM-004 testcase. FAILED_DEPENDENCY SEQUENTIAL BACKOUT
0.000

dependency chain: tc_po_item_003

3 gc_63 GC Product Code Issue testcase. FAILED NONE BACKOUT Driver 0 255.748

failure in step 49: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 27. Expected '┌' (0x250C at relative Y 5, relative X 27) and found ' ' (0x0000 at relative Y 5, relative X 27), template: screens/gc/gc_63/codes_step11.txt.)'

4 gc_64 GC 64 Create and Issue PICK in MAJIC testcase. FAILED NONE BACKOUT Driver 0 209.488

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), template: screens/tc/common/inventory_inquiry_screen.txt.)'

Investigating.

As noted previously the tc_job_002, tc_po_item_003, tc_po_item_004 are all known failures which can be ignored.

The others need to be fixed as your top priority.

Greg Shah wrote:

As noted previously the tc_job_002, tc_po_item_003, tc_po_item_004 are all known failures which can be ignored.

The others need to be fixed as your top priority.

I understand. I'm working on it right now.

Regarding gso_281. The only mismatch I see is the value of the PO field. It is 662622 instead of 662621. I think I had this issue before and it dissapeared `by itself`.

Regarding gso_281. The only mismatch I see is the value of the PO field. It is 662622 instead of 662621. I think I had this issue before and it dissapeared `by itself`.

Agreed. That is probably not an issue.

The other failed gso_* tests also look familiar:
152 gso_307 GSO 307 - Screen does not refresh at Stock Order Selection Screen. FAILED NONE BACKOUT Driver 0 221.880

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), template: screens/gso/gso_307/stock_order_selection_screen_step11.txt.)'
wait = true; millis = '180000'; failing screen =

10/02/2015                      INVENTORY INQUIRY                       05:51:19
┌──────────────────────────────── Item Master ─────────────────────────────────┐
│Item: 0A0001ERN       Mfg P/N: PS870B1/2-6OZ            On Hand:    1,112.000 │
│ Description: PS870B1/2-6OZ SEALANT 6 OZ #654 SEMKIT        U/M: EA           │
│   2nd Desc.: 5512694999 BMS5-95TY1CLB1-2 MIL-PRF81733 On Order:      240.000 │
│    Alt Item: BMS5┌─────── Stock Order Information ───────┐ Job:        0.000 │
│Product Code: PGC │                                       │ Qty:    1,112.000 │
│        Type: M (M│ Order #:                    Stat:     │tock:    1,188.000 │
│ Drawing Nbr:     │     S/O: 125329  Oper: 1     Rtg:     │able: no           │
│Hazmat Types: #$FH│ Qty Required:           1.00 EA       │Time:    6  days   │
│Current Site: GSO │ Pickup Local: H2   HANGAR 2           │otes: yes          │
└──────────────────│                                       │───────────────────┘
              ┌────└───────────────────────────────────────┘─────┐
              │Site Fac  Location             On Hand Expires MRB│
              │──── ──── ─────────────── ──────────── ─────── ───│
              │                                                  │
              │GSO       IC                     0.000         Yes│
              │GSO  A    STOCK                  0.000         Yes│
              │GSO  B    STOCK                  0.000         Yes│
              └──────────────────────────────────────────────────┘

 ***  ORDER NOT PRINTED  *** 

Enter a valid Service Order
06/11/2009                      INVENTORY INQUIRY                       13:37:09

┌──────────────────────────────── Item Master ─────────────────────────────────┐
│Item: 0A0001ERN       Mfg P/N: PS870B1/2-6OZ            On Hand:    1,112.000 │
│ Description: PS870B1/2-6OZ SEALANT 6 OZ #654 SEMKIT        U/M: EA           │
│   2nd Desc.: 5512694999 BMS5-95TY1CLB1-2 MIL-PRF81733 On Order:      241.000 │
│    Alt Item: BMS5┌─────── Stock Order Information ───────┐ Job:        0.000 │
│Product Code: PGC │                                       │ Qty:    1,112.000 │
│        Type: M (M│ Order #:                    Stat:     │tock:    1,188.000 │
│ Drawing Nbr:     │     S/O: 125329  Oper: 1     Rtg:     │able: no           │
│Hazmat Types: #$FH│ Qty Required:           1.00 EA       │Time:    6  days   │
│Current Site: GSO │ Pickup Local: H2   HANGAR 2           │otes: yes          │
└──────────────────│                                       │───────────────────┘
              ┌────└───────────────────────────────────────┘─────┐
              │Site Fac  Location             On Hand Expires MRB│
              │──── ──── ─────────────── ──────────── ─────── ───│
              │                                                  │
              │GSO       IC                     0.000         Yes│
              │GSO  A    STOCK                  0.000         Yes│
              │GSO  B    STOCK                  0.000         Yes│
              └──────────────────────────────────────────────────┘

 Press any key for corrections or ENTER to print

221 gso_394 GSO 394 - User returned to Inv Inquiry when F4 selected at Maint Stock Order Status screen. FAILED NONE BACKOUT Driver 6 182.226

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), template: screens/gso/gso_394/maint_stock_order_status_step4.txt.)'

wait = true; millis = '180000'; failing screen =

10/02/2015 05:58           MAINT STK ORDER STATUS       ENTER BADGE NBR:        

 Enter period s(.) to quit 

06/22/2009 13:01           MAINT STK ORDER STATUS       ENTER BADGE NBR:

222 gso_395 GSO 395 - Abend at MAINTENANCE STOCK ORDER REQUEST screen Proxeee Playback 021 Exception 3. FAILED NONE BACKOUT Driver 0 247.477

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), template: screens/gso/gso_395/maintenance_stock_order_step17.txt.)'

wait = true; millis = '180000'; failing screen =

10/02/2015 05:59           MAINT STK ORDER STATUS       ENTER BADGE NBR:        

 Enter period s(.) to quit 

06/25/2009 14:13           MAINT STK ORDER STATUS       ENTER BADGE NBR:

The firsta failure which looks like a result of my changes is gc_62

Ctrl-C tests passed

1. Strangely enough gso_281 and gso_307 still fail due to a wrong value of the field.
2. The situation with gc_62 is also unclear. The mismatched screen is (I see it in the manual testing as well):

10/02/201┌───────────────Select Standard PO Text Blocks───────────────┐ 05:38:50
┌────────│                                                             ────────┐
│    PO: │                                                             ed)     │
│        │                                                             gular)  │
│Vendor: │                                                                     │
│        │                                                                     │
│        │                                                                     │
│        │                                                                     │
│        │                                                                     │
│        │                                                                     │
│        │                                                             0       │
│     Buy│                                                                     │
│ Request│                                                                     │
│Vendor O│                                                                     │
│        │                                                                     │
│       T│                                                                     │
│        │                                                                     │
│Payment │                                                                     │
│Notes: (│                                                             uy?: no │
└────────│                                                             ────────┘
         └────────────────────────────────────────────────────────────┘

08/21/201┌───────────────Select Standard PO Text Blocks───────────────┐ 15:41:15
┌────────│┌──────────────────────────────────────────────────────────┐ ────────┐
│    PO: ││    1 - Drug Program                                        ed)     │
│        ││    2 - Hazmat Customer (will carry)                        gular)  │
│Vendor: ││    3 - Hazmat Customer (will NOT carry)                            │
│        ││    4 - Direct Ship Authority                                       │
│        ││    5 - 8130-3 Required                                             │
│        ││    6 - 8130-3 with Dual Release Required                           │
│        ││    7 - Confidential and Proprietary (Email)                        │
│        ││    8 - Confidential and Proprietary (Document)                     │
│        ││    9 - EASA Registered Aircraft                            0       │
│     Buy││   10 - Tops Cleaners Statement                                     │
│ Request││   11 - Do Not Use PMA Parts                                        │
│Vendor O││   12 - DPAS Statement                                              │
│        ││   13 - Shelf Life Statement                                        │
│       T││   14 - Pass Through For Customer                                   │
│        ││                                                                    │
│Payment ││                                                                    │
│Notes: (││                                                            uy?: no │
└────────│└──────────────────────────────────────────────────────────┘ ────────┘
         └────────────────────────────────────────────────────────────┘

The coorresponding class is aero.timco.majic.po.TxtBlockSelect (po/po.p). The frame which is empty is TxtBlockSelectF1. It contains SelectionListWidget, SkipEntity and two instances of the ButtonWidget. None of them where affected by my changes (only resetAutoResize() method was added to the ButtonWidget). Please note that buttons are not seen on the expected screen (but I can see them if the terminal size is larger). Nothing unusual can be seen in both client and server logs.

Continue investigation.

Resolved the issue with gc_62 (manual test passed).

Committed to branch 2567b revno 10977

Restarted regression test

After the fix only two tests failed: tc_codes_employees_021 and tc_job_clock_002 (in addition to expected tc_job_002, tc_po_item_003, tc_po_item_004). As far as I remember these tests typically fail when started at a wrong time. Test restarted.

Only expected expected tc_job_002, tc_po_item_003, tc_po_item_004 tests failed in two runs after the fix.
Started ctrl-c tests

Ctrl-C tests passed

Igor, review for 2567b rev 10977. Only one issue about the change in SelectionList.afterConfigUpdate:

//      if (!sizeAdjusted)
//      {
         adjustSize();
//         sizeAdjusted = true;
//      }

Constantin Asofiei wrote:

As I understand, this caused one or more regressions in testing. Please add a comment why the uncommented version in afterConfigUpdate does not work (without mentioning MAJIC tests/code, describe how the test which fails works, eventually duplicate the logic in a separate 4GL test file and put it in the testcases/ project).

Yes, this caused a regression. It took me a whole day to figure that. Of course I can try to reproduce the issue in a standalone test but it can take time.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

As I understand, this caused one or more regressions in testing. Please add a comment why the uncommented version in afterConfigUpdate does not work (without mentioning MAJIC tests/code, describe how the test which fails works, eventually duplicate the logic in a separate 4GL test file and put it in the testcases/ project).

Yes, this caused a regression. It took me a whole day to figure that.

Please specify which test failed.

Of course I can try to reproduce the issue in a standalone test but it can take time.

OK, defer it for now.

Constantin Asofiei wrote:

Please specify which test failed.

It was gc_62. Please see a detailed description above.

Please add the comments as described in note 412, if it is clear enough Constantin will give the go ahead to merge to trunk.

Added comment to a recent fix.

Committed to branch 2567b revno 10978

The branch is ready for merge.

Igor Skornyakov wrote:

Added comment to a recent fix.

Committed to branch 2567b revno 10978

The branch is ready for merge.

The selection-list code in GC 62 is in the po/txt-block-select.p.cache program. I've changed the comment to this in rev 10979:

      // when enabled, the following check is incorrect.
      // It results in a zero widget size in some situations: it might be related to using the
      // INNER-CHARS/LINES or scrollbar options set at the selection-list definition.

Please update your 2567b branch to rev 10979 and merge to trunk.

Constantin Asofiei wrote:

Please update your 2567b branch to rev 10979 and merge to trunk.

Sorry, but it seems that the current revno of the trunk is 10942. What do you mean talking about upgrade to revno 10979?
Thank you.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Please update your 2567b branch to rev 10979 and merge to trunk.

Sorry, but it seems that the current revno of the trunk is 10942. What do you mean talking about upgrade to revno 10979?
Thank you.

I meant to do a bzr update in your copy of branch 2567b so you have rev 10979 which I've committed to this branch... I was not referring to trunk.

Constantin Asofiei wrote:

I meant to do a bzr update in your copy of branch 2567b so you have rev 10979 which I've committed to this branch... I was not referring to trunk.

Got it. Thank you. Sorry for misunderstanding.

Tha branch 2567b has been merged to P2J trunk revision 10943