FWD » Core Development » User Interface

After you have done some initial analysis, please organize the work into "sections" which can be logically separated from each other. My idea is for you to be able to regression test/check-in the work in this task in phases, rather than building up a massive single update.

Greg Shah wrote:

After you have done some initial analysis, please organize the work into "sections" which can be logically separated from each other. My idea is for you to be able to regression test/check-in the work in this task in phases, rather than building up a massive single update.

OK - I completely agree that a single update is not practical.

Code Review ias_upd20150327a.zip

1. Please change BoxedWidget to BoxInterface. Please also change is/setBoxed() to is/setBox(). This is more consistent with the Progress terminology which makes it easier for customers to understand the Java implementation. The use if "Interface" instead of Widget is more consistent with the fact that we generally try to only have the server-side widget class use the suffix "Widget".

2. BoxedWidget is missing a file header.

3. handle.java doesn't need unwrapEditorInterface() because it already has unwrapEditor() which returns an EditorInterface. This just means that the interface type in methods_attributes.rules should be specified as Editor instead of EditorInterface.

4. HandleCommon needs to be updated with the new BoxInterface.

5. returnInserted needs to be be added to the processing in EditorConfig.apply().

6. In EditorWidget.setSelection(int64 start, int64 end), if end is unknown what does the 4GL do?

7. I see that EditorWidget.getCursor*() methods now call down to the client to read/write the cursor values. Since the values it relies upon are synched to the server, can't we just rely upon the server state to calculate that value for reading? If so, then we can avoid a round trip to the client side which is expensive.

8. Please replace (non-Javadoc) with the copied javadoc from the interface definitions. Optimally, these should be enhanced with specific details about the editor implementation.

9. The javadoc in ClientExports, LogicalTerminal and ThinClient has some trash (``````````) characters in several places.

Greg Shah wrote:

Code Review ias_upd20150327a.zip

1. Please change BoxedWidget to BoxInterface. Please also change is/setBoxed() to is/setBox(). This is more consistent with the Progress terminology which makes it easier for customers to understand the Java implementation. The use if "Interface" instead of Widget is more consistent with the fact that we generally try to only have the server-side widget class use the suffix "Widget".

2. BoxedWidget is missing a file header.

3. handle.java doesn't need unwrapEditorInterface() because it already has unwrapEditor() which returns an EditorInterface. This just means that the interface type in methods_attributes.rules should be specified as Editor instead of EditorInterface.

4. HandleCommon needs to be updated with the new BoxInterface.

5. returnInserted needs to be be added to the processing in EditorConfig.apply().

OK, I will do that.

6. In EditorWidget.setSelection(int64 start, int64 end), if end is unknown what does the 4GL do?

It returns 'true' and sets selection to 'unknown' (like in my code).

7. I see that EditorWidget.getCursor*() methods now call down to the client to read/write the cursor values. Since the values it relies upon are synched to the server, can't we just rely upon the server state to calculate that value for reading? If so, then we can avoid a round trip to the client side which is expensive.

Both getCursor* (except getLine) and setCursor* methods' implementation depends on the content - this is why I decided to delegated them to the client side. I understand that it is expensive but my assumption was that these methods will not be called frequently. Of course it is possible to adjust these values at every content change but in this case on each content change we'll have to send config to the server which is also not cheap.

8. Please replace (non-Javadoc) with the copied javadoc from the interface definitions. Optimally, these should be enhanced with specific details about the editor implementation.

9. The javadoc in ClientExports, LogicalTerminal and ThinClient has some trash (``````````) characters in several places.

I will fix it.

Both getCursor* (except getLine) and setCursor* methods' implementation depends on the content - this is why I decided to delegated them to the client side. I understand that it is expensive but my assumption was that these methods will not be called frequently. Of course it is possible to adjust these values at every content change but in this case on each content change we'll have to send config to the server which is also not cheap.

I believe that the config.* values are already being edited for every content change.

Those changes automatically get synchronized with the server the next time the control flow of the program shifts to the server. Thus, there is no extra network trips needed for this sync to occur. Unless something is not properly coded in the client-side editor, this sync is already happening.

I believe that the config.* values are already being edited for every content change.

Those changes automatically get synchronized with the server the next time the control flow of the program shifts to the server. Thus, there is no extra network trips needed for this sync to occur. Unless something is not properly coded in the client-side editor, this sync is already happening.

OK - I will double check and change my implementation.

Greg,
I suggest to leave setCursor* as now - this will be more efficient then to send the entire config. In addition the the new value can be correctly validated only at the client side (as a content is needed). What do you think?

Igor Skornyakov wrote:

Greg,
I suggest to leave setCursor* as now - this will be more efficient then to send the entire config. In addition the the new value can be correctly validated only at the client side (as a content is needed). What do you think?

Agreed. For these, we have to take a trip to the client anyway to re-draw/apply the changes synchronously. So we there is no efficiency to be gained.

Code Review ias_upd20150331a.zip

The code looks good. The only (minor) issue is see is that Editor.setCursorChar(int) has an extra javadoc param.

To achieve that the logic of deleting chars (including remove part of the replace operation) should be re-implemented - the parseContent method should take into consideration not just the starting line but the start/end of the affected interval (or a new method just for deleting should be implemented). The replace operations must re-parse content twice: after deleting and after insert step. Such an approach can be even more effective than existing one (at least in some situation) as potentially a smaller part of the content will be re-parsed.

This can be implemented/tested in a day or two.

I agree. Please implement this.

Code Review ias_upd20150409a.zip

Generally, I'm good with the changes. In some ways the result is cleaner and in some other ways it is more complex. If it is more correct overall, then it is well worth it. Hopefully it will pass regression testing.

Some trivial code formatting issues in Editor.java:

1. Line.setOffset() needs javadoc.
2. There should be a blank line after rtrim() and mergeLines().
3. The content of rtrim() is indented too much.
4. splitLine() should be indented 3 spaces instead of 2.

Greg Shah wrote:

Generally, I'm good with the changes. In some ways the result is cleaner and in some other ways it is more complex. If it is more correct overall, then it is well worth it. Hopefully it will pass regression testing.

I agree that the new approach is more complicated. However the natural idea of a global reformatting after content changing simply isn't compatible with 4GL behavior. For example replacing fragment with a new content and manual deleting it and typing-in the replacement not always result in the same screen. Moreover, even logically equivalent manual changes not always result in the same screen (even single insert/delete may not commute visually). I was considering to have 'soft LFs' to retain the idea with global reformatting but it looks prohibitively expensive and will require even more code changes.

How compatible are we? Please document any areas where differences remain (as far as you know).

Greg Shah wrote:

How compatible are we? Please document any areas where differences remain (as far as you know).

Well, at this moment I know only one incompatibility - if the number of lines reduces substantially after massive REPLACE operation 4GL leaves widget in a strange state which is (partially) fixed after it gains focus. I planned to make a number of additional tests but after some break. If you don't mind I will now concentrate on the rest of this task and will return to editor at the next week.

I agree. Actually, if you think the described behavior is rare enough that it is less likely to be encountered AND if there is no strong functional reason for the quirk to be replicated, then we can wait to add this later (as part of a different task).

Greg Shah wrote:

I agree. Actually, if you think the described behavior is rare enough that it is less likely to be encountered AND if there is no strong functional reason for the quirk to be replicated, then we can wait to add this later (as part of a different task).

I think that this particular quirk should be fixed either when it will be really needed or if it will be a part of a more general change. Otherwise the code will be polluted with ad hoc changes which may never be needed in real life.

Please create a new task that has a recreate (both a 4GL testcase and a set of instructions) for the problem.

Code Review ias_upd20150410a.zip

This looks good.

Code Review ias_upd20150411a.zip

The change looks fine.

All regression tests (except tc_job_002) passed.

Please check it in.

As far as I understand the SET-SELECTION method (and its companion SELECTION-TEXT attribute) for COMBO-BOX depends on the FORMAT attribute. so I think it makes sense to add a support for this attribute as well. This attribute is meaningful for DROP-DOWN-LIST combo-boxes only, but this is the only combo-box type supported for ChUI.

ComboBoxConfig is a ControlSetConfig which already supports the FORMAT attribute. Is that not sufficient?

Greg Shah wrote:

ComboBoxConfig is a ControlSetConfig which already supports the FORMAT attribute. Is that not sufficient?

