FWD » Core Development » User Interface

• test the FRAME-COL function to ensure it is compatible
• frame options
  • SCROLL (runtime stubs)
  • USE-TEXT (runtime stubs)
  • NO-ATTR-SPACE (partial runtime)
• attributes
  • browse:num-entries
  • editor:auto-indent
  • widget:next-tab-item
  • frame:prev-tab-item
• event support
  • SCROLL-NOTIFY - high level events for browse widget
    • "A mouse action in the scrollbar area of a browse."
    • "This event allows the developer to track physical movement of the focused row in the browse viewport."
    • "This event is supported only for backward compatibility. Use SCROLL-HORIZONTAL and SCROLL-VERTICAL instead."
  • SCROLL-HORIZONTAL and SCROLL-VERTICAL - high level events for browse widget
  • OFF-END - high level event for browse widget, "A keyboard or mouse actionthat tries to move after the last row of a browse."
  • OFF-HOME - high level event for browse widget, "A keyboard or mouse actionthat tries to move before the first row of a browse."
  • PARENT-WINDOW-CLOSE - high level event for window widget, "An event that each descendant window receives when the common ancestor window in that family receives a WINDOW-CLOSE event."
  • DESELECT
    • not sure if this is the same as the DESELECTION event documented as a direct manipulation event, there is no other documented event that can match
    • How it is generated? "For all selected widgets in a frame, click the mouse SELECT button on an unselected widget or in empty space in the frame."
    • Affects "Frame and field-level widgets with SELECTABLE attribute set to TRUE; browses. For a single selected EXTEND button on a widget, click the mouse selected widget."
    • Processing
      • "Internal: Sets the widget's SELECTED attribute to FALSE. This setting takes effect after any trigger for the event executes."
      • "Screen: Removes the highlight from the affected widget or widgets."

Branch 4335a revision 11420 implements the runtime for the BROWSE:NUM-ENTRIES undocumented attribute. The conversion was already supported. The testcase from #4164-3 is checked in as uast/testcases/browse/browse_num_entries.p. That testcase works properly with the new implementation.

Stanislav: Please review.

Stanislav Lomany wrote:

Stanislav: Please review.

The approach in getNumEntries doesn't work if a browse is not fully filled and the number of rows is more that DOWN, see the testcase. I suppose the only way to get the correct number of rows is to query the client side.

Do we already have an API for this? If not, do we store this value on the client?

4335a revision 11428 adds conversion and server-side support for EDITOR:AUTO-INDENT. The client-side runtime still needs to be implemented. I think the code needs to be changed in com.goldencode.p2j.ui.client.Editor (see both placeLF() and splitLine()).

Interestingly, TABs are used for focus change and they seem to be eaten, even if there is no other widget in the tab order (when the editor is the only enabled widget). So it looks like only space chars need to be considered. We should test if there are any other whitespace chars that are honored (either by typing the characters or from the clipboard or reading from file or variable).

The behavior is pretty simple, it works no matter if you are splitting a line or if you are adding a line by hitting enter at the end of a line. It works anywhere there is a preceding line.

We have usage of both NEXT-TAB-ITEM and PREV-TAB-ITEM in a customer application. Frames are used as well as widgets. Our gap analysis marks both attributes as rt_lvl_partial (conversion is fully supported already). The comment (at least for NEXT-TAB-ITEM) is that frames aren't supported. But as far as I can see the support seems to be there for both widgets and frames.

Can someone please provide more detail on the parts that are missing in regard to NEXT-TAB-ITEM and PREV-TAB-ITEM for frames?

Greg Shah wrote:

Can someone please provide more detail on the parts that are missing in regard to NEXT-TAB-ITEM and PREV-TAB-ITEM for frames?

A part which is missing is the GenericFrame support - something like frame f1:next-tab-item will not work, unless all access to these attrs is converted via the frame handle.

Is this just a conversion issue? The methods are implemented in CommonFrame and FrameWidget.

Greg Shah wrote:

Is this just a conversion issue? The methods are implemented in CommonFrame and FrameWidget.

GenericFrame throws an exception for this - the fix is simple, just delegate the call to FrameWidget.

Task branch 4335a was merged to trunk as revision 11345.

Greg Shah wrote:

Stanislav Lomany wrote:

Stanislav: Please review.

The approach in getNumEntries doesn't work if a browse is not fully filled and the number of rows is more that DOWN, see the testcase. I suppose the only way to get the correct number of rows is to query the client side.

Do we already have an API for this? If not, do we store this value on the client?