We have format field on configs but not access methods in widgets. Moreover according to 4GL documentation for the COMBO-BOX (with an DROP-DOWN-LIST type) assignment to this attribute causes the reformatting of values (I don't know now what this mean exactly).

GenericWidget.setFormat() calls WidgetConfig.setDynamicFormat() which should set the ControlSetConfig.format.

Greg Shah wrote:

GenericWidget.setFormat() calls WidgetConfig.setDynamicFormat() which should set the ControlSetConfig.format.

I see. Sorry, I haven't noticed that - I was looking for the annotated setter/getter.

I was looking for the annotated setter/getter

That annotation does look like it is missing in CommonWidget.setFormat(String). We are also missing the setFormat(character) variant in CommonFrame (it already exists in GenericWidget). Please add these.

Greg Shah wrote:

I was looking for the annotated setter/getter

That annotation does look like it is missing in CommonWidget.setFormat(String). We are also missing the setFormat(character) variant in CommonFrame (it already exists in GenericWidget). Please add these.

OK. BTW: when we implement explicit setters/getters and when it is better to do it via reflection API (like in 'format' case)?

Greg Shah wrote:

I was looking for the annotated setter/getter

That annotation does look like it is missing in CommonWidget.setFormat(String). We are also missing the setFormat(character) variant in CommonFrame (it already exists in GenericWidget). Please add these.

CommonWidget.setFormat should be removed, as it collides with CommonField.setFormat (which is properly annotated). If new versions are needed, then add them, to CommonField, not CommonWidget.

On a side note, CommonFrame doesn't need to have its APIs annotated, as this interface doesn't map a legacy resource. FrameWidget is responsible for 4GL-like access to FRAME's attributes.

OK. BTW: when we implement explicit setters/getters and when it is better to do it via reflection API (like in 'format' case)?

For instance fields in the WidgetConfig hierarchy, you don't need to add setters/getters - actually, we are currently working on removing even the current setColumn/Row/etc APIs. The format case is special and uses reflection to access the config field because it exists in more than one config class - so to avoid maintaining which config class defines this field, we use reflection to set it if exists, otherwise the call will silently fail. All other cases - when we know exactly where the config field resides - must use direct field access.

Contrary to the 4GL docs the INNER-LINES attribute for the COMBO-BOX does have meaning in ChUI - it affects the height of the activated drop-down list.
The setter for the INNER-LINES COMBO-BOX attribute behavior look like the following.

a. Initial value is 8 (initial value of the HEIGHT-CHARS attribute is 1), length of the last directly assigned (not via DELETE/ADD-LAST/ADD-FIRST) before realization LIST-ITEMS or LIST-ITEM-PAIRS or 1 of such a list was empty.
b. After changing LIST-ITEMS/LIST-ITEM-PAIRS neither INNER-LINES nor HEIGHT-CHARS changes.
c. SIZE clause in the VIEW-AS phrase is silently ignored
d. INNER-LINES clause in the VIEW-AS phrase sets the value of the INNER-LINES attribute but doesn't affect the HEIGHT-CHARS (it is still 1).
e. HEIGHT-CHARS is read-only attribute
f. INNER-LINES attribute is writeable. If the assigned value is less then zero, warning is emitted. If the new value is 'unknown' literal or less then 3 a value 3 is assigned. The 'unlnown' value is not acceptable.
After assignment the HEIGHT-CHARS attribute value is <new value>+2.
The new value N of INNER-LINES attribute depends on the assigned value M as following: there are exist integers N1 < N2 < N3 and r(N): {n| n >=N1} -> {n| n >= 0}
- if M >= N3 for the very first assignment then N = N3
- if M >= N2 for the first assignment after the last widget activation then N = N2.
- if M >= N1 the behavior is more complicated. To describe it we need two auxiliary INTEGER (in 4GL sense) variables X and K.
After each widget activation X = ?, K = 0
If assigned value M >= N1 then K = K+1.
If K == r(M) then X = N = min(M, N2)
If K == r(M) + 1 then N = X
If K > r(M) + 1 then X = N = max(0, X - 1).

The N3 looks to be equal to w:WINDOW:MAX-HEIGHT-CHARS - w:FRAME:ROW where w is the COMBO-BOX widget.
I do not understand how N2 and N1 can be expressed in terms of the widget attributes (N2 seems to be equal to N3 - 2 but I'm not sure). The r(N) function also looks mysterious (seems to be decreasing locally constant with values 4, 3, 2, 1, 0)

Wow, that is not complicated at all. :)

I've added Hynek and Eugenie as watchers. Eugenie is working on ComboBox (GUI implementation) and Hynek has some very useful experience with layout and positioning. Guys: please review note 36 and share any useful insight. I wonder if N2, N1 and r(N) are somehow dependent upon the layout of the surrounding frame.

The algorithm maybe accounts for the widget border, at least in the case of N2?

Hynek Cihlar wrote:

The algorithm maybe accounts for the widget border, at least in the case of N2?

Well, both N3 and N2 depend on the enclosing frame's BOX attribute (if there are no box they are greater by 1). The COMBO-BOX itself doesn't have a border.

1. the combo's drop box (when drawn) can not exceed beyond the status area. This means that the drop-box's height can be a max of screen-lines - w:row - 2, where:
   - w:row is the combo-box's physical row on screen (it has nothing to do with the frame's row...) And do not confuse with the ROW attribute, which provides the widget's position in the parent container, and not in the physical window.
   - screen-lines is the window's height (in GUI this might be the virtual height) minus the status area (1 row). For some reason, this includes the message area, even if is not empty.
   - 2 represents the drop-box's top and bottom border.
2. now, some weird stuff. If the drop-box has not yet been initialized, it can have a height bigger than the above formula. In the test bellow, use this scenario:
   - press "1"
   - press cursor-down to activate the drop-down
   - you will see that its height is bigger than the formula (it the possible max value, 21, as if the widget was on row 1). But even in this case, the drop-down's location is adjusted so that it fits in the window.
3. if the drop-box has been initialized, then the INNER-LINES value is adjusted so that it doesn't exceed the max value. Scenario, for the test bellow:
   - press cursor-down to initialize the drop-box
   - press return to select an item and close the drop-box
   - press "1" again
   - press cursor-down to show the drop-box - now it has 18 rows (and not 22, as we've set INNER-LINES to), so it matches the above formula: 23(avail window rows) - 3(widget position) - 2(border)
4. and the really weird stuff, what I think you were trying to describe in your formula. In the example bellow, use for example 10 instead of 22 when assigning the INNER-LINES in the trigger.

Now, first scenario:
- press "1" until 13 10 12 is shown on the message area
- press "1" again - the drop-down's INNER-LINES starts to decrement by 1.
- press "1" until 23 0 12 is shown on the message area
- press "1" again - you are going into negative values, which breaks 4GL, when you try to activate the drop-down via cursor-down

Next, exit the test and run it again with a window height of 30 chars. The scenario will look like:
- press "1" until 19 10 12 is shown on the message area
- press "1" again - the drop-down's INNER-LINES starts to decrement by 1.
- press "1" until 29 0 12 is shown on the message area
- press "1" again - you are going into negative values, which breaks 4GL, when you try to activate the drop-down via cursor-down

From these, it looks like if the INNER-LINES is set a consecutive number of times without activating the combo's drop-down, then there is some internal counter K, which adjusts INNER-LINES so that K + INNER-LINES does not exceed the max value given by the formula above. Counter K is reset back to 1 each time the drop-down is made visible.

More, looks like this stands even if the consecutive assignments are not to the same value. If, after K + INNER-LINES reaches the maximum value, you set INNER-LINES to a value so that now is K + INNER-LINES is less than the max value, for example, in the trigger (window height 24):

on "1" anywhere do:
  i = i + 1.
  if i = 13 then x:inner-lines in frame f1 = 5.
  else if i >= 14 then x:inner-lines in frame f1 = 6.
  else x:inner-lines in frame f1 = 10.
  message i x:inner-lines in frame f1 x:height-chars in frame f1.
end.

The test:

def var x as char view-as combo-box list-items "a", "d", "e".

form x with frame f1 no-box no-labels size 20 by 5.

x:row in frame f1 = 3.
def var i as int.

i = 1.

on "1" anywhere do:
  i = i + 1.
  x:inner-lines in frame f1 = 22.
  message i x:inner-lines in frame f1 x:height-chars in frame f1.
end.

update x with frame f1.

You can experiment with boxed or no-boxed frames, other widget/frame positions; if you find something to contradict my assumptions, do let me know.

Constantin Asofiei wrote:

You can experiment with boxed or no-boxed frames, other widget/frame positions; if you find something to contradict my assumptions, do let me know.

Thank you very much Constantin! I will check your ideas.

Well, Constantin was right - the rule for the INNER-LINES attribute setter is pretty simple:
<new value> = min(<assigned value>, SCREEN-LINES - (FRAME:ROW + if FRAME:BOX then 1 else 0) + 2 - N - delta)
where N is the total number of assignments (including the last one) after the last COMBO-BOX activation and delta = if (widget was at least once activated) then COMBO-BOX:ROW else 0. It seems that it doesn't matter if the widget was realized or not.

Thank you very much Constantin!

Igor Skornyakov wrote:

Well, Constantin was right - the rule for the INNER-LINES attribute setter is pretty simple:
<new value> = min(<assigned value>, SCREEN-LINES - (FRAME:ROW + if FRAME:BOX then 1 else 0) + 2 - N - delta)
where N is the total number of assignments (including the last one) after the last COMBO-BOX activation and delta = if (widget was at least once activated) then COMBO-BOX:ROW else 0. It seems that it doesn't matter if the widget was realized or not.

Thank you very much Constantin!

You're welcome. But, are you sure you need FRAME:ROW/FRAME:BOX attributes? If the formula is based on widget's location, then you need to rely on that location as it appears on screen, and not on its (or its parent) ROW attribute (note that FRAMEs can parent each other, so do not assume the FRAME's parent is always the WINDOW, the tree hierarchy can be WINDOW->FRAME-FRAME->FRAME->COMBO-BOX). How do you plan to maintain/adjust INNER-LINES? On server-side or client-side?

On client-side there are the Widget.screenLocation()/screenPhysicalLocation() APIs which can give you the location relative to the window, and not the parent containers. On server-side, I don't think we have such APIs yet. My opinion is to call pushScreenDefinition on server-side, after INNER-LINES is set, and let the client-side adjust the INNER-LINES (as the counter for sure needs to be kept on client-side); when the pushScreenDefinition returns, the combo-box's INNER-LINES will be "refreshed" with the adjusted value.

Constantin Asofiei wrote:

You're welcome. But, are you sure you need FRAME:ROW/FRAME:BOX attributes? If the formula is based on widget's location, then you need to rely on that location as it appears on screen, and not on its (or its parent) ROW attribute (note that FRAMEs can parent each other, so do not assume the FRAME's parent is always the WINDOW, the tree hierarchy can be WINDOW->FRAME-FRAME->FRAME->COMBO-BOX). How do you plan to maintain/adjust INNER-LINES? On server-side or client-side?

On client-side there are the Widget.screenLocation()/screenPhysicalLocation() APIs which can give you the location relative to the window, and not the parent containers. On server-side, I don't think we have such APIs yet. My opinion is to call pushScreenDefinition on server-side, after INNER-LINES is set, and let the client-side adjust the INNER-LINES (as the counter for sure needs to be kept on client-side); when the pushScreenDefinition returns, the combo-box's INNER-LINES will be "refreshed" with the adjusted value.

I think you're right and we actually need frame row relative the top of the screen. I tried to write a test with embedded frames but so far I do not understand how to do it. I think that your suggestion is what we need. Thank you very much again!

Today I analyzed the case of nested frames and some other corner cases. Hopefully now we have a complete picture.

Let W - be the COMBO-BOX widget,
F->W - its enclosing frame,
Fn-> ... ->F - n nested frames contaning F (if n == 0 there are no such frames),
N2 = SCREEN-LINES - (F:ROW + if F:BOX then 1 else 0) + 2,
D = SUM(Fi:ROW + (if Fi:BOX then 1 else 0) - 1 | i = 1,...,n) + W:ROW (as usual if n = 0 then SUM == 0 being a summation over an empty set),
N1 = N2 - D,
Vc - the current value of the W:INNER-LINES,
V - the value assigned to the W:INNER-LINES,
K - the number of assignments (including the current one) since the last activation of V,
Vn - the new value of the W:INNER-LINES

1. if W is not realized then Vn = V, after W is realized Vc = max(min(Vc, N2), W:NUM-ITEMS)
2. if W was not activated before then Vn = min(V, N2 - K).
3. if after the last activation Vc <= N1 then Vn = min(V, N1 - K)
4. if after the last activation Vc > N1 then set Nx = Vc. After subsequent assignments Vn = min(V, Nx - K)

A couple of additional comments:

1. I tried hard to understand the underlying logic behind the non-idempotent setter (as it looks really weird for me) but failed. From the other side I think that it will be useful to understand this logic for the future 4GL behavior deciphering (if it is not just a 4GL bug). Does anybody have any ideas?

2. I've noticed that querying a HEIGHT-CHARS attribute results in the COMBO-BOX widget realization. This is not mentioned in the 4GL documentation, but there are other attributes described in the documentation whose setting or getting result in the widget realization. If we want to support this then my question is how it should be done? I'm not sure that just setting the corresponding flag in the widget config is a right approach. Of course I can investigate the problem if required by may be the solution is obvious?

Igor checked in ias_upd20150411a.zip as bzr rev 10833 on April 13, 2015.

Please provide a status update with:

1. The expected time when your next update will be ready for review.
2. A summary of what features are expected in it.
3. The list of any work in this task that will not yet be done.

1. I hope to finish tomorrow or at the weekend.
2. All features mentioned in the task will be done.
3. I've promised to perform additional testing of the EDITOR part and create a corresponding task with remaining corner cases. I plan to do it in a background after finishing with the current work,

Please provide some details about the status of this task.

Greg Shah wrote:

Please provide some details about the status of this task.

Greg,
I hope to finish the task at the weekend. Sorry for a delay.

It seems that we have a problem with nested frames - all the intermediate frames and inner widget have a frame field value equal to the most outer frame. As a result intermediate frames are not visible and even have a false value of the realized attribute. As far as I understand the problem is in FrameWidget.setFrame(FrameWidget frame) method as it uses not the argument for the assignment but its frame. This causes problem with setting INNER-LINES attribute for the COMBO-BOX widget.

We don't have frame family support yet (that is the last part of #1798). It won't get worked until later this summer. Do you need this now?

Greg Shah wrote:

We don't have frame family support yet (that is the last part of #1798). It won't get worked until later this summer. Do you need this now?

I need it for correct INNER-LINES support in case of nested frame.

I think that I understand the root of the problem. The FrameWidget.frame field initially contains the corresponding instance of the GenericFrame but setFrame() method overrides it with the GenericFrame of the enclosing frame. It looks like we need a separate field in FrameWidget to hold its own GenericFrame.

You can work with Constantin to plan the changes needed. At this point, just put in the minimum support that you need to get this task done. If it turns into something that requires pretty complete frame family support, then we will defer that work.

Greg Shah wrote:

You can work with Constantin to plan the changes needed. At this point, just put in the minimum support that you need to get this task done. If it turns into something that requires pretty complete frame family support, then we will defer that work.

OK. I have some ideas how to deal with the issue. If it will not work in an hour I will just add a comment in a place where implementation should be adjusted.

It seems that there is a problem with config changes' propagation from client side to the server one. I change 3 fields at the client side (all are changed in the same method). At the server side I see two config updates. The first update contains an updated value for the one field, the second one has old values for all 3 fields (before the client-side changes). Do I understand correctly that the changes should be propagated automatically (via AOP) or some addition actions are required?

Igor, which fields are not propagated? Please post a sample code.

Hynek,
I've added 3 fields to the ComboBoxConfig :

/** The number of assignments to INNER-LINES since the last activation */
   public int innerLinesAssignments = 0;

/** The max value of INNER-LINES */
   public int innerLinesLimit = 0;

/** The adjustement of  innerLinesLimit after the widget activation*/
   public int innerLinesLimitDelta = 0;

The client-side code is at the beginning of the ComboBox.activate() method:

if (config.innerLinesAssignments != 0)
{
         config.innerLinesAssignments = 0;
      }
      if (config.innerLinesLimitDelta != 0)
{
         config.innerLinesLimit = Math.max(config.innerLinesLimit - config.innerLinesLimitDelta,
               config.innerLines);
         config.innerLinesLimitDelta = 0;
      }

The first config update contains new value for the innerLinesLimit, the second one - old values for all 3 fields.

Additional observations: addDirtyConfig was invoked for all 3 fields.

Are all the fields properly handled in readExternal, writeExternal and applyConfig? Please post your code changes.

Well, I've noticed some strange thing - the backup values for 2 field are stale (do not reflect the values before changing) - looks like ABA issue.

Please post your whole update and a small sample I can run. Thanks.

Hynek Cihlar wrote:

Please post your whole update and a small sample I can run. Thanks.

My code is currently in a bad shape and the test scenario is pretty complicated. However I've commented out the comparison of 'active' and 'backup' values in the getConfigUpdates (I understand that it is an optimization) and now everything works OK.
I plan to finish with this task soon and will prepare the description of the testing scenario.

The additional note: the second config update I've mentioned before is actually a one triggered by a server-side SyncConfigChangesAspect and is an update of another instance of the ComboBoxConfig.

Need to add frame validation at the moment of realization to ensure that all widgets fit in the frame. At this moment the visible height of the SELECTION-LIST is adjusted but the INNER-LINES attribute value remains intact. In 4GL the realization is not performed with error message. This is OK for valid programs but demonstrates incompatible behavior in case of a program logic error.

Igor Skornyakov wrote:

Need to add frame validation at the moment of realization to ensure that all widgets fit in the frame. At this moment the visible height of the SELECTION-LIST is adjusted but the INNER-LINES attribute value remains intact. In 4GL the realization is not performed with error message. This is OK for valid programs but demonstrates incompatible behavior in case of a program logic error.

Is this something which prevents the 4GL program to run? Basically, our goal is to convert and run VALID 4GL programs - we don't want to duplicate 4GL programming errors.

Constantin Asofiei wrote:

Is this something which prevents the 4GL program to run? Basically, our goal is to convert and run VALID 4GL programs - we don't want to duplicate 4GL programming errors.

Well, formally speaking it doesn't prevent the program to run. However it seems that in theory it is possible to imagine a situation when this will result in an incompatible behavior:
one can enable frame, check if the widget is visible and adjust the value of the INNER-LINES if it is not and enable it again or (even worth) start doing something else). I understand that this is a pretty strange scenario, but it is possible in theory. After all in many places we simulate a very weird 4GL behavior including error messages.

Igor Skornyakov wrote:

I've noticed the following issue (see attached sample code). With native 4GL one can see a bordered frame w/o scrollbars. With converted code the is a frame with both horizontal and vertical scrollbars.

Probably due to the following in DropDownGuiImpl.setHeightAndPos()?

// init scrollbars, always visible?
scrollPaneGui.setShowBars(ScrollPaneGuiImpl.ShowBars.ALWAYS);

Hynek Cihlar wrote:

Probably due to the following in DropDownGuiImpl.setHeightAndPos()?

[...]

Thank you Hynek. It looks like that. I haven't investigated it myself yet as formally it is not in a scope of the task I'm working on now.

BTW: I would like to repeat the question I've asked earlier. What is the exact semantics of the widget 'realization'? I failed to find an answer in the ABL documentaion. Looking at our code I can see that it is more or less equivalent to 'made visible at least once'. However there are some corner cases in 4GL when the widget becomes 'realized' after an access to some of its attributes and (in my tests) it doesn't become visible in such cases. As I can see we do not simulate any of such cases.

Igor Skornyakov wrote:

BTW: I would like to repeat the question I've asked earlier. What is the exact semantics of the widget 'realization'? I failed to find an answer in the ABL documentaion.

From 4GL 9.1 Progress Programming Handbook, chapter 16-26, page 614:

When you create or define a widget, Progress creates an internal data structure associated with that widget. Before the widget can be displayed on the screen, the window system must also create a data structure for the widget. When this second data structure exists, the widget is realized.
...
In general, a widget becomes realized when the application needs to make it visible on the screen.

Eugenie Lyzenko wrote:

From 4GL 9.1 Progress Programming Handbook, chapter 16-26, page 614:

[...]

Thank you Eugenie. Unfortunately for me it is not a description of the semantics - obviously some internal data structure is created when the widget is initially created. Regarding the clause that this internal structure is created when the widget is about to become visisble - this is what my question was about as sometimes the widget becomes realized after just a read access to some of its attributes. In other words: what should be considered as a counterpart of this secondary structure in the converted application? Is it sufficient to set a 'realized' flag in the widget config?

My suggest: the widget become realized when it become the part of some frame. More specifically: when it is used in widget placement procedure(what we do in LayoutManager.doLayout()) - when it's(and other widgets in a frame) position and size is calculated. This usually happening just before it become visible(at least for the first time).

Igor Skornyakov wrote:

Is it sufficient to set a 'realized' flag in the widget config?

There already is a WidgetConfig.realized flag...

Eugenie Lyzenko wrote:

My suggest: the widget become realized when it become the part of some frame. More specifically: when it is used in widget placement procedure(what we do in LayoutManager.doLayout()) - when it's(and other widgets in a frame) position and size is calculated. This usually happening just before it become visible(at least for the first time).

Unfortunately, I don't think this is not the case. In P2J, layout is performed as soon as the frame is defined - when the frame's first pushScreenDefinition is called, layout will be performed.

Later on, when the frame gets viewed, the WidgetConfig.realized flag gets set, too.

Igor: please post your example for the case you are trying to solve.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor: please post your example for the case you are trying to solve.

Constantin. In one of my tests I've discovered that an assignment to the frame BOX attribute was rejected with a "message widget is realized" after the read access to the HEIGTH-CHARS attribute of the widget belonging to this frame. In the ABL documentation I've found several places where it was mentioned that an access to some attributes make widget realized (but nothing like this was said about HEIGHT-CHARS). I started to investigate what 'realized' actually means in 4GL and P2J and couldn't figure it. See a sample code attached.

OK, this is interesting; I think reading width/height attributes will make the frame "realized" and force it to compute its layout. A question is: what happens when these are accessed for an unrealized widget? Also, does this block your current work?

Constantin Asofiei wrote:

OK, this is interesting; I think reading width/height attributes will make the frame "realized" and force it to compute its layout.

"To compute a layout" is a good semantics. As I understand now there is no direct counterpart for this in P2J.

A question is: what happens when these are accessed for an unrealized widget?

Sorry I do not understand your question.

Also, does this block your current work?

Formally it doesn't but I'm trying to understand the underlying 4GL logic as pure "black-box" simulation seems unreliable.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

OK, this is interesting; I think reading width/height attributes will make the frame "realized" and force it to compute its layout.

"To compute a layout" is a good semantics. As I understand now there is no direct counterpart for this in P2J.

A question is: what happens when these are accessed for an unrealized widget?

Sorry I do not understand your question.

I mean, does the frame get realized regardless which widget gets its size read?

Constantin Asofiei wrote:

I mean, does the frame get realized regardless which widget gets its size read?

I don't know. For some widgets/attributes the realization-on-access is documented. It is possible of course to test (in the scope of a separate task) it but it will take some time as in this case is makes sense to test not a single attribute for a widget. The lesson I've learned so far is that the realization can be a non-trivial operation and it is worth to check the behavior both before and after realization (and how the widget state affects the realization).

Hynek Cihlar wrote:

Igor Skornyakov wrote:

I've noticed the following issue (see attached sample code). With native 4GL one can see a bordered frame w/o scrollbars. With converted code the is a frame with both horizontal and vertical scrollbars.

Probably due to the following in DropDownGuiImpl.setHeightAndPos()?

[...]

I believe that Eugenie has resolved this in his latest work in #2546 (the update is not yet posted).

Eugenie: can you please confirm this?

Greg Shah wrote:

Probably due to the following in DropDownGuiImpl.setHeightAndPos()?

[...]

I believe that Eugenie has resolved this in his latest work in #2546 (the update is not yet posted).

Eugenie: can you please confirm this?

1. Yes, this call was removed in my recent update.
2. This call worked only for drop-down list and only for the time it exists. No other classes that use ScrollPane are touched. So this line is not a reason I think.

Eugenie Lyzenko wrote:

2. This call worked only for drop-down list and only for the time it exists. No other classes that use ScrollPane are touched. So this line is not a reason I think.

Just for the record, when ShowBars.ALWAYS is replaced with ShowBars.NEVER, vertical scroll bar in drop down of combo box widget is not displayed.

I've created a separate task #2572 for work on realized.

Is another task is needed for the issue in note 67, 71 - 72 and 84 - 86?

Greg Shah wrote:

Is another task is needed for the issue in note 67, 71 - 72 and 84 - 86?

I do not consider this issue as the part of the current task. I think it makes sense to create a separate task.

Igor Skornyakov wrote:

Greg Shah wrote:

Is another task is needed for the issue in note 67, 71 - 72 and 84 - 86?

I do not consider this issue as the part of the current task. I think it makes sense to create a separate task.

I'm not clear about:

1. Is there only 1 issue described in those notes or more than one?
2. Is Eugenie's change to not force ShowBars.ALWAYS a solution to part or all of the issue?
3. I don't understand the point about ShowBars.NEVER.

Greg Shah wrote:

1. Is there only 1 issue described in those notes or more than one?

I understand that it is a single issue.

2. Is Eugenie's change to not force ShowBars.ALWAYS a solution to part or all of the issue?

Sorry - I haven't checked it yet.

Greg Shah wrote:

3. I don't understand the point about ShowBars.NEVER.

There seemed to be a confusion about the code snippet from note 71 being executed or not as part of combo box dropdown rendering. Changing the ShowBars enumeration was just a demonstration.

The frame.enableUnlessHidden() call is generated for enable unless-hidden all statement/Generic but CommonFrame/GenericFrame doesn't have such method. Fixing.

1. SCREEN-VALUE attribute getter returns empty string (at least for the FILL-IN widget).
2. SELECTION-TEXT attribute for the EDITOR widget doesn't work (my fault)
3. EDITOR appearance after initialization is wrong (regression)

The FillIn.appVar initial value is not 'unknown'. As a result it is impossible to correctly calculate screenValue() when no data have been entered. For example for the DEFINE VARIABLE fi AS INTEGER FORMAT "$>,>>>ABC" NO-UNDO VIEW-AS FILL-IN. FillIn.getText() returns "$ABC" instead of "".

Igor Skornyakov wrote:

The FillIn.appVar initial value is not 'unknown'. As a result it is impossible to correctly calculate screenValue() when no data have been entered. For example for the DEFINE VARIABLE fi AS INTEGER FORMAT "$>,>>>ABC" NO-UNDO VIEW-AS FILL-IN. FillIn.getText() returns "$ABC" instead of "".

Do you have a more complete testcase which shows your problem?

In P2J, the widget's "uninitialized" state is tracked via the WidgetConfig.state variable. More, SCREEN-VALUE always shows what is actually on the screen; so if the widget is not yet realized, it may be empty string, and after widget realization, it will be "$ABC", for your case.

To provide the correct implementaion of the REPLACE method for list widgets (COMBO-BOX/SELECTION-LIST/RADIO-SET) a new ReplaceInterface was introduced. This requires a substantial refactoring of the EditorWidget class including implementaion of new methods which are not alaways just error reporting. In particular it appears that EDITOR supports undocumented REPLACE method with two arguments which is a documented REPLACE with the default value of the third argument. Some previously implemented methods of list widgets (e.g. RadioSetWidget.delete(int)) should be changed as well to provide a correct support for dynamic widgets - the error message should be different from the one currently implemented.

This should be a standard approach for all polymorphics methods with different signatures for different widgets. I haven't realized this before and used UnimplementedFeature.unsupported() while 4GL reports either invalid argument type or incorrect number of arguments (for dynamic widgets).

I've just realized the following. If we assign a value to an attribute via widget handle then 'unknown' literal ? is accepted both by 4GL and p2j (at least in some cases, e.g. for the AUTO-ZAP attribute). As we discussed before (in the case of ADD-LAST/ADD-FIRST method) 4GL makes difference between the 4GL literal and the variable with unknown value (e.g. assigned from the field with the ? value) while p2j compiler generates typed variable with the unknown value, not 'unknown' literal.
For example if h is a handle of the FILL-IN widget and az is a LOGICAL variable then the assignment

az = ?.
h:AUTO-ZAP = az.

will result in an error message "**Unable to assign UNKNOWN value to attribute AUTO-ZAP on FILL-IN fi. (4083)" while the compiled p2j code looks like the following:

logical az = new logical(false);
az.setUnknown();
h.unwrapAutoZapElement().setAutoZap(azv);

The code of the setAutoZap() method cannot generate the error message as 4GL doesn't complain about the logical variable with the 'unknown' value if there was no direct assignment of ? literal to this variable.

At this moment the only way to fix it I can see is to make all descendants of the BaseDataType to be childs of the 'unknown' class and always(?) generate new unknown() for the ? literal.

Sorry - this idea is incorrect.

Another solution to the above mentioned issue is to use static immutable constant BaseDataType.unknown (of type BaseDataType) instead of the unknown class. After all ? is a literal.

Sorry - this is a stupid idea.

At this moment the only way to fix it I can see is to make all descendants of the BaseDataType to be childs of the 'unknown' class and always(?) generate new unknown() for the ? literal.

This is too big of a change to our BDT inheritance hierarchy. I don't want to take this approach just to fix this obscure bug.

Another solution to the above mentioned issue is to use static immutable constant BaseDataType.unknown (of type BaseDataType) instead of the unknown class. After all ? is a literal.

For future reference, the reason we don't always use the unknown (which is the original replacement for the literal) is because the converted 4GL for parameter passing (in internal procedures, external procedures and functions) currently requires a specific BDT sub-type (e.g. integer) to be passed. Since unknown is not an integer and cannot be cast as such, we often have to take the approach to "wrap" the literal as a specific type.

For now, just put a very clear TODO into the code at that point. The bug is obscure and hopefully we won't have to fix it for some time.

Greg Shah wrote:

For now, just put a very clear TODO into the code at that point. The bug is obscure and hopefully we won't have to fix it for some time.

OK, thank you. In fact I don't even consider this as a bug (at least for the AUTO-ZAP attribute as in this case the assignment of the non-literal 'unknown' value is just ignored). I've mentioned it just for memory as something of this kind can be seen in other circumstances.

Let fi be a FILL-IN widget. According to my tests the AUTO-ZAP and DISABLE-AUTO-ZAP attributes' behavior is as described below:

A.DISABLE-AUTO-ZAP = no (default)

AUTO-ZAP values:

A.1. Initial value: yes
A.2. After first entered char/ cursor move: no
A.3. After assigning 'yes' value: yes
A.4. On focus change: ON LEAVE fi: no, ON ENTRY <next focus>: no, after that: yes
A.5. After assigning 'no' value: yes
A.6. On focus gain: yes
A.7. After assigning 'no' value: no

B.DISABLE-AUTO-ZAP = yes

AUTO-ZAP values:

B.1. Initial value: yes
B.2. After first entered char/ cursor move: no
B.3. After assigning 'yes' value: yes
B.4. On focus change: ON LEAVE fi: no, ON ENTRY <next focus>: no, after that: yes
B.5. After assigning 'no' value: yes
B.6. On focus gain: yes
B.7. After assigning 'no' value: no

if DISABLE-AUTO-ZAP = no AND AUTO-ZAP = yes then the first valid character overrides the previous content
if DISABLE-AUTO-ZAP = yes then regardless value of the AUTO-ZAP doesn't affect the data entry, but the format is not applied completely: for example if the format is "$>,>>>ABC" then only ">,>>>" will be used.

The assignment of the 'unknown' value doesn't affect neither AUTO-ZAP nor DISABLE-AUTO-ZAP, however an attempt to assign an 'unknown' literal results in an "**Unable to assign UNKNOWN value to attribute <attr-name> on FILL-IN fi. (4083)" error message

There is nothing specific regarding these attributes for dynamic widgets.

Keyboard.keyCode(event) returns -1 for some valid key labels (such as CTRL-ALT-X). As a result (?) the trigger ON CTRL-ALT-X ANYWHERE is never invoked

Please post the list of which key labels you have found which are wrong. For each one, please also post the key code which Progress returns.

Greg Shah wrote:

Please post the list of which key labels you have found which are wrong. For each one, please also post the key code which Progress returns.

This list will be pretty long. I'm trying now to compose a 4GL program which will generate it.

We have something like that already, see testcases/uast/keyboards/key_code_report.p

We did use that (and some others) in the past to generate our current list. I'm not sure how we missed the ones you have found.

Our previous investigation was on v9.1c on HP-UX. If you are testing on windev01, that would explain some of the differences, since Windows 4GL does seem to report some differences in keys.

Greg Shah wrote:

We have something like that already, see testcases/uast/keyboards/key_code_report.p

We did use that (and some others) in the past to generate our current list. I'm not sure how we missed the ones you have found.

Our previous investigation was on v9.1c on HP-UX. If you are testing on windev01, that would explain some of the differences, since Windows 4GL does seem to report some differences in keys.

I see. Thank you. I'm testing on lindev01.

I think I understand what's up. Some keycodes in 4GL have different labels and keylabel function returns only one of them. For example keycode 1025 corresponds to ALT-CTRL-A/CTRL_ALT-A and ESC-CTRL-A labels. The documentation describes the first one while the keylabel function returns the second.

Igor Skornyakov wrote:

Keyboard.keyCode(event) returns -1 for some valid key labels (such as CTRL-ALT-X). As a result (?) the trigger ON CTRL-ALT-X ANYWHERE is never invoked

I think CTRL-ALT-X is an alternative for ESC-CTRL-X, as this gives the same key-code on lindev01 (both are 1048):

message key-code("esc-ctrl-x") key-code("ctrl-alt-x").

Also, please see the uast/keyboards/linux-chui.txt - this is a report with all the key codes/labels/functions for lindev01.

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Keyboard.keyCode(event) returns -1 for some valid key labels (such as CTRL-ALT-X). As a result (?) the trigger ON CTRL-ALT-X ANYWHERE is never invoked

I think CTRL-ALT-X is an alternative for ESC-CTRL-X, as this gives the same key-code on lindev01 (both are 1048):
[...]

Also, please see the uast/keyboards/linux-chui.txt - this is a report with all the key codes/labels/functions for lindev01.

Thank you Constantin. I understand that. The problem is that 4GL accepts both versions while p2j only the second one.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Keyboard.keyCode(event) returns -1 for some valid key labels (such as CTRL-ALT-X). As a result (?) the trigger ON CTRL-ALT-X ANYWHERE is never invoked

I think CTRL-ALT-X is an alternative for ESC-CTRL-X, as this gives the same key-code on lindev01 (both are 1048):
[...]

Also, please see the uast/keyboards/linux-chui.txt - this is a report with all the key codes/labels/functions for lindev01.

Thank you Constantin. I understand that. The problem is that 4GL accepts both versions while p2j only the second one.

Is this something you need for the current task? If not, please create another task under the User Interface project and relate it to #2423.

Constantin Asofiei wrote:

Is this something you need for the current task? If not, please create another task under the User Interface project and relate it to #2423.

Thank you Constantin. I do not need it now and will create a new task.

Please look at Stanislav's most recent changes in #2422. He is making changes related to AUTO-ZAP in FillIn. Specifically, he is making it transfer to between client and server.

Stanislav has also implemented CURSOR-OFFSET for FillIn in #2422.

Greg Shah wrote:

Stanislav has also implemented CURSOR-OFFSET for FillIn in #2422.

Thank you Greg.

Greg,
As I can see my changes for the FillIn significantly intersect with ones of Stanislav. My first impression is that my approach to the CURSOR-OFFSET is more naive than Stanislav's. Regarding AutoZap I see that Stanislav's approach at least ignores the DISABLE-AUTO-ZAP attribute. From the other side I've not noticed the fact that 4GL returns the AUTO-ZAP for the focused FILL-IN, not for the one referenced.
Do you think that it makes sense for me to continue working on the AUTO-ZAP?

Do you think that it makes sense for me to continue working on the AUTO-ZAP?

Not for FILL-IN. But COMBO-BOX support is not part of Stanislav's work.

Please review Stanislav's work and make a list of any functional differences from the behavior you have found. Post those notes into #2422.

I think we might as well implement DISABLE-AUTO-ZAP at this time too, since your note 102 describes the behavior in detail. Do you want to add that after Stanislav's changes are checked in?

Greg Shah wrote:

Not for FILL-IN. But COMBO-BOX support is not part of Stanislav's work.

For the COMBO-BOX thw AUTO-ZAP, DISABLE-AUTO-ZAP and CURSOR-OFFSET attributes are neither gettable nor settable for ChUI (I guess because in this mode the widget is not directly editable). The corresponding error message generation is already implemented.

Please review Stanislav's work and make a list of any functional differences from the behavior you have found. Post those notes into #2422.

I think we might as well implement DISABLE-AUTO-ZAP at this time too, since your note 102 describes the behavior in detail. Do you want to add that after Stanislav's changes are checked in?

I think that it is more practical to add the support for the DISABLE-AUTO-ZAP after Stanislav's changed will be checked-in. It is also difficuly for me now to check his implementation against my tests as it will require a tricky merge of con-committed changes. This will be a very simple update (almost one-liner).

My suggestion: in a meantime I will roll-back my changes regarding CURSOR-OFFSET and AUTO-ZAP and fix the only remaining issue with the EDITOR regression.

Actually I suggest to retain a DISABLE-AUTO-ZAP support in the config and at the server-side (getters/setters).

Greg,
What do you think about that?
Thank you,
Igor.

OK, the plan:

1. You can keep the DISABLE-AUTO-ZAP support in the config and server-side getters/setters of your current code.
2. Let Stanislav check in his update. That will bring AUTO-ZAP and CURSOR-OFFSET to fill-in. You merge those changes into your update.
3. At that point, you can implement DISABLE-AUTO-ZAP on the client side.
4. On that version, I want you to also test your findings of note 102. Correct anything that is a deviation.

Does that make sense?

Greg Shah wrote:

OK, the plan:

1. You can keep the DISABLE-AUTO-ZAP support in the config and server-side getters/setters of your current code.
2. Let Stanislav check in his update. That will bring AUTO-ZAP and CURSOR-OFFSET to fill-in. You merge those changes into your update.
3. At that point, you can implement DISABLE-AUTO-ZAP on the client side.
4. On that version, I want you to also test your findings of note 102. Correct anything that is a deviation.

Does that make sense?

I think it is a right approach.

Code Review ias_upd20150605a.zip

1. I think we may want to implement a method in GenericWidget for the core validation processing in GenericFrame.validateLayout(). The default version can do nothing and the SelectionListWidget version can implement the logic that is specific to it.

2. I think the same thing should be done with finishSetup(). This should be called for all widgets, but the generic version can do nothing. This will help us remove widget-specific logic from GenericFrame.

3. Should GenericFrame.validateLayout() be called whenever the frame is realized (not just during ENABLE)?

4. For the FILL-IN:SELECTION-TEXT attribute, I would prefer if the result can be returned with server-side processing only. The necessary state is already synchronized via the FillinConfig right? And the complete text for the fillin can be obtained from the screen-buffer. I want to avoid the client round-trip for this attribute (and for reading the other related selection attributes. Setting the attributes will still need a trip down to reflect the change immediately.

5. In EditorWidget, private methods like invalidPosition() should be at the end of the class.

6. I think the 088 history entry for CommonWidget should state that it is removing support for text selection. It currently says it is adding support.

7. The list of ControlSetEntity, FillInWidget interfaces that are implemented should have each one on its own line.

8. The code at line 435 of ControlSetEntity seems unnecessary.

9. Lines 243, 369, 439 of ControlSetEntity are improperly indented.

10. Lines 240, 330 of RadioSetWidget are improperly indented.

11. The protected methods in ControlSetEntity should be placed above all the private methods.

12. GenericFrame.validateLayout() is missing javadoc.

13. Please search on ) { in GenericFrame. Fix the missing newline for the new methods. Example:

public void enableUnlessHiddenExcept(GenericWidget<?> widget ) {

Greg Shah wrote:

Code Review ias_upd20150605a.zip

3. Should GenericFrame.validateLayout() be called whenever the frame is realized (not just during ENABLE)?

Sorry, I do not understand the question. In which other situations the frame is realized?

4. For the FILL-IN:SELECTION-TEXT attribute, I would prefer if the result can be returned with server-side processing only. The necessary state is already synchronized via the FillinConfig right? And the complete text for the fillin can be obtained from the screen-buffer. I want to avoid the client round-trip for this attribute (and for reading the other related selection attributes. Setting the attributes will still need a trip down to reflect the change immediately.

OK. I will change the implementation.

I've fixed the rest of the issues you've mentioned.

3. Should GenericFrame.validateLayout() be called whenever the frame is realized (not just during ENABLE)?

Sorry, I do not understand the question. In which other situations the frame is realized?

I think it is realized the first time it is made visible. ENABLE is just one way to make the frame visible. VIEW is another way. And then there are things that have hidden enable or view inside like DISPLAY, PROMPT-FOR, SET, UPDATE, CHOOSE...

Please test to see if the same "validation" behavior needs to exist in these cases. If so, then we should call this as part of frame realization.

Greg Shah wrote:

I think it is realized the first time it is made visible. ENABLE is just one way to make the frame visible. VIEW is another way. And then there are things that have hidden enable or view inside like DISPLAY, PROMPT-FOR, SET, UPDATE, CHOOSE...

Please test to see if the same "validation" behavior needs to exist in these cases. If so, then we should call this as part of frame realization.

Thank you Greg. I will check this. Sorry for my ignorance.

Besides the items requested from the code review, what else still needs to be done for this task?

Greg Shah wrote:

Besides the items requested from the code review, what else still needs to be done for this task?

I hope that there is nothing else.

Should I fix this bug in the scope of this task?

No. Please create a new task in the UI project and set its target version to M12. Also add me as a watcher.

When you are finished with the current changes, post the update for review. Then move to #2567 while waiting for the merge. The changes for #2422 are in testing, so I hope they will be checked in soon. At that time, you can merge, we'll do the final code review and you will test.

Greg Shah wrote:

No. Please create a new task in the UI project and set its target version to M12. Also add me as a watcher.

Thank you Greg. I've created the new task #2586.
BTW: I've created another task #2582 as per Constantin request but forgot to add watches at the creation and I do not see how to it later. Sorry for the inconvenience.

BTW: I've created another task #2582 as per Constantin request but forgot to add watches at the creation and I do not see how to it later. Sorry for the inconvenience.

You can add watchers later, using the "Watchers (x)" section at the right side-bar of the task page. There is an "Add" link.

To change the target version, click "Update" for the task like you are going to add a history entry. The "Target Version" field is editable. Change it and then click the "Submit" button.

Greg Shah wrote:

You can add watchers later, using the "Watchers (x)" section at the right side-bar of the task page. There is an "Add" link.

To change the target version, click "Update" for the task like you are going to add a history entry. The "Target Version" field is editable. Change it and then click the "Submit" button.

Thank you Greg. I've added you and Constantin as watchers but I do not know what the target version should be.

Please set it to M12.

Greg Shah wrote:

Please set it to M12.

Done.

I have a problem. P2J compiles

DEFINE VARIABLE sl AS CHARACTER NO-UNDO
VIEW-AS SELECTION-LIST
INNER-LINES 20
INNER-CHARS 10
LIST-ITEMS "l0", "l1", "l2", "l3", "l4".
.

def frame fl sl with size 20 by 15 title "frame".

to

public static class SlFlDef
   extends WidgetList
{
      SelectionListWidget sl = new SelectionListWidget();

public void setup(CommonFrame frame)
{
         frame.setDown(1);
         frame.setWidthChars(20);
         frame.setHeightChars(15);
         frame.setTitle("frame");
         sl.setDataType("character");
         sl.setLabel("sl");
         sl.setInnerLines(20);
         sl.setInnerChars(10);
         sl.setItems(new ControlSetItem[]
   {
            new ControlSetItem(new character("l0"), new character("l0")),
            new ControlSetItem(new character("l1"), new character("l1")),
            new ControlSetItem(new character("l2"), new character("l2")),
            new ControlSetItem(new character("l3"), new character("l3")),
            new ControlSetItem(new character("l4"), new character("l4"))
         }, false);
      }

{
         addWidget("sl", "sl", sl);
      }
   }

For the implementation of the sl.setInnerLines(20) is indistinguishable from the regular assignment to the INNER-LINES attribute, however 4GL doesn't complain about the too big argument value in the first case and generates an error message (and truncates the value) in the second one.

Is there any easy way to deal with this problem?
Thank you.

Regarding the previous question. Is it OK to add a separate method setInititialInnerLines() for use in compilation of the VIEW-AS?
Thank you.

Igor Skornyakov wrote:

Regarding the previous question. Is it OK to add a separate method setInititialInnerLines() for use in compilation of the VIEW-AS?
Thank you.

Yes. This is a good idea.

Code Review ias_upd20150612a.zip

Please go ahead with the merge. Stanislav's changes have been checked in. Here are some other minor items to clean up:

1. FillInWidget has duplicated code on line 269.

2. The GenericWidget.finishSetup() javadoc references browse but it should be generic. Please also add a blank line after that method definition.

3. Why is SelectionListWidget importing org.eclipse.core.runtime?

4. Please remove the extra blank line at SelectionListWidget line 483.

5. Please remove the extra blank line at EditorWidget line 1547.

6. It seems like ClientExports, LogicalTerminal and ThinClient should not be in the update. In each one, the only change remaining is your history entry.

7. BrowseWidget, InnerLines and Frame each need a history entry added.

I found useful to add a method LogicalTerminal.debug(int widgetId, message) and a corresponding (noop) method AbstractWidget.debug(). This helped me a lot in understanding the sequence of the client-side events in the context of the server-side activity. Can I leave the corresonding methods or it is better to remove them?
Thank you,
Igor.

Hynek Cihlar wrote:

Igor, I will not give you an answer to your question but I will at least share with you a little bit from my kitchen.

It helps quite a bit to see the serialized calls when you try to understand a piece of functionality, especially when multiple processes and threads are involved. I created a little class that makes tracing a bit more fun. The TraceAspect class can serialize methods (with arguments) based on the predefined patterns into a csv file. Then the csv file can be conveniently filtered in a spreadsheet app. See the attached file.

Thank you Hynek,
What you've mentioned can be very useful indeed. However I had another issue: I had to trace all the changes of the particular field at the client side and see which ones happens before or after some particular client-side methods. I've tryied to used breakpoints but the situation remained unclear, so I decided to implement the debug method. The idea was to provide an ability to generate a single log with printouts for different events. And this approach proved to be useful for me.

Igor Skornyakov wrote:

Thank you Hynek,
What you've mentioned can be very useful indeed. However I had another issue: I had to trace all the changes of the particular field at the client side and see which ones happens before or after some particular client-side methods. I've tryied to used breakpoints but the situation remained unclear, so I decided to implement the debug method. The idea was to provide an ability to generate a single log with printouts for different events. And this approach proved to be useful for me.

If I get your case I thing you could achieve the same with the right tracing filters, filtering the trace entries by argument values and combining the server and client logs (every entry has a timestamp). But indeed, sometimes it is easier to use a debugger or quick debug changes.

Igor Skornyakov wrote:

I found useful to add a method LogicalTerminal.debug(int widgetId, message) and a corresponding (noop) method AbstractWidget.debug(). This helped me a lot in understanding the sequence of the client-side events in the context of the server-side activity. Can I leave the corresonding methods or it is better to remove them?
Thank you,
Igor.

I'm OK with you leaving those so long as the javadoc explains their purpose clearly AND explains how to use them.

Hynek Cihlar wrote:

Igor, I will not give you an answer to your question but I will at least share with you a little bit from my kitchen.

It helps quite a bit to see the serialized calls when you try to understand a piece of functionality, especially when multiple processes and threads are involved. I created a little class that makes tracing a bit more fun. The TraceAspect class can serialize methods (with arguments) based on the predefined patterns into a csv file. Then the csv file can be conveniently filtered in a spreadsheet app. See the attached file.

This is certainly quite interesting. It seems like we could make this a part of the project if we excluded it from compilation by default (like the widget browser).

I can also see that it would need some improvements to make it more generally usable (the code itself notes that this is still WIP). I would potentially like to see the output sent to our log instead of in a CSV file. I also would like to see a different way to configure it besides the file-based approach, but I'm not exactly sure what is the right way.

Anyway, good work on that. Let's think on this a bit more and perhaps we can come up with a way to make this something that can be included.

Code Review ias_upd20150623a.zip

1. FillIn.isZapActive() is missing javadoc and needs a blank line after it.

2. It seems like the AbstractWidget.debug() implementation should probably simply output the message (and some basic state of the widget) to the log file. That debug() method will not be called in production, so it seems safe enough. Since this is a code change, you can wait on that until a future update.

3. In FillInWidget.setAutoZap(logical autoZap), shouldn't the comment be marked as a TODO to make it clear that the functionality is missing? Also, please add an explanation of why it is not currently possible to detect the literal.

Since 1 and 3 are just comment changes, the modified update can be checked in if it passes regression testing.

Greg Shah wrote:

Hynek Cihlar wrote:

Igor, I will not give you an answer to your question but I will at least share with you a little bit from my kitchen.

It helps quite a bit to see the serialized calls when you try to understand a piece of functionality, especially when multiple processes and threads are involved. I created a little class that makes tracing a bit more fun. The TraceAspect class can serialize methods (with arguments) based on the predefined patterns into a csv file. Then the csv file can be conveniently filtered in a spreadsheet app. See the attached file.

I can also see that it would need some improvements to make it more generally usable (the code itself notes that this is still WIP). I would potentially like to see the output sent to our log instead of in a CSV file. I also would like to see a different way to configure it besides the file-based approach, but I'm not exactly sure what is the right way.

The idea was to be able to gather trace data with additional runtime information (for example method argument and return values) during a controlled time-frame (for example between two user actions) and to be able to do some simple analysis on the gathered trace data (filter specific threads, methods, argument values, count method calls, see call sequences from multiple processes, measure times between arbitrary methods, etc.). Also I didn't want to spend too much time on it. Changing the config file name was an easy way how to activate/deactivate or even change the trace filters during the runtime of the target process. The csv format allows to do the post-tracing analysis, it can be loaded into OpenOffice, the lines can be sorted, filtered, etc.

I would like to put some more energy into this when time permits. The Aspect-way of monitoring method calls is slow, maybe it would make more sense to use the java.lang.instrument services. Also additional output formats would be useful as you mentioned as well as better way to control the tracing on/off status. And of course make it very simple to use.

Code Review ias_upd20150623b.zip

I'm good with the changes.

The Aspect-way of monitoring method calls is slow, maybe it would make more sense to use the java.lang.instrument services.

If we can instrument things at runtime, then I would like the solution to have the following attributes:

1. Configured via directory or via an API that can be called dynamically at runtime.

2. Different output formats would be fine, but I'd still like the output to go to the log. If we do this right, it is easy to filter those records from the log into another file (csv or whatever) for further analysis. But I think the most common use case is just to see the output in the log file.

My point: I'd like this approach to be a "dtrace" for P2J. But as you say, the AspectJ approach can't really be used for that purpose.

I would like to put some more energy into this when time permits.

Please create a task and add it to the "Deployment and Management Improvements" milestone. You can reference back to the specific notes in this task for the details. We'll work this task later in the project.

Good stuff!

Greg Shah wrote:

My point: I'd like this approach to be a "dtrace" for P2J. But as you say, the AspectJ approach can't really be used for that purpose.

May be it is worth to consider a BTrace (or Byteman) - based approach? I've used Byteman before for ad-hoc tracing and it was pretty useful.

Igor Skornyakov wrote:

Greg Shah wrote:

My point: I'd like this approach to be a "dtrace" for P2J. But as you say, the AspectJ approach can't really be used for that purpose.

May be it is worth to consider a BTrace (or Byteman) - based approach? I've used Byteman before for ad-hoc tracing and it was pretty useful.

Yes, this seems interesting. We already use ASM and BCEL in the project. These can be considered too.

Please see #2594 - Implement a tracing facility for P2J.

What is the status of regression testing for the 0623b update?

Also: I think you need to merge to the latest P2J revision. This will require another round of testing. I'd like you to check in next if we can get this done soon.

Greg Shah wrote:

What is the status of regression testing for the 0623b update?

Also: I think you need to merge to the latest P2J revision. This will require another round of testing. I'd like you to check in next if we can get this done soon.

I've found and fixed some of my bugs.
At this moment all CTRL-C tests passed but I see a lot of timeouts in the main part (mostly in gc_tests and tc_tests). They don't look to be related for my changes, but I'm trying to figure more.
I'm planning to restart regression with the most recent version right now.

I'm planning to restart regression with the most recent version right now.

Please post the merged update for review.

Do you think I have to rebuild the majic code?

It depends on when you last ran conversion there. To be safe, it is a good idea to go ahead and run the build target.

Code Review ias_upd20150630a.zip

I'm fine with the changes. What is the status of testing for this update?

Greg Shah wrote:

I'm fine with the changes. What is the status of testing for this update?

There are still a lot of failures (timeouts) in tc_tests and especially gso_tests. I'm analyzing logs and so far see nothing which looks related to my changes/ What I do not like is that I see many NPEs inside the ObjectOutputStream.writeUTF() method invoked fro the NativeTypeSerializer.StringSerializer (at the client side). I haven't seen this before. Trying to understand.

There are also many 'Connection ended abnormally' caused by 'Queue stop is requested by application' - in this time all the call chain is in the p2j code.

The situation with NPE in writeUTF() seems to be a known issue with OpenJDK. See https://bugs.openjdk.java.net/browse/JDK-6620047. Moreover a comment states that it is an expected behavior. This is very strange indeed.

Sorry. I've previously found a wrong reference (it was about DataOutputStream). See however this one: https://netbeans.org/bugzilla/show_bug.cgi?id=249455

I don't understand what this bug has to do with our failure. They state that it is specific to JDK 9.

Please post a stack trace so that we can understand better what the failure looks like.

Greg Shah wrote:

I don't understand what this bug has to do with our failure. They state that it is specific to JDK 9.

Please post a stack trace so that we can understand better what the failure looks like.

Indeed, but the stack trace looks the same.

WARNING: {Writer} failure in writing loop
java.lang.NullPointerException
at java.io.ObjectOutputStream$BlockDataOutputStream.getUTFLength(ObjectOutputStream.java:2135)
at java.io.ObjectOutputStream$BlockDataOutputStream.writeUTF(ObjectOutputStream.java:2006)
at java.io.ObjectOutputStream.writeUTF(ObjectOutputStream.java:868)
at com.goldencode.util.NativeTypeSerializer$StringSerializer.writeValueWorker(NativeTypeSerializer.java:652)
at com.goldencode.util.NativeTypeSerializer.writeValue(NativeTypeSerializer.java:113)
at com.goldencode.p2j.ui.WidgetConfigUpdates.writeExternal(WidgetConfigUpdates.java:107)
at com.goldencode.p2j.ui.ClientState.writeExternal(ClientState.java:142)
at java.io.ObjectOutputStream.writeExternalData(ObjectOutputStream.java:1458)
at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1429)
at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1177)
at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:347)
at com.goldencode.p2j.net.SyncMessage.writeExternal(SyncMessage.java:122)
at java.io.ObjectOutputStream.writeExternalData(ObjectOutputStream.java:1458)
at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1429)
at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1177)
at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:347)
at com.goldencode.p2j.net.Protocol$Writer.objToByteArray(Protocol.java:549)
at com.goldencode.p2j.net.Protocol$Writer.run(Protocol.java:469)
at java.lang.Thread.run(Thread.java:744)