What is NUM-ENTRIES attribute hold? Is it the number of entries left? When I ran the browse_num_entries2.p in 4GL, 3 was displayed. In FWD, is displayed 11.

What is NUM-ENTRIES attribute hold? Is it the number of entries left?

It is the number of data rows in the view.

Read the comments in browse_num_entries.p and look carefully at the current FWD implementation/comments. Make sure you read #4164-3 through #4164-6 and also #4164-10.

BTW, I've already implemented the NEXT-TAB-ITEM and PREV-TAB-ITEM change, but just haven't checked it in yet.

As Stanislav noted above, FWD returning 11 is a BUG. So you don't need to analyze how 11 relates to anything.

4GL has no such F7 function.

You must be testing the wrong code in the 4GL. The "F7 function" that you mention is just a trigger in the 4GL code. It should be there in both FWD and the 4GL.

Please read the testcases and the previous notes carefully. Stanislav has already provided the details on what needs to be done in #4164-10.

Stanislav Lomany wrote:

Do we already have an API for this? If not, do we store this value on the client?

There's no server API for it, you can create a function to get Browse.getCacheSize() from the client side.

Is there already some data sharing from client to server going on, so I can see an example? Is LogicalTerminal's refreshCurrentRow implementation of ClientExport's refreshCurrentRow doing the same type of servicing?

Yes, except you will only have the browseId parameter and you must return an int.

Greg Shah wrote:

Yes, except you will only have the browseId parameter and you must return an int.

What's the relationship between the PageRowCount (number of rows in the browse viewport set either by DOWN or SIZE options) and the CacheSize (number of entries visible in the browse)? They sound very similar.

Consider you have a browse:

data row 1
data row 2
<empty row>
<empty row>
<empty row>

DOWN is 5, cache size is 2.

Stanislav Lomany wrote:

Consider you have a browse:

[...]

DOWN is 5, cache size is 2.

Thanks.

• ClientExports: public int getBrowseCacheSize(int browseId);
• BrowseWidget: private int getBrowseCacheSize() Currently I have this storing LogicalTerminal.getBrowseCacheSize(this) in private Integer cacheSize
• LogicalTerminal: public static int getBrowseCacheSize(BrowseWidget bw) which returns locate().client.getBrowseCacheSize(bw.getId());

• ThinClient: public int getBrowseCacheSize(int browseId) where this is returning eventDrawingBracket(browse, () -> res[0] = browse.getCacheSize());

Does that look accurate?
I see that on the client, the value returned by Browse.getCacheSize is dcData.length. In order to make this available to the server,

• ClientExports: public int getBrowseCacheSize(int browseId);
• BrowseWidget: private int getBrowseCacheSize()
• LogicalTerminal: public static int getBrowseCacheSize(BrowseWidget bw)

I would prefer to name functions in other way, e.g. getBrowseRowsNum, because the "cache" term is internal to client-side browse part.

eventDrawingBracket(browse, () -> res[0] = browse.getCacheSize());

You don't need to start an eventDrawingBracket because no drawing happens here.

I don't see any value in BrowseWidget.getBrowseCacheSize() when getNumEntries() can call LogicalTerminal.getBrowseCacheSize() directly.

Updated per suggestions. Thanks!

Committing to: /home/rfb/secure/code/p2j_repo/p2j/active/4231b/                                                                                                                                
modified src/com/goldencode/p2j/ui/chui/ThinClient.java
modified src/com/goldencode/p2j/ui/ClientExports.java
modified src/com/goldencode/p2j/ui/BrowseWidget.java
modified src/com/goldencode/p2j/ui/LogicalTerminal.java
Committed revision 11377. 

Review rev 11377: Roger, you forgot to remove the array need for eventDrawingBracket. You can just write return browse != null ? browse.getCacheSize() : 0; Otherwise the update is good.

Stanislav Lomany wrote:

Review rev 11377: Roger, you forgot to remove the array need for eventDrawingBracket. You can just write return browse != null ? browse.getCacheSize() : 0; Otherwise the update is good.

Thanks! I hadn't worked with eventDrawingBracket so I was being conservative.

Please read the 4GL docs to see what the FRAME-COL function is meant to do. It is the counterpart of FRAME-ROW and both are used to see the location in (characters) of the frame's upper left corner. So in order to test this, you probably need to use it on frames that are displayed at different locations on the screen, right? Also, what happens when the frame is not displayed at all?

Branch 4231b revision 11395 includes GenericFrame changes that complete next-tab-item and prev-tab-item per #4164-12. Those items are now done (and the gap marking was also updated accordingly).

I've updated #4164-1 to highlight the remaining work.