Here is the stack trace for another common failure.

com.goldencode.p2j.net.SilentUnwindException: Connection ended abnormally
at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1142)
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:1423)
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: com.goldencode.p2j.net.ApplicationRequestedStop: Queue stop is requested by application
at com.goldencode.p2j.net.Conversation.block(Conversation.java:304)
at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:257)
at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1128)
... 12 more

As I can see from the reports in many failed tests the client exits at the very first screen:

wait = true; millis = '180000'; failing screen =

MAJIC                             Syman Menu                  06/30/2015 16:56 
┌────────────────────── TIMCO - Greensboro GSO TIPR833K ───────────────────────┐
│ I  Inventory                           F  Syman - Master File Maintenance    │
│ O  Order Entry                         C  Codes and Parameters               │
│ V  Service Order Entry                 M  Menu of Outputs and Reports        │
│ P  Purchasing                          U  Utilities                          │
│ G  Shipping                            N  Inquiry Menu                       │
│ D  Data Collection                                                           │
│                                        E  Estimating                         │
│ 1  General Ledger                      J  Job Definition                     │
│ 2  Accounts Receivable                 A  Component Tracking                 │
│ 3  Accounts Payable                    W  Work Center Capacity & Dispatching │
│ 4  Payroll                                                                   │
│ 5  Fixed Assets                        R  Quit MAJIC/Symix                   │
│ 6  Customer Menu                                                             │
│ 7  RFQ Menu                                                                  │
│ 8  Quality Assurance                                                         │
│                                                                              │
└──────────────────────────────[     No mail    ]──────────────────────────────┘

ias@devsrv01:~$

Here is the stack trace for another common failure.

com.goldencode.p2j.net.SilentUnwindException: Connection ended abnormally
at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1142)

This can commonly occur when there is an unexpected problem with the protocol and thus the queue is terminated abnormally. You can search on new ApplicationRequestedStop() to see where these can occur. In other words, this may just be another manifestation of the same problem.

I guess this has something to do with your changes to the configuration processing or widget config data. Perhaps some instrumentation/logging in WidgetConfigUpdates might give us a better idea of what is going on?

Greg Shah wrote:

I guess this has something to do with your changes to the configuration processing or widget config data. Perhaps some instrumentation/logging in WidgetConfigUpdates might give us a better idea of what is going on?

Thank you Greg. I'm looking at this now. However NPE in writeUFT() is weird. I've used it a lot w/o any problem. However I used Sun/Oracle JRE most of the time.

Igor Skornyakov wrote:

Thank you Greg. I'm looking at this now. However NPE in writeUFT() is weird. I've used it a lot w/o any problem. However I used Sun/Oracle JRE most of the time.

Well it seems that I forgot something. ObjectOutputStream.writeUTF(null) generates NPE indeed.I suggest the following workaround in the NativeTypeSerializer

public static void writeValue(Object value, NativeTypeSerializer worker, ObjectOutput out)
   throws IOException
{
      if (value == null)
   {
         out.writeByte(OBJECT_SERIALIZER.mark);
         OBJECT_SERIALIZER.writeValueWorker(value, out);
         return;
      }
      out.writeByte(worker.mark);
      worker.writeValueWorker(value, out);
   }