All you care about is positioning the frame in different locations. This is trivial to do in the AT phrase of the frame phrase.

WITH frame f AT { COLUMN column ROW row | X x Y y }

I had a testcase:

def var i as int init 0 format 99.

def frame f0   i with width 10 title "F0 Title". 
def frame f1   i with width 20 title "F1 Title". 
def frame f2   i with width 30 title "F2 Title".
def frame f3   i with width 40 title "F3 Title".
def frame f4   i with width 50 title "F4 Title".
def frame f5   i with width 40 title "F5 Title".
def frame df0  i with view-as dialog-box width 50 title "Default Dialog Title". 

message "display-type=" session:display-type.

{frame_layout\frame_loc_attributes_helper.i &fr="f0"} 
{frame_layout\frame_loc_attributes_helper.i &fr="f1"} 
{frame_layout\frame_loc_attributes_helper.i &fr="f2"}
{frame_layout\frame_loc_attributes_helper.i &fr="f3"}
{frame_layout\frame_loc_attributes_helper.i &fr="f4"}
{frame_layout\frame_loc_attributes_helper.i &fr="f5"}
{frame_layout\frame_loc_attributes_helper.i &fr="df0"}

view frame {&fr}.
message 
      "{&fr}: " 
      "x="  i:x         /* pix loc of wid left edge rel to left parent edge */
      "y="  i:y         /* pix loc of wid top edge rel to top parent edge */ 
      "c="  i:column    /* col pos of wid left edge rel to left (same as x) */
      "r="  i:row       /* row pos of wid top edge rel to top (same as y ) */
      "fx=" i:frame-x   /* pix loc of wid left edge rel to ul of frame */
      "fy=" i:frame-y   /* pix loc of wid top edge rel to ul of frame */ 
      "fc=" i:frame-col /* Dec col pos of wid left edge rel to ul of wid frame */
      "fr=" i:frame-row /* Dec col pos of wid top left edge rel to ul of wid frame */
      .
wait-for go of frame {&fr}.

I should just be able to update the frame phrase with AT column x row y like:

def frame f0   i with width 10 title "F0 Title" at column 0 row 0. 
def frame f1   i with width 10 title "F0 Title" at column 1 row 10. 
def frame f2   i with width 10 title "F0 Title" at column 2 row 20. 
...

That is not really correct, because the attribute is off the widget, in this case i.

Please read #4164-1 more carefully. FRAME-COL(frame) is a built-in function NOT a widget attribute. Your test is written wrong.

In regard to the error 4102, pick frame positions that are inside the same Window.

Greg Shah wrote:

That is not really correct, because the attribute is off the widget, in this case i.

Please read #4164-1 more carefully. FRAME-COL(frame) is a built-in function NOT a widget attribute. Your test is written wrong.

In regard to the error 4102, pick frame positions that are inside the same Window.

My bad... I will have results after conversion of the testcase.

view frame {&fr}.
/* Returns a DECIMAL value that is the column position of the left corner of a frame within its window. */
message "{&fr}=" frame-col({&fr}).
wait-for go of frame {&fr}.

I think you should check the FRAME-ROW(frame) builtin for all of the same case.

Make sure to test:

• What do these return before the frame is realized (made visible)? Is it different from post-realization?
• Can you set negative values or values outside the window for the frame, when the frame is not realized? If so what is returned for those cases?
• What happens if you use the same values for two frames that share the same window? I suspect the pre-realized/post-realized values might be different. For example, the 4GL may move the frame automatically.
• what happens when you display a frame that has no explicit positioning, but there are already other frames displayed? I think the location will be calculated automatically and will be different pre/post realization.

Make a list of the problems here and fix them.

Greg Shah wrote:

Make a list of the problems here and fix them.
1. What do these return before the frame is realized (made visible)? Is it different from post-realization?

4GL pre-realization values are always 0. FWD are always 1.

New testcase has expanded my ability to validate values, but I'm not sure it's on the mark yet. I was expecting the frame to move around, but it stays at (1,1).

def var i as int init 0 format 99.
def var b4col as int.
def var b4row as int.

def frame f0   i with title "F0 Title" at column 1 row 1. 