(the visibility of the mark field becomes 'public').

I'm OK with this.

Constantin/Hynek?

Igor Skornyakov wrote:

(the visibility of the mark field becomes 'public').

Please leave it protected instead of public. Otherwise the change is good.

Igor Skornyakov wrote:

Fixed serialization for the null string. Regression test restarted

When you have abends like this one, is useful to manually test some basic MAJIC login/navigation before starting runtime testing. Is enough to login, press V/C at the main menu and connect to RFQ (the first steps in some of the CTRL-C scenarios). If this works, then is safe to start main part.

Igor Skornyakov wrote:

I suggest the following workaround in the NativeTypeSerializer

public static void writeValue(Object value, NativeTypeSerializer worker, ObjectOutput out)
throws IOException {
if (value == null) {
out.writeByte(OBJECT_SERIALIZER.mark);
OBJECT_SERIALIZER.writeValueWorker(value, out);
return;
}
out.writeByte(worker.mark);
worker.writeValueWorker(value, out);
}

(the visibility of the mark field becomes 'public').

With the workaround, we are losing type information about the serialized field but in this case this is probably fine.

Hynek Cihlar wrote:

With the workaround, we are losing type information about the serialized field but in this case this is probably fine.

Well, I can suggest a cleaner solution which is also more efficient - to add a new type for serializer NULL_STRING_SERIALIZER with dummy write method and read just returning null

Well, the situation is much better now. All CTRL-C tests passed. Only one gso_test (gso_269) and two rc_tests (tc_item_stockroom_011 and tc_job_002) failed. The main part is restarted.

See #2603 for a response on the harness change.

The tc_item_stockroom_011 still fails. The mismatching screen is

07/03/2015                  ITEM STOCKROOM LOCATIONS                    07:00:14
┌──────────────────────────────────────────────────────────────────────────────┐
│Item: 0A0001EUV       Prod Code: PG        U/M: RL          Site: GSO         │
│      R326V-125X1" TAPE RUBATEX 5844010125               On Hand:       56.000│
│      BMS8-283G .125"X1.000"X50'                             MRB:        0.000│
└──────────────────────────────────────────────────────────────────────────────┘
┌──────────────────────────────────────────────────────────────────────────────┐
│  Facility: H2    ABC Code: C   Cycle Type:          Min On Hand:        0.000│
│Cycle Freq: 0    Triggered: No  Last Count: 07/31/09 Max On Hand:        0.000│
└──────────────────────────────────────────────────────────────────────────────┘
┌──────────────────────────────────────────────────────────────────────────────┐
│ >     EXPIRED                        0  0                 0.000  Y           │
│  A    STOCK                          0  0                 0.000  Y           │
│  H1   STOCK                          0  0                 0.000  Y           │
|  H3   STOCK                          0  0                 0.000  Y           │
└──────────────────────────────────────────────────────────────────────────────┘