/*message "display-type=" session:display-type.*/
os-delete "x.txt".
{frame_layout\frame_loc_attributes_helper.i &out="x.txt" &fr="f0" &colval=1 &rowval=1} 
{frame_layout\frame_loc_attributes_helper.i &out="x.txt" &fr="f0" &colval=10 &rowval=2} 
{frame_layout\frame_loc_attributes_helper.i &out="x.txt" &fr="f0" &colval=2 &rowval=3} 
{frame_layout\frame_loc_attributes_helper.i &out="x.txt" &fr="f0" &colval=10 &rowval=4} 
{frame_layout\frame_loc_attributes_helper.i &out="x.txt" &fr="f0" &colval=1 &rowval=5} 
{frame_layout\frame_loc_attributes_helper.i &out="x.txt" &fr="f0" &colval=10 &rowval=4} 
{frame_layout\frame_loc_attributes_helper.i &out="x.txt" &fr="f0" &colval=2 &rowval=3} 
{frame_layout\frame_loc_attributes_helper.i &out="x.txt" &fr="f0" &colval=10 &rowval=2} 
{frame_layout\frame_loc_attributes_helper.i &out="x.txt" &fr="f0" &colval=1 &rowval=1} 

/* Returns a DECIMAL value that is the column position of the left corner of a frame within its window. */
assign 
  b4col = frame-col({&fr})
  b4row = frame-row({&fr})
 .
display i with frame {&fr} at col {&colval} row {&rowval}.
output to {&out} append.
message 
   "{&fr}: b4col/b4row="b4col "/" b4row 
   "aftcol/aftrow="frame-col({&fr}) "/" frame-row({&fr}) 
   "col/row="{&colval} "/" {&rowval}
   " i:" 
   "x/y=" i:x "/" i:y                   /* pix loc of wid left/top edge rel to left parent edge */
   "c/r=" i:column "/" i:row            /* col/row pos of wid left/top edge rel to left/top (same as x/y) */
   "fc/fr=" i:frame-col "/" i:frame-row /* Dec col pos of wid left/top edge rel to ul of wid frame */
   "fx/fy=" i:frame-x "/" i:frame-y     /* pix loc of wid left/top edge rel to ul of frame */
   .
output close.   
wait-for go of frame {&fr}.

Roger, the AT COL ROW in the frame phrase (for DISPLAY) sets the frame's definition location, not at runtime.

To move it at runtime, use the frame's col/row attributes.

As Constantin notes, you are telling the code to only have the frame positioned in col 1 row 1 because only the FIRST use of that is honored. Since you only ever have one frame, it is always set to the same value.

Instead of changing the attributes, you can just remove the frame definition and then use DIFFERENT frame names in each usage of the inlcude file.

Please confirm editor:auto-indent already converts and just needs runtime support only (so I can bypass)?

Roger Borrello wrote:

Please confirm editor:auto-indent already converts and just needs runtime support only (so I can bypass)?

From #4164-7:

4335a revision 11428 adds conversion and server-side support for EDITOR:AUTO-INDENT. The client-side runtime still needs to be implemented. I think the code needs to be changed in com.goldencode.p2j.ui.client.Editor (see both placeLF() and splitLine()).

Greg Shah wrote:

4335a revision 11428 adds conversion and server-side support for EDITOR:AUTO-INDENT. The client-side runtime still needs to be implemented. I think the code needs to be changed in com.goldencode.p2j.ui.client.Editor (see both placeLF() and splitLine()).

Interestingly, TABs are used for focus change and they seem to be eaten, even if there is no other widget in the tab order (when the editor is the only enabled widget). So it looks like only space chars need to be considered. We should test if there are any other whitespace chars that are honored (either by typing the characters or from the clipboard or reading from file or variable).

The behavior is pretty simple, it works no matter if you are splitting a line or if you are adding a line by hitting enter at the end of a line. It works anywhere there is a preceding line.

Assuming uast/editor_auto_indent.p the correct testcase for EDITOR:AUTO-INDENT...

With respect to FRAME-COL (and FRAME-ROW), I am debugging why these are incorrect when checked using this code:

def frame f0   i with width 10 title "Fr f0" at col 1 row 1. 
def frame f1   i with width 20 title "Fr f1" at col 1 row 10. 

display i with frame f0.
message "aftcol/aftrow="frame-col(f0) "/" frame-row(f0).

display i with frame f1.
message "aftcol/aftrow="frame-col(f1) "/" frame-row(f1).

This displays:

aftcol/aftrow= 1 / 1
aftcol/aftrow= 0 / 0

aftcol/aftrow= 1 / 1
aftcol/aftrow= 1 / 10

These values are retrieved from ThinClient methods getFrameCol

   public double getFrameCol(int frameId)
   {
      Frame frame = getFrame(frameId);
      double result = -1;

      if (frame != null && frame.isVisible() && !frame.isRedirected())
      {
         result = ConfigHelper.getAssignedColumn(frame.config());
      }

      if (result == BaseConfig.INV_COORD)
      {
         result = 0;
      }

      return ++result;
   }

   public double getFrameRow(int frameId)
   {
      Frame frame = getFrame(frameId);
      double result = 0;

      if (frame != null && frame.isVisible())
      {
         result = frame.location().y + 1;
      }

      return result;
   }