<Arrows>-scroll  <RETURN>-to select MOVE-TO location  <F4>-Quit

12/17/2009                  ITEM STOCKROOM LOCATIONS                    16:07:21
┌──────────────────────────────────────────────────────────────────────────────┐
│Item: 0A0001EUV       Prod Code: PG        U/M: RL          Site: GSO         │
│      R326V-125X1" TAPE RUBATEX 5844010125               On Hand:       56.000│
│      BMS8-283G .125"X1.000"X50'                             MRB:        0.000│
└──────────────────────────────────────────────────────────────────────────────┘
┌──────────────────────────────────────────────────────────────────────────────┐
│  Facility: H2    ABC Code: C   Cycle Type:          Min On Hand:        0.000│
│Cycle Freq: 0    Triggered: No  Last Count: 07/31/09 Max On Hand:        0.000│
└──────────────────────────────────────────────────────────────────────────────┘
┌─ Fac  Location     Lot             PO  Ln Serial#       On Hand MRB Expires ─┐
│ >     EXPIRED                        0  0                 0.000  Y           │
│  A    STOCK                          0  0                 0.000  Y           │
│  H1   STOCK                          0  0                 0.000  Y           │
|  H3   STOCK                          0  0                 0.000  Y           │
└──────────────────────────────────────────────────────────────────────────────┘

<Arrows>-scroll  <RETURN>-to select MOVE-TO location  <F4>-Quit

Did you check this scenario by hand (manually trying the same inputs)?

Greg Shah wrote:

Did you check this scenario by hand (manually trying the same inputs)?

Actually I tried to find a corresponding 4GL code. It was a bad idea. Now I'll try to reproduce it manually.

Igor Skornyakov wrote:

Greg Shah wrote:

Did you check this scenario by hand (manually trying the same inputs)?

Actually I tried to find a corresponding 4GL code. It was a bad idea. Now I'll try to reproduce it manually.

MAJIC has some built-in "help" triggered by pressing F2 in almost all screens - this will show you which 4GL program is currently running (beside some other options). Otherwise, for this specific case, I think is either something relate to frame's title or PUT SCREEN statement. Search in the MAJIC's 4GL code for something specific (like Ln Serial#) - this would narrow the list of potential program files.

Constantin Asofiei wrote:

MAJIC has some built-in "help" triggered by pressing F2 in almost all screens - this will show you which 4GL program is currently running (beside some other options). Otherwise, for this specific case, I think is either something relate to frame's title or PUT SCREEN statement. Search in the MAJIC's 4GL code for something specific (like Ln Serial#) - this would narrow the list of potential program files.

Thank you Constantin. I will check this.

Sorry, but I need an advise? I'm trying to execute the test manually, and I have to send VK_F1 at some step, but when I press F1 nothing happens with the app and I see Ubuntu help instead.
Can anybody help me?
Thank you,
Igor.

Try CTRL-X. This should also produce GO function.

Igor Skornyakov wrote:

Sorry, but I need an advise? I'm trying to execute the test manually, and I have to send VK_F1 at some step, but when I press F1 nothing happens with the app and I see Ubuntu help instead.
Can anybody help me?
Thank you,
Igor.

The F1 key is probably caught by the terminal app. See in the terminal app options whether you can turn off handling of FX keys. If you don't find this option, use a different terminal app or simply open a text terminal.

Eugenie Lyzenko wrote:

Try CTRL-X. This should also produce GO function.

Thanks a lot! It works!

Well, the problem with the screen was reproduced manually.

The situation is very strange. On my machine the manual testing is OK (the dynamic header is in place).

I recommend that you zip up the p2j/ directory on devsrv01 and copy it to your local system. Make sure to exclude the p2j/.bzr/ to keep the size small. Unzip it locally and compare the files to see if there are any differences. It seems the most likely case would be some p2j difference between the two systems.

Greg Shah wrote:

I recommend that you zip up the p2j/ directory on devsrv01 and copy it to your local system. Make sure to exclude the p2j/.bzr/ to keep the size small. Unzip it locally and compare the files to see if there are any differences. It seems the most likely case would be some p2j difference between the two systems.

Thank you Greg.

Code Review ias_upd20150709a.zip

I'm fine with the changes.

The only thing I need you to change is that you should not have a separate history entry for widget-id modifications. Please merge those history entries before you check in. Since it is just a change in comments, it won't require re-testing. Of course, please do upload the final update here too.

Greg Shah wrote:

I'm fine with the changes.

The only thing I need you to change is that you should not have a separate history entry for widget-id modifications. Please merge those history entries before you check in. Since it is just a change in comments, it won't require re-testing. Of course, please do upload the final update here too.

Thank you Greg, I will do it.
BTW: the problem was with the extra pushScreenDefinition which overrides dynamic header. It seems a little bit fragile. May be it makes sense to fix it in a better way then I did now? Or at least create a separate task for this - just for memory? The problem is that it seems tricky to create a standalone test.

BTW: the problem was with the extra pushScreenDefinition which overrides dynamic header. It seems a little bit fragile. May be it makes sense to fix it in a better way then I did now? Or at least create a separate task for this - just for memory? The problem is that it seems tricky to create a standalone test.

Yes, this is a good idea. Please do create a new task for this. Put enough details in to recreate the problem and to understand your findings.

Then link that new task as a "Related Task" to this one.

After two runs the intersection of the failed tests' sets is tc_job_002 only.

Code Review ias_upd20150709b.zip

I'm good with the changes.

Please commit and distribute this update. Well done!

Igor: is the only feature left for this task the MAX-CHARS GUI support?

Greg Shah wrote:

Igor: is the only feature left for this task the MAX-CHARS GUI support?

Greg. In my understanding this is was not in the scope of this task. I've just checked that this has no meaning for the ChUI. Was I wrong?

No, you are not wrong. I'm just confirming my understanding that the only remaining functionality from this task is MAX-CHARS. Am I correct?

Greg Shah wrote:

No, you are not wrong. I'm just confirming my understanding that the only remaining functionality from this task is MAX-CHARS. Am I correct?

I think so.

Greg Shah wrote:

No, you are not wrong. I'm just confirming my understanding that the only remaining functionality from this task is MAX-CHARS. Am I correct?

For EDITOR, MAX-CHARS was implemented as part of #2563 (conversion + runtime). Conversion + server-side part should be common.

Igor, can you point me to a testcase which covers your H046 change in Editor.java? This no longer works with the #2563 refactoring, so I want to make sure my change is compatible (you can see it in branch 2563b rev 10900)

Constantin Asofiei wrote:

Igor, can you point me to a testcase which covers your H046 change in Editor.java? This no longer works with the #2563 refactoring, so I want to make sure my change is compatible (you can see it in branch 2563b rev 10900)

Constantin. As far as I remember this was my pretty stupid bug: the selected text was not retievable at all. It was a one line fix.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor, can you point me to a testcase which covers your H046 change in Editor.java? This no longer works with the #2563 refactoring, so I want to make sure my change is compatible (you can see it in branch 2563b rev 10900)

Constantin. As far as I remember this was my pretty stupid bug: the selected text was not retievable at all. It was a one line fix.

I meant what testcase did you use to check the SET-SELECTED method?

The bug has been found during testing combo-box widget with setting attribute LIST-ITEMS outside the define ... view-as combo-box ... statement. For example:

...
define variable Combo as character format "x(13)" initial "0 Item number" 
    view-as combo-box inner-lines 9 sort.
...
Combo:list-items =
"0 Item number,1 Item number,2 Item number,3 Item number,4 Item number,5 Item number,6 Item number,7 Item number,8 Item number,9 Item number,A Item number,B Item number,C Item number,D Item number,E Item number,F Item number". 
...

In this test the internal integer number assigned to first item(0 Item number) is 16 instead of correct 1.

The problematic place is ControlSetEntity:

...
   protected ControlSetItem controlSetItem(character label, BaseDataType value) 
   {
      return new ControlSetItem(config.itemId++, 
            label.isUnknown() ? new character(" ") : (character)label.duplicate(), 
            value.duplicate()); 
   }
...

The wrong item number causes the combo-box to not properly select the currently selected item(when drop-down is opening).

Eugenie Lyzenko wrote:

The bug has been found during testing combo-box widget with setting attribute LIST-ITEMS outside the define ... view-as combo-box ... statement. For example:

It looks like my stupid fault. I will take a look tomorrow. Can you please provide a complete 4GL program that reveals the issue?
Thank you.

uast/combo_box/combo_box12_3.p from testcases bzr repo. I guess you will need the GUI combo-box recent code too(branch 2546a) to run this sample.

Eugenie Lyzenko wrote:

uast/combo_box/combo_box12_3.p from testcases bzr repo. I guess you will need the GUI combo-box recent code too(branch 2546a) to run this sample.

Thank you Eugenie

Eugenie.
The test seems running OK with my version of the code and looking at your version of the ComboBoxWidget I do not see how it can fail.

From the other side I've realized now that if one replaces LIST-ITEMS/LISTS-ITEM_PAIRS after selection was made with exactly the same value then the selected position will not be retained. This can be easily fixed with resetting the id generatior at the assignment of these attributes. However the issue remains if the new list is not exactly the same but contains selected item and I do not see a solution at this moment as an extensive testing of 4GL behavior in such a situation is required (I missed this use case when I was working on the task).

Did you mean this case?

Thank you,
Igor.

Hello Eugenie.
I've introduced the internal ids exactly for the correct restoring of the selected item. I've tested it extensively and with my version of the code there is no issue you describe (in ChUI). The id is assigned to an item only once and it shouldn't cause any issues apart from the cases I described before. The combo_box12_3.p doesn't contain such cases as far as I understand. Please double check your changes.