1. For frame f0, isVisible is valid after the DISPLAY is processed (as you'd expect). But for frame f1, it doesn't not become true after the DISPLAY.
2. Why is FRAME-COL retrieved from the configuration, but FRAME-ROW retrieved from the actual coordinates?
3. Why does getFrameCol include !frame.isRedirected() and getFrameRow doesn't?

Since they return DECIMAL, I believe they should be the actual coordinates, rather than what is configured. Documentation says, "Returns a DECIMAL value that is the column (row position) of the left (upper-left) corner of a frame within its window."

During debug, I found that when the visible flag is set prior to the frame being displayed, the getFrame(frameId) returns FrameGuiImpl. After the frame is displayed, it returns FrameChuiImpl.

During the f0 frame handling, the visible flag is found to be true in the FrameChuiImpl, so it's values for FRAME-COL and FRAME-ROW are returned correctly. But for all the other frames, it is left as false.

I am not familiar enough with why FrameGuiImpl is the frame class before the display, and FrameChuiImpl after, to know if that's working correctly. I do see when the frame is displayed, the visible in FrameGuiImpl is what is set.

Roger Borrello wrote:

During debug, I found that when the visible flag is set prior to the frame being displayed, the getFrame(frameId) returns FrameGuiImpl. After the frame is displayed, it returns FrameChuiImpl.

During the f0 frame handling, the visible flag is found to be true in the FrameChuiImpl, so it's values for FRAME-COL and FRAME-ROW are returned correctly. But for all the other frames, it is left as false.

I am not familiar enough with why FrameGuiImpl is the frame class before the display, and FrameChuiImpl after, to know if that's working correctly. I do see when the frame is displayed, the visible in FrameGuiImpl is what is set.

Area you using streams in your tests, or the redirected terminal ( unnamed stream)? In GUI we have ChUI mode for 'redirected terminal' or stream output, and GUI mode otherwise.

Can you post a breakpoint in the c'tor and see who creates FrameChuiImpl (on client-side)?

Constantin Asofiei wrote:

Area you using streams in your tests, or the redirected terminal ( unnamed stream)? In GUI we have ChUI mode for 'redirected terminal' or stream output, and GUI mode otherwise.

I am using "output to..." to collect the output. I can try without. However, I would think this is a bug.

Can you post a breakpoint in the c'tor and see who creates FrameChuiImpl (on client-side)?

I did, and it shows StreamableWidget.convertToStreamable -> FrameGuiImpl.convertToStreamable -> WidgetFactoryAdapter.ChuiWidgetFactory.create ...

Roger Borrello wrote:

I am using "output to..." to collect the output. I can try without. However, I would think this is a bug.

OK, so we are in a 'normal' path for redirected terminal in GUI. Yes, this looks like a bug, the configuration should be shared (or at least sync'ed back) to the GUI frame.

But, in my experience, a frame used for reports (to redirected terminal/streams) will not be used in GUI, too. So, create a duplicate test for this issue and add a bug in the UI project.

Constantin Asofiei wrote:

Roger Borrello wrote:

I am using "output to..." to collect the output. I can try without. However, I would think this is a bug.

OK, so we are in a 'normal' path for redirected terminal in GUI. Yes, this looks like a bug, the configuration should be shared (or at least sync'ed back) to the GUI frame.

But, in my experience, a frame used for reports (to redirected terminal/streams) will not be used in GUI, too. So, create a duplicate test for this issue and add a bug in the UI project.

This is "normal" in that it's easier for testing, but I would agree, not in "regular" normal.

Testcase:

uast/frame_layout/frame_loc_attributes_get2-redirected.p
uast/frame_layout/frame_loc_attributes_get2.p                                                                                                   
uast/frame_layout/frame_loc_attributes_helper.i

The helper takes a pre-processor directive for a log file. If it's there, it redirects. If it's not, it doesn't redirect.

I also notice on the last frame (a dialog box) the F2 never is recognized in the wait-for go of frame df0. I have to use ESC.

Wasn't someone working in the keystroke area of the client that may have changed something related to this?

When not using redirection, the testcase passes.

f0: b4col/b4row= 0 / 0 aftcol/aftrow= 1 / 1 col/row= 1 / 1  i: x/y= 0 / 0 c/r= 1 / 1 fc/fr= 1 / 2.67 fx/fy= 0 / 35
f1: b4col/b4row= 0 / 0 aftcol/aftrow= 1 / 10 col/row= 1 / 10  i: x/y= 0 / 0 c/r= 1 / 1 fc/fr= 1 / 2.67 fx/fy= 0 / 35
f2: b4col/b4row= 0 / 0 aftcol/aftrow= 10 / 1 col/row= 10 / 1  i: x/y= 0 / 0 c/r= 1 / 1 fc/fr= 1 / 2.67 fx/fy= 0 / 35
f3: b4col/b4row= 0 / 0 aftcol/aftrow= 10 / 10 col/row= 10 / 10  i: x/y= 0 / 0 c/r= 1 / 1 fc/fr= 1 / 2.67 fx/fy= 0 / 35
df0: b4col/b4row= 0 / 0 aftcol/aftrow= 1 / 1 col/row= 1 / 1  i: x/y= 0 / 0 c/r= 1 / 1 fc/fr= 1 / 2.67 fx/fy= 0 / 35

However, I'd still like to understand of getFrameCol and getFrameRow need to be merged to using the coordinates or the configuration.

Hynek: I am trying to understand why the implementations for FRAME-COL and FRAME-ROW are so different, and Greg suggested asking you.

ThinClient.getFrameCol uses ConfigHelper.getAssignedColumn(frame.config()) while ThinClient.getFrameRow uses frame.location().y. The results are accurate either way, but was there any specific reason for the difference?

Roger Borrello wrote:

Hynek: I am trying to understand why the implementations for FRAME-COL and FRAME-ROW are so different, and Greg suggested asking you.

ThinClient.getFrameCol uses ConfigHelper.getAssignedColumn(frame.config()) while ThinClient.getFrameRow uses frame.location().y. The results are accurate either way, but was there any specific reason for the difference?

Both of these methods were at some point changed by myself and by Constantin as part of bigger refactoring chunk. I don't think there is any valid reason for the different implementations. I don't even think we need to do a remote call to get the value of FRAME-ROW and FRAME-COL functions. Both could be determined from the frame's config using ConfigHelper.getEffectiveRow and ConfigHelper.getEffectiveColumn.

Hynek Cihlar wrote:

Both of these methods were at some point changed by myself and by Constantin as part of bigger refactoring chunk. I don't think there is any valid reason for the different implementations. I don't even think we need to do a remote call to get the value of FRAME-ROW and FRAME-COL functions. Both could be determined from the frame's config using ConfigHelper.getEffectiveRow and ConfigHelper.getEffectiveColumn.

I will update ThinClient and run my testcases that verify the attributes, then submit for review.

One other significant difference between the two is that getFrameCol includes a check for !frame.isRedirected() where getFrameRow does not. This condition of whether or not the col/row is retrieved. Which is more correct?

Roger Borrello wrote:

One other significant difference between the two is that getFrameCol includes a check for !frame.isRedirected() where getFrameRow does not. This condition of whether or not the col/row is retrieved. Which is more correct?

Sorry, I can't answer this. But it should be easy to find out by testing in native 4GL.

getEffectivexxx methods are returning 0.0 for clientColumn and clientRow when reading the attributes. The getAssignedxxx methods are returning the actual row/col accurately when I call them directly, because in the getEffectivexxx methods, they are protected by validating the row/col are BaseConfig.INV_COORD before checking.

The testcases are getting frame-col(f0)/frame-row(f0) before/after realization. The frames are defined with at col x row y at the beginning.

It looks like the server-assigned values returned by getAssignedxxx are more accurate than getEffectivexxx (albeit 0-based). Does this warrant more investigation, or should I be OK using the getAssignedxxx methods?

Updated src/com/goldencode/p2j/ui/chui/ThinClient.java in 4231b-11504. Please review.

From Hynek:

Both getAssigned and getEffective return 0-based values. These must be incremented by one (and perhaps also decremented if negative) before the values are returned to legacy code. See BaseEntity.getRow for an example. But please run some test cases in native 4GL to confirm the same behavior for FRAME-ROW and FRAME-COL.

I think you have to use getEffective. Consider the case when the frame position is not assigned explicitly, in this case getAssigned should/may return invalid coordinates.

Thanks,
Hynek

Roger Borrello wrote:

From Hynek:

Both getAssigned and getEffective return 0-based values. These must be incremented by one (and perhaps also decremented if negative) before the values are returned to legacy code. See BaseEntity.getRow for an example. But please run some test cases in native 4GL to confirm the same behavior for FRAME-ROW and FRAME-COL.

I think you have to use getEffective. Consider the case when the frame position is not assigned explicitly, in this case getAssigned should/may return invalid coordinates.

Thanks,
Hynek

Thanks for that info. We do have checks for invalid coordinates, as part of the methods. Please review, as my testcases were successful after I incremented the return value.

Code review 4231b revision 11504. The code changes seem OK. I think you should also test the changes in ChUI if you haven't.

NO-ATTR-SPACE¶

All the documentation indicates ATTR-SPACE and NO-ATTR-SPACE have no effect. That they are supported only for backward compatibility.

Prove it (with testcases). You can only do this with frame that have many widgets. Make sure you test:

• more than 1 widget side by side on the same "row"
• widgets in multiple rows
  • explicitly using SKIP
  • implicitly by having too many widgets for the width of the frame
• ? (other stuff I'm not thinking about now)

One issue is there isn't any documentation indicating the proper usage, or even what ATTR-SPACE and NO-ATTR-SPACE are supposed to do. The 9.1.E ABL reference says for backward compatibility. That's the oldest I've found. Internet searches, the same thing.

There are about 32 procedures in customer code that use either ATTR-SPACE or NO-ATTR-SPACE. I could start with them.

Even though documentation for IS-ATTR-SPACE function indicates it is also supported only for backward compatibility, I'm looking through our code and getting the impression that ATTR-SPACE is like a setter for setting the "space-taking" attribute to yes and NO-ATTR-SPACE is the setter for setting the "space-taking" attribute to no, and IS-ATTR-SPACE is the getter for the "space-taking" attribute.

I've captured the sample code provided into ./uast/frames/is_attr_space.p, only to find we haven't implemented this built-in.

define variable termtype as logical format "spacetaking/non-spacetaking".

termtype = is-attr-space.

display "You are currently using a" termtype no-label "terminal" 
  with frame d1 centered row 5.

The comment in gaps is is-attr-space is not supported but runtime backend is ready(for attribute). I don't think the conversion rule is in place, since conversion of the testcase yields:

    [javac] /home/rfb/projects/VirtualBox-VMs/shared/projects/testcases/src/com/goldencode/testcases/frames/IsAttrSpace.java:31: error: no suitable method found for assign(no arguments)
    [javac]          termtype.assign()

• ui.client.Frame has isAttrSpace and setAttrSpace methods
• ui.ControlConfig has public boolean attrSpace = false; but no isAttrSpace or setAttrSpace methods
• ui.ControlEntity has setAttrSpace and setNoAttrSpace methods, but no isAttrSpace method.
• ui.FrameWidget has setAttrSpace and isAttrSpace methods.
• ui.GenericFrame has setAttrSpace method, but no isAttrSpace method.

As part of this task, there were changes to the ThinClient.getFrameCol() method.

The original code:

   public double getFrameCol(int frameId)
   {
      Frame frame = getFrame(frameId);
      double result = 0;

      if (frame != null && frame.isVisible() && !frame.isRedirected())
      {
         result = ConfigHelper.getAssignedColumn(frame.config());
      }

      if (result == BaseConfig.INV_COORD)
      {
         result = 0;
      }

      return ++result;
   }

The changed code which caused a regression in ChUI:

   public double getFrameCol(int frameId)
   {
      Frame frame = getFrame(frameId);
      double result = 0;

      if (frame != null && frame.isVisible() && !frame.isRedirected())
      {
         result = ConfigHelper.getAssignedColumn(frame.config()) + 1;
      }

      if (result == BaseConfig.INV_COORD)
      {
         result = 0;
      }

      return result;
   }

The problem here is that this line result = ConfigHelper.getAssignedColumn(frame.config()) + 1; can now assign result = -2147483647 where previously it could result in result = -2147483648. Consider that INV_COORD Integer.MIN_VALUE -2147483648. This means that the next code if (result == BaseConfig.INV_COORD) will be true in the original version and false in the changed version. In the original code that means the result would be reset to 0 and then incremented before returning it. But in the changed code the -2147483647 was returned directly which is bad. This breaks our ChUI regression suite as you can imagine that code dependent upon this value being 1 will not work well when given -2147483647 instead.

I've backed out this change in 4231b rev 11610. The proper fix will need to be in the next branch.

Task branch 4231b has been merged to trunk as revision 11347.

Greg Shah wrote:

As part of this task, there were changes to the ThinClient.getFrameCol() method.

I've backed out this change in 4231b rev 11610. The proper fix will need to be in the next branch.

Fix checked into 3821c-11354.

Code Review Task Branch 3821c Revision 11354

This is incorrect. ConfigHelper.getAssignedColumn(frame.config()) can return BaseConfig.INV_COORD. If you then add 1 to that number, then you will bypass the code below which resets it to 0.

If you test the broken ChUI application (test xxx 146), you will see the change does not work.

Greg Shah wrote:

Code Review Task Branch 3821c Revision 11354

This is incorrect. ConfigHelper.getAssignedColumn(frame.config()) can return BaseConfig.INV_COORD. If you then add 1 to that number, then you will bypass the code below which resets it to 0.

If you test the broken ChUI application (test xxx 146), you will see the change does not work.

If ConfigHelper.getAssignedRow(frame.config()) can return BaseConfig.INV_COORD as well, I will have to modify getFrameRow() method, as well, as that is where I took the getFrameCol() code from.

Roger Borrello wrote:

If ConfigHelper.getAssignedRow(frame.config()) can return BaseConfig.INV_COORD as well, I will have to modify getFrameRow() method, as well, as that is where I took the getFrameCol() code from.

It does not. So the implementations will need to be a little different. I'll update.

This was implemented in 3821c-11358. Please review, and let me know if there's a way to run an individual test for that ChUI application. I am scrambling to restore some disk space to the laptop so I can complete a customer build.

Code Review Task Branch 3821c Revision 11358

This seems better. Confirm it with testing.

Greg Shah wrote:

Code Review Task Branch 3821c Revision 11358

This seems better. Confirm it with testing.

Regression testing was successful.

My estimates:

SCROLL - unfortunately, that looks very time-consuming to me.
USE-TEXT - it works, but frame layout is not quite correct.
NO-ATTR-SPACE - documentation states that it has no effect.

auto-indent - that may take some time to investigate behavior in edge cases

SCROLL-NOTIFY, OFF-END, OFF-HOME, PARENT-WINDOW-CLOSE - shoudn't be a problem.
DESELECT(DESELECTION) - assuming that selection/deselection of widgets work fine, that shouldn't be a problem.

So, only SCROLL looks complicated as this point.

Focus on clearing everything except SCROLL. That one will be worked last.

1. Sets frame.setUseText(true) which has no actual effect.
2. Converts widgets to TextWidget instead of FillInWidget.

It makes a widget affected by USE-TEXT to be correctly displayed as a text widget. However it may have incorrect width/height/font type because these parameters are controlled by widget.explicitViewAs(true) flag.

It would probably make sense to check useText flag of the parent frame in the same location as explicitViewAs flag. But the problem is that there may be no parent frame when explicitViewAs is checked (e.g. in widget initialize).

So the obvious solution I see is to add widget.explicitViewAs(true) for widgets affected by USE-TEXT. What do you think?

And another issue to fix:

widget VIEW-AS FILL-IN ... WITH FRAME frame USE-TEXT

I don't understand how explicitViewAs flag is used during frame construction. AFAIK this flag is used only on client-side, after the frame has been setup. And a widget can't be pushed to the client-side without its frame.

I don't understand how explicitViewAs flag is used during frame construction. AFAIK this flag is used only on client-side, after the frame has been setup.

Yes, it is used on the client side (defines if the font is fixed in initialize, and in nativeWidth/nativeHeight).

And a widget can't be pushed to the client-side without its frame.

That's good! That means I can use TC.getFrame(config.frameId) there.

Constantin Asofiei wrote:

I don't understand how explicitViewAs flag is used during frame construction. AFAIK this flag is used only on client-side, after the frame has been setup. And a widget can't be pushed to the client-side without its frame.

OK, I see what you are referring to - initialize is called on client-side before the widget is attached to the frame as a child widget (and its parent is set). So you can't find it via that.

But the widget's config.frameId should be already set - and you can resolve it from the registry via its ID.

Stanislav Lomany wrote:

I don't understand how explicitViewAs flag is used during frame construction. AFAIK this flag is used only on client-side, after the frame has been setup.

Yes, it is used on the client side (defines if the font is fixed in initialize, and in nativeWidth/nativeHeight).

And a widget can't be pushed to the client-side without its frame.

That's good! That means I can use TC.getFrame(config.frameId) there.

Exactly.

USE-TEXT changes are committed 3821c rev 12317. Please review.

Code review 3821c revision 12317.

Text.initialize assumes there is always a valid parent frame. This assumption may not hold true for dynamic widgets.

Fixed in 3821c revision 12322.

AUTO-INDENT committed 3821c rev 12364.

I assume than implementation of SCROLL-NOTIFY should include SCROLL-HORIZONTAL and SCROLL-VERTICAL events (they are not quite the same).

Yes.