FWD » Core Development » User Interface

Make sure that even if the option supported in conversion, that the runtime support is implemented/complete.

NO-UNDERLINE, KEEP-TAB-ORDER, THREE-D, FONT, DEFAULT-BUTTON, CANCEL-BUTTON, NO-VALIDATE, SIZE, COLOR, WIDTH, USE-DICT-EXPS, SIZE-CHARS, WIDGET-ID, NO-HELP, STREAM-IO, NO-HIDE, TOP-ONLY, NO-AUTO-VALIDATE

High priority frame options for milestone 4:

COLOR
FONT
IN WINDOW
STREAM-IO

1. COLOR
   syntax:
   

   { NORMAL | INPUT | MESSAGES | protermcap-attribute | dos-hex-attribute | 
     { [ BLINK- ] [ BRIGHT- ] [ fgnd-color ] [ bgnd-color ] } | 
     { [ BLINK- ] [ RVV- ] [ UNDERLINE- ] [ BRIGHT- ] [ fgnd-color ] } | 
     VALUE ( expression )
   

   
   What is not working is value(expression). Everything else converts to frame.setDColor(new ColorPhrase(...))
2. FONT integer-expression
   Only case is working is FONT int-constant. Converts to frame.setFont(constant)
3. STREAM-IO
   Converts to frame.setStreamIO(true) when present
4. IN WINDOW <handle-expression>
   This should convert to a frame.[update|display|set|etc](elementList#, <handle-expression>) but I can't put the handle expression as parameter. Any ideas how I should glue the handle expression with the UI statement ?

1. IN WINDOW <handle-expression>
   This should convert to a frame.[update|display|set|etc](elementList#, <handle-expression>) but I can't put the handle expression as parameter. Any ideas how I should glue the handle expression with the UI statement ?

I found the way to emit the IN WINDOW handle to the API, now the annoying part is to add API versions for each statement which accepts the IN WINDOW clause (in CommonFrame and GenericFrame).

In regard to the COLOR option, all of the cases are "custom" COLOR names (the protermcap-attribute). VALUE is not used for milestone 4.

The FONT clause that is in use is only the FONT NUM_LITERAL case ("FONT 8" actually).

IN WINDOW <handle-expression>

This should convert to a frame.[update|display|set|etc](elementList#, <handle-expression>) but I can't put the handle expression as parameter. Any ideas how I should glue the handle expression with the UI statement ?

I am concerned with the approach you have taken. I always assumed that the use of IN WINDOW <my_window_handle> within a FRAME_PHRASE meant that the frame was permanently associated with the given window (as its parent). In other words, I expected it to be like other things in the frame phrase: a static configuration item. I did not expect that it would be a temporary thing passed to the specific language statement like DISPLAY but then not honored in other usage of the frame. Can you please confirm which interpretation is correct? If I am correct, then we only need to provide a configuration call from the business logic to associate the given window handle as a frame's parent window and the GenericFrame/CommonFrame changes could be removed.

I do think we are going to need to add IN WINDOW support for other (non-frame) statements like MESSAGE, PAUSE... but these are not in use for milestone 4. And since they are not frame-related, we would work on them in some other task.

Because of my confusion above, I am not going into testing with this change yet.

Looks like my instinct was correct. Using this test:

DEF VAR w1 AS HANDLE.
DEF VAR w2 AS HANDLE.
DEF VAR i AS INT.

CREATE WINDOW w1.
CREATE WINDOW w2.

w1:TITLE = "w1".
w2:TITLE = "w2".

DISPLAY i WITH FRAME f1 IN WINDOW w1.
MESSAGE FRAME f1:WINDOW:TITLE. /* this shows w1 */
PAUSE. /* the frame is shown in w1, but not in w2 */

i = 10.
DISPLAY i WITH FRAME f1 IN WINDOW w2.
MESSAGE FRAME f1:WINDOW:TITLE. /* this shows w2 */
PAUSE. /* the frame is removed from w1 and moved to w2 */

i = 1000.
DISPLAY i WITH FRAME f1 IN WINDOW w1.
MESSAGE FRAME f1:WINDOW:TITLE. /* this shows w1 */
PAUSE. /* the frame is removed from w2 and moved to w1 */

I wish you were not right but clearly you are. Luckily for us, you are right often. :) But being right this time means more work for us later.

I should have known this was the case by the fact that an expression (even if just a handle) was included. This means it has to be resolved at runtime and anything that works that way is not a static part of the frame.

I reviewed the code and it looks fine. It is going into conversion testing now.

Passed conversion testing and checked into bzr as 10195.

For milestone 4, only a limited amount of runtime is needed (see note 3 above). In addition, if we must, we can even do a limited form of the features because the use cases in milestone 4 are limited. However, I prefer to do a more complete implementation of these 4 features unless absolutely necessary.

1. COLOR - there are a few thousand places in the app that use the color directives, all of the color specs are custom color names (from protermcap, presumably). The plan would be to do the following:

• manually extract the custom color definitions and map those to the CHUI support we have already
• augment the color support for additional colors in CHUI mode if needed
• create a format for storing a list of custom colors (in XML) that would be optionally read from the application jar
• build the feature to load the custom_color_map.xml from the jar and to modify the color tables with the results
• implement/fix custom color support to ensure that we properly honor the colors

I expect 60 hours for this work.

2. FONT - only "FONT 8" is used (5 times in 2 files). As far as I know, fonts are ignored in CHUI mode. We will have to test if this is true or not. If not ignored, then this could take some time to implement. I will plan on 4 hours for investigations to prove it is not needed for the server project.

3. STREAM-IO - this is used in 38 locations across 15 files. The idea is that:

If you specify STREAM-IO for a frame, the USE-TEXT option is assumed and all font specifications are ignored. The frame is formatted using a fixed font in a manner appropriate for streaming to a text file or printer. In particular, all border padding for FILL-IN widgets is dropped and the default system font is used.

USE-TEXT is described like this:

Specifies that the default widget type for all widgets in the frame is TEXT rather than FILL-IN. Thus, all border padding on the widgets is dropped.

This will change how the frame definition is generated. There may be little in the way of runtime work since CHUI doesn't have different fonts (I hope). At a minimum, we may need to put in some TODOs to disable the font specifications at runtime OR we might need to change the frame generation to drop font specs. I expect this to take 32 hours.

4. IN WINDOW clause - only used in 2 places and only with CURRENT-WINDOW/DEFAULT-WINDOW. This will be dependent upon #2018 and I think it is primarily about properly hooking up the right window for the frame and any necessary resetting/state management that may be needed. I will plan for 40 hours.

The work for #2037 should start with the COLOR implementation. The IN WINDOW clause should be already covered by #2027 and FONT and STREAM-IO are not reached by the appserver code (see #2155).

Do we have access to 4GL file where the custom colors are defined? Because we have color phrases like COLOR C-LABEL, and I think these are read from the protermcap file. IMO, it would be cleaner to maintain the legacy color name in the converted code, and resolve it at runtime using some mappings defined in some xml file.

Do we have access to 4GL file where the custom colors are defined? Because we have color phrases like COLOR C-LABEL, and I think these are read from the protermcap file.

Yes, as far as I understand it, these values come from the protermcap.

I don't have a copy of the protermcap file, but I suspect that it must be available on lindev01 or windev01.

IMO, it would be cleaner to maintain the legacy color name in the converted code, and resolve it at runtime using some mappings defined in some xml file.

Yes, I agree.

I just want to make sure we get the XML file from the application jar.

If it is expedient, I am OK with us hand-coding the XML file for this project. In the future, we would parse the protermcap and create the XML file automatically.

Greg Shah wrote:

I don't have a copy of the protermcap file, but I suspect that it must be available on lindev01 or windev01.

I searched the protermcap file on lindev01 and there is no color specification for i.e. C-LABEL. On windows, the colors are defined in ini files or registry: can't find anything related to C-LABEL.

Please ask Guy for some details. I guess we need to know:

• How do they normally "install" the various custom color names on UNIX and Windows?
• What are the colors set to in UNIX and Windows?
• Is it a supported configuration for these to be left unspecified at runtime?

#2210 was added with details about how the colors in the server project are used. Basically, they are defined in the protermcap file, from which we can extract them (manually at first, as there are not that many colors) into a separate XML file. We will need to determine the format of the colors encoded in the protermcap file.

Also, I don't understand how the color names are mapped to the color numbers in the protermcap. The only place the mapping seems to exist is in the comment at the top of the file. Sometimes the names appear embedded with the color number in the definitions below, but not always. Some color names aren't ever embedded at all, but they are definitely referenced.

I wonder if the protermcap shown would actually work for those color names. That should be tested.

Greg, see the #2210 - C-LABEL is emitted only when compiling (and converting) on UNIX, which they no longer do; they compile the sources on windows, thus emitting the WHITE/BLACK color - which appears in the protermcap file.

Yes, I understand. I just don't understand why they would ever have had a protermcap that was not properly setup with the names if it couldn't work. I am worried that the 4GL somehow would make the association if it were to be used.

Internally to P2J, a color is represented with the Color class. The Color class holds the (logical) color value and attributes. The color value (with attributes) is only meaningful to the actual console driver. The Ncurses driver interprets the logical color value as the index to the list of custom-defined color pairs. The Windows driver interprets the logical color value as the combination of color components.

The color definitions in the protermcap file seem to conform to the ANSI escape codes.

Considering the existing Java data types, the most obvious implementation approach seems to be to parse the control sequences in the protermcap file into ANSI color definitions (or Windows console API color components if needed). Then two mappings would be defined, (1) the mapping of the color name (i.e. WHITE/BLACK) to a color index, and (2) the mapping of the color index to the color value understood by the actual console driver. Because each terminal type can have its own set of control sequences, the mapping of the color index to the driver color value would be defined for each supported terminal type.

Here's a proposed sample of the xml format for storing the color mappings. Parsing the escape sequence into a more structured form probably doesn't bring any benefits as the sequence can be XSD-validated with a simple regular expression anyway.

<color-mapping>
    <ansi-terminal type="vt100">
        <color name="BLACK/WHITE" sequence="\E[30&amp;47m:\E[0&amp;34m:0:\"/>
        <color name="WHITE/RED" sequence="\E[30&amp;43m:\E[0&amp;34m:0:\"/>
    </ansi-terminal>
    <ansi-terminal type="vt220">
        <color name="BLACK/WHITE" sequence="\E[30&amp;47m:\E[0&amp;34m:0:\"/>
        <color name="WHITE/RED" sequence="\E[30&amp;43m:\E[0&amp;34m:0:\"/>
    </ansi-terminal>    

    <!-- is the windows console mapping required as well? -->
    <win32-console>
        <color name="BLACK/WHITE" attrs="FOREGROUND_BLUE | BACKGROUND_GREEN | COMMON_LVB_UNDERSCORE"/>
    </win32-console>
</color-mapping>

This is going in the right direction.

If I understand you correctly, you are suggesting that for NCurses/Curses we directly use the parsed escape sequences as the color values sent over the wire?

One question I have is: how much of this must be custom? It seems to me that some of this will be "semi-standardized". For example, the most common 16 color values sent to the terminal driver (black, red, green, yellow, blue, magenta, cyan, white in both normal and bright forms) would be commonly supported on most every device. Why not provide built-in support for these (so that P2J knows the proper codes for our list of supported terminals (vt100, vt220, vt320, xterm). We would also support blink, underline, reverse highlight attributes. My thinking is that this will cover the vast majority of use cases and then we might have a simpler color name/index map except for special cases that don't fit.

is the windows console mapping required as well?

It is my understanding that this is not done from the protermcap (which is only used on Linux/UNIX). Instead, the color definitions/mappings come from a progress-specific section of the registry. We do need to support it, but I'm not sure if we need it now. If it is straighforward to add support, let's do it.

Greg Shah wrote:

If I understand you correctly, you are suggesting that for NCurses/Curses we directly use the parsed escape sequences as the color values sent over the wire?

Yes. For reasons why, see below.

One question I have is: how much of this must be custom? It seems to me that some of this will be "semi-standardized". For example, the most common 16 color values sent to the terminal driver (black, red, green, yellow, blue, magenta, cyan, white in both normal and bright forms) would be commonly supported on most every device. Why not provide built-in support for these (so that P2J knows the proper codes for our list of supported terminals (vt100, vt220, vt320, xterm). We would also support blink, underline, reverse highlight attributes. My thinking is that this will cover the vast majority of use cases and then we might have a simpler color name/index map except for special cases that don't fit.

I think there may be a few reasons why the simple color palette would not work.

• There are a few cases of special attributes that supplement the actual colors (for example alternate fonts of the mono-chrome terminals).
• The COLOR definition in the protermcap file contains two parts - a start escape sequence before the actual text is output and the end escape sequence. The end sequence is not always the same, some colors have different end sequences defined. (Although I don't understand the logic behind the variable end sequences.) See OpenEdge Deployment:Managing ABL Applications : Maintaining User Environments : Maintaining the UNIX user environment.
• Some terminals don't conform to the ANSI escape codes.

But if we can rule out the above edge cases and somehow solve (or ignore) the problem with end-sequences, then I would also incline towards your proposed solution with the standard color palette.

is the windows console mapping required as well?

It is my understanding that this is not done from the protermcap (which is only used on Linux/UNIX). Instead, the color definitions/mappings come from a progress-specific section of the registry. We do need to support it, but I'm not sure if we need it now. If it is straighforward to add support, let's do it.

I think it makes sense to have the color mappings for all supported systems/terminals in one place. The amount of work on the Windows console driver should not differ much from the Ncurses driver.

Alright, please do try to resolve the variable end sequences as elegantly as possible. Try to implement the simple/standard color palette along with the ability to override from the application-specific color mapping XML file. That way, for the common cases this color mapping file can be simple but for the complex cases we allow the necessary customization.

Go ahead and implement the Windows support too.

Greg Shah wrote:

Alright, please do try to resolve the variable end sequences as elegantly as possible. Try to implement the simple/standard color palette along with the ability to override from the application-specific color mapping XML file. That way, for the common cases this color mapping file can be simple but for the complex cases we allow the necessary customization.

Go ahead and implement the Windows support too.

Ok, the color representation in the application layer, console drivers as well as the color mapping configuration file will be based on the knowledge of the color palette of the "standard" 16 colors. With the possibility to override the standard color with a custom escape sequence. The custom escape sequences will be suitable for custom fonts (on mono-chrome terminals), non-ANSI escape sequences, 265-color mode, etc.

Could somebody explain the rationale behind the varying end escape sequences? I.e., why are some colors terminated with escape sequence X and some colors with escape sequence Y? Alternatively I can try to figure this out on my own if I get access to a Progress instance with the possibility to modify the protermcap file.

Could somebody explain the rationale behind the varying end escape sequences? I.e., why are some colors terminated with escape sequence X and some colors with escape sequence Y?

I wish I could, but I don't know the answer.

Alternatively I can try to figure this out on my own if I get access to a Progress instance with the possibility to modify the protermcap file.

lindev01 should be sufficient. You can edit your own version of the protermcap file and then force your 4GL session to use that modified version.

In the original code, ColorSpec contains the method addMapping(String, Color) allowing dynamically add color mappings into the ColorSpec's internal map and through the method ColorSpec.getChanges to push the added colors to the connected client. The client then stores the colors in ServerState.colors and pushes them further down to ChuiScreenDriver or ConsoleDriver. The drivers don't use the colors, the calls end in empty implementations of ScreenDriver.addColors.

Since I didn't find any bindings from the converted code to ColorSpec.addMapping, I'd like to know the expected use case.

I believe this was intended as the mechanism by which the server-side code would configure custom colors and have them transferred to and registered at the client. Since we never finished color support, the code was left empty on the client.

Code Review 0117a

I was disconnected most of the weekend and didn't see your update until now. I'll check the 0119a update next. For now, here is the review of the first update.

1. Is the color-map.xml an example of the customer-specific (custom) color map?

2. The code current warns if there is no loaded color map. If the color map is optional, we probably don't want that to be output each time the first session is created on the server. Make it INFO level instead.

3. You have removed the state synchronization code (ColorSpec, ThinClient and ServerState). That is fine so long as you have an alternative transport for moving the color information to the client. But I don't see anything there. That would generally be something in ClientExports (implemented by ThinClient and called by LogicalTerminal). The reason this is important is because:

• The custom color map (if it exists) will be loaded from the application jar.
• The application jar doesn't exist on the client, only on the server.
• ColorSpec is a server-side class and is not used on the client. It is meant to provide the interface for colors to the converted business logic.

There will also have to be some code added to configure the colors in the driver once these are sent down.

4. It seems like getColorAttribute() in terminal_linux.c is not complete or may need enhancements to get all colors working right. Is that correct?

5. addArrayNative() in terminal.c has a memory leak in the case where chrsp is allocated but attrsp is not OR both chrsp and attrsp are allocated successfully and colorsp is not. At that point the function exits without releasing the already allocated arrays.

6. Is it necessary to split the attribute from the color definition? What are the total number of active colors supported by Progress at any time (i.e. what is their palette size)? It seems if that is limited, then the attributes and colors can be encoded in a single int. Your new approach passes 2 arrays of ints for these values.

7. I personally like ColorAttribute better than CharacterAttribute.

8. In LogicalTerminal, the terminalType member will always be null. I like the idea of caching the result of any previous call to client.getTerminalType(), but it is actually never cached.

Code Review 0119a

1. Why is there a dependency on javax.jws.soap.SOAPBinding.Use in ConsolePrimitives?

2. Please provide individual javadoc comments for each E_ constant in ColorMapLoader.

3. Please put the catch() statement on its own line (see line 165) in ColorMapLoader.

4. ColorPalette.java copyright date range should not start in 2005. If you did any work on this file in 2013, then make it 2013-2014 otherwise just 2014.

5. Please put the else clause on line 119 of ColorSpec on its own line.

Greg Shah wrote:

Code Review 0117a
1. Is the color-map.xml an example of the customer-specific (custom) color map?

Yes. I built the color-map.xml based on the protermcap file attached to #2210. It needs to be tested to confirm, the converted mapping yields the expected outputs.

2. The code current warns if there is no loaded color map. If the color map is optional, we probably don't want that to be output each time the first session is created on the server. Make it INFO level instead.

Ok.

3. You have removed the state synchronization code (ColorSpec, ThinClient and ServerState). That is fine so long as you have an alternative transport for moving the color information to the client. But I don't see anything there. That would generally be something in ClientExports (implemented by ThinClient and called by LogicalTerminal). The reason this is important is because:

• The custom color map (if it exists) will be loaded from the application jar.
• The application jar doesn't exist on the client, only on the server.
• ColorSpec is a server-side class and is not used on the client. It is meant to provide the interface for colors to the converted business logic.

• the color-map.xml file is loaded on the server side and map of supported colors is built,
• the converted code on the server calls LogicalTerminal.putScreen (I am simplifying) with a ColorSpec instance
• before the client remote call is made, ColorSpec.convert is called producing a Color instance containing all the information so that the actual terminal driver can pick the right color and attributes
• the Color instance is sent to the client through Java serialization to be picked by the active terminal driver

4. It seems like getColorAttribute() in terminal_linux.c is not complete or may need enhancements to get all colors working right. Is that correct?

The function is actually complete. The way it works is that it calculates an index of the expected color pair from the foreground and background color values. The reason this gives the expected color pair is the specific color-pair layout built by the function initColors also in terminal_linux.c.

5. addArrayNative() in terminal.c has a memory leak in the case where chrsp is allocated but attrsp is not OR both chrsp and attrsp are allocated successfully and colorsp is not. At that point the function exits without releasing the already allocated arrays.

Right, this is embarrassing. Will be fixed.

6. Is it necessary to split the attribute from the color definition? What are the total number of active colors supported by Progress at any time (i.e. what is their palette size)? It seems if that is limited, then the attributes and colors can be encoded in a single int. Your new approach passes 2 arrays of ints for these values.

Progress reference documentation doesn't mention the limit of the color palette for unix systems, or at least I didn't find it. In theory the palette size depends on the output terminal. I think the practical limit for character based terminals is 256 colors. One color value defined for the foreground and one for the background. The number of "usable" attributes (mentioned at http://en.wikipedia.org/wiki/ANSI_escape_code) and excluding colors is 27. Then one bit left for the default color. Then there is this mysterious 8-bit wide dos-hex-attribute in the COLOR phrase.

But yes, at the moment, we would be able to represent all the currently supported attributes and colors in one integer value.

Do you want me to merge the color and attribute?

7. I personally like ColorAttribute better than CharacterAttribute.

Ok.

8. In LogicalTerminal, the terminalType member will always be null. I like the idea of caching the result of any previous call to client.getTerminalType(), but it is actually never cached.

True, I miss the initialization in the getter. Will fix.

Greg Shah wrote:

Code Review 0119a

1. Why is there a dependency on javax.jws.soap.SOAPBinding.Use in ConsolePrimitives?

It must have gotten there by Eclipse when implementing the changes. Will be removed.

2. Please provide individual javadoc comments for each E_ constant in ColorMapLoader.

3. Please put the catch() statement on its own line (see line 165) in ColorMapLoader.

4. ColorPalette.java copyright date range should not start in 2005. If you did any work on this file in 2013, then make it 2013-2014 otherwise just 2014.

5. Please put the else clause on line 119 of ColorSpec on its own line.

2-5: Ok.

Another argument for splitting the color from attribute I forgot to mention is the Color class itself. It also keeps the color and attribute in separate int fields. Even considering they are actually sent over the wire.

Unless I am missing a usecase here, the code changes are in-line with your points.

OK, I was misunderstanding the purpose of the state sync. I guess when the custom override support is added, some kind of transport will be needed? So long as we are just using the predefined color palette, then no definitions ever need to be transported.

Please do test the COLOR statement and the common usage of setting the color for specific widgets/frames using the format phrase or frame phrase (e.g. DCOLOR or PFCOLOR).

4. It seems like getColorAttribute() in terminal_linux.c is not complete or may need enhancements to get all colors working right. Is that correct?

The function is actually complete. The way it works is that it calculates an index of the expected color pair from the foreground and background color values. The reason this gives the expected color pair is the specific color-pair layout built by the function initColors also in terminal_linux.c.

OK. I think I did not fully understand the TODO comment in ColorPalette which is creating an implicit mapping of P2J color numbers and the ncurses color ordinals. Please add the following text:

"The implicit ordinal created by Java for this enum directly maps each enum "element" to the "COLOR_element" constant in NCurses/Curses. This mapping is implicitly utilized in NCurses/Curses drivers without any further translation. Other drivers will have to provide some kind of mapping from these values to their native rendering."

This is mostly a restatement of what you already have put in the comment, but I hope it may clarify things for future readers.

Do you want me to merge the color and attribute?

No, leave it as is.

Greg Shah wrote:

Unless I am missing a usecase here, the code changes are in-line with your points.

OK, I was misunderstanding the purpose of the state sync. I guess when the custom override support is added, some kind of transport will be needed? So long as we are just using the predefined color palette, then no definitions ever need to be transported.

I assume this "custom override support" will enable the converted code to define new colors on the fly. IMO the state sync mechanism wouldn't help anyway, because the client already receives ready-made Color instances with all the values in-place. So the custom override must take place already on the server, and probably using a similar mechanism like the color-map. Anyway, I don't know the functional details, so the implementation may be more complicated.

Please do test the COLOR statement and the common usage of setting the color for specific widgets/frames using the format phrase or frame phrase (e.g. DCOLOR or PFCOLOR).

I've tested frame, message, alert box, editor widget, label widget using the COLOR phrase. I didn't test the DCOLOR and PFCOLOR phrases, so I will do that. Any other widget types to test or all the widget types?

4. It seems like getColorAttribute() in terminal_linux.c is not complete or may need enhancements to get all colors working right. Is that correct?

The function is actually complete. The way it works is that it calculates an index of the expected color pair from the foreground and background color values. The reason this gives the expected color pair is the specific color-pair layout built by the function initColors also in terminal_linux.c.

OK. I think I did not fully understand the TODO comment in ColorPalette which is creating an implicit mapping of P2J color numbers and the ncurses color ordinals. Please add the following text:

"The implicit ordinal created by Java for this enum directly maps each enum "element" to the "COLOR_element" constant in NCurses/Curses. This mapping is implicitly utilized in NCurses/Curses drivers without any further translation. Other drivers will have to provide some kind of mapping from these values to their native rendering."

This is mostly a restatement of what you already have put in the comment, but I hope it may clarify things for future readers.

Ok, will add.

Any other widget types to test or all the widget types?

FILL-IN and BUTTON.

A question about DCOLOR and PFCOLOR. Is the following sample the correct way to set the protermcap color?

def button b2 dcolor blue/black.

The above statement fails to convert, I am getting an unexpected token "blue/black". Progress reference documentation unfortunately doesn't give the exact answer about the right syntax.

I don't fully know the answer. I think you will have to do some testing in the real 4GL.

I do know how we have coded the matching logic in progress.g:

ui_stuff
   :
      (
           KW_BGCOLOR^
         | KW_DCOLOR^
         | KW_FGCOLOR^
         | KW_PFCOLOR^ 
         | KW_FONT^         
         | KW_CTX_H_ID^
         | KW_COL^
         | c:KW_COLUMNS^ { #c.setType(KW_COL); }
         | KW_ROW^
         | KW_MOU_PTR^
         | KW_COL_BGC^
         | KW_COL_DC^
         | KW_COL_FGC^
         | KW_COL_FONT^
         | KW_COL_PFC^
         | KW_LAB_BGC^
         | KW_LAB_DC^
         | KW_LAB_FGC^
         | KW_LAB_FONT^
         | KW_LAB_PFC^
         | KW_ACCEL^
         | KW_WID_ID^
      )
      expr
   ;

This means that it must be followed by a valid expression. In simple tests on lindev01, this code generates a result like:

** Unknown Field or Variable name - blue/black. (201)

If you use "blue/black" then you get this:

Incompatible data types in expression or assignment. (223)
FGCOLOR, BGCOLOR, PFCOLOR, or DCOLOR require an integer expression. (3381)

But what I don't know is if this changes when you have a protermcap that properly configures this as a color.

Please investigate further.

Here are the findings.

DCOLOR and PFCOLOR exist only in the form of attributes of INTEGER type. For the character UI the value corresponds to the value of the color definition in protermcap file. For the graphical UI the value corresponds to an index into a color table that can be managed through the COLOR-TABLE handle. P2J handles setting the value of DCOLOR/PFCOLOR ok.

So I am extending the color-map.xml to include the integer color value and the ColorSpec class to be able to map from an integer value to actual Color instance.

How soon can you upload a candidate for review? I will get it reviewed quickly and hopefully it will be ready for regression testing (and retesting of any of the previously identified manual testing that has not been redone on this version).

Our deadline for M7 is tomorrow. It is important to get this into testing tonight so that we have time to fix/re-test tomorrow if needed.

The implementation is finished. I am doing basic functional tests and will be uploading for review in about half an hour.

Code Review 0123a

I am fine with the version, except that the color-map.xml should not be checked into the P2J project since it is customer-specific. I presume that if it does not exist, that an application (e.g. MAJIC) that has no such map will work just like it did before this update.

If my assumption is correct, you can delete that file, upload the final update here and start regression testing. Also please re-do the manual testing on the final update.

It looks good. Go ahead with testing.

hc_upd20140123b.zip passed regression testing and was checked-in to bazaar repository as revision 10451.

The following is the remaining work for this task. These need to be implemented for conversion and runtime for FRAME, DIALOG and (some for) WINDOW. The conversion support is there for many of these already, but if not there, please do add it. Of course, you will need to craft testcases to fully explore functionality before implementing the runtime side.

Attributes

BOX (frame)
OVERLAY (frame)
TOP-ONLY (frame and window)
BORDER-* (frame and dialog)
BACKGROUND (frame and dialog)
VIRTUAL-* (frame and dialog)
DEFAULT-BUTTON (frame and dialog)
CANCEL-BUTTON (frame and dialog)
SCROLL-BARS (window)
SCROLLABLE (frame and dialog)
THREE-D (frame, dialog and window)
IDE-PARENT-HWND (window, just make a null implementation, do not implement real runtime function for this one)
IDE-WINDOW-TYPE (window, just make a null implementation, do not implement real runtime function for this one)

Options

NO-UNDERLINE
THREE-D
SCROLLABLE
DEFAULT-BUTTON
CANCEL-BUTTON
NO-VALIDATE
USE-DICT-EXPS
WIDGET-ID
NO-HELP
NO-AUTO-VALIDATE
STREAM-IO
TOP-ONLY

All references to IDE-PARENT-HWND and IDE-WINDOW-TYPE have been removed from the current project. Do NOT implement support for these in this task.

Task #2424 left some unfinished work related to frame sizing/scrolling. Frame must honor the VIRTUAL* attributes. This involves:
- updating the values of the related attributes when the VIRTUAL* values are set,
- validating the VIRTUAL* values,
- sizing the frame properly and displaying scroll bars according to native 4GL,
- improving frame layout manager to honor the VIRTUAL* attributes.

Another open item: please test the affect of widget auto-resizing upon frame size and layout. See #2567-296 .

Instead of using 2038a, please contribute your changes into 2272b. Ovidiu has already put some changes in place in that branch, but the changes are focused on the conversion side only. Please:

1. Review the changes there.
2. Fill in the gaps in conversion and make sure that the conversion is complete/working for all of the needed features.
3. After the conversion support is complete, then work on the runtime support.

Igor: I assume that you are working on item 2 above. Please document the gaps you found and when do you expect it to be complete.

Greg Shah wrote:

Igor: I assume that you are working on item 2 above. Please document the gaps you found and when do you expect it to be complete.

Yes, I'm working on this item now. I will post my findings at the end of the day.

I've noticed a strange change in the conversion logic.

We have a BoxInterface and the following conversion rule:

         <rule>list.put(prog.kw_box,      execLib("cr_descr", "Box"                        , "isBox"                 , "setBox"                 , true       ))</rule>

And an unwrapping method

   public BoxInterface unwrapBox()
   {
      return unwrapImpl(this, BoxInterface.class);
   }

However the reference to the BOX attribute is converted to unwrapBoxInterface instead of unwrapBox. The similar situation with DEBLANK attribute.

See #2272 notes 80 and 81. It should be fixed.

The current state is described below:

Attributes

BOX (frame)                                DONE
OVERLAY (frame)                            DONE
TOP-ONLY (frame and window)                Conversion and runtime support for FRAME is done. TODO: Runtime support for WINDOW
BORDER-* (frame and dialog)                Both conversion (config fields/getters) and runtime to be implemented
BACKGROUND (frame and dialog)              Runtime suppore to be implemented
VIRTUAL-* (frame and dialog)               Seems to be done, more detailed testing is required
DEFAULT-BUTTON (frame and dialog)          Setters/Getters and runtime to be implemented
CANCEL-BUTTON (frame and dialog)           Setters/Getters and runtime to be implemented, conversion should be changed to support assignment
SCROLL-BARS (window)                       The conversion support is done (with config field getters/setters), but the semantics seems to be not implemented yet.
SCROLLABLE (frame and dialog)              Seems to be done, more detailed testing is required
THREE-D (frame, dialog and window)         There is a conversion support for SESSION which doesn't look correct the setter SessionUtils.set3D() is not static. The conversion for is implemented for the Widget and setters/getters generate errors.

Options

NO-UNDERLINE           The conversion support is done (with config field getters/setters), but the semantics seems to be not implemented yet.
THREE-D                There is a conversion support for SESSION which doesn't look correct the setter SessionUtils.set3D() is not static. The conversion for is implemented for the Widget and setters/getters generate errors.
SCROLLABLE             Seems to be done, more detailed testing is required
DEFAULT-BUTTON         Both conversion and runtime to be implemented
CANCEL-BUTTON          Both conversion and runtime to be implemented
NO-VALIDATE            The conversion support is done (with config field getters/setters), but the semantics seems to be not implemented yet.
USE-DICT-EXPS          No conversion support (the option is just ignored)
WIDGET-ID              Conversion support is done in #2567. The semantics implementtion was postponed.
NO-HELP                The conversion support is done (with config field getters/setters), but the semantics seems to be not implemented yet.
NO-AUTO-VALIDATE       The conversion support is incomplete (the corresponding setter generates an error message)
STREAM-IO              The conversion support is incomplete (the corresponding setter is a noop)
TOP-ONLY               Conversion and runtime support for FRAME is done. TODO: Runtime support for WINDOW

Not in the scope

IDE-PARENT-HWND (window, just make a null implementation, do not implement real runtime function for this one)
IDE-WINDOW-TYPE (window, just make a null implementation, do not implement real runtime function for this one)

How quickly can you get the conversion parts of this done? Just stub out the runtime features that aren't already there, so converted code can compile.

Greg Shah wrote:

How quickly can you get the conversion parts of this done? Just stub out the runtime features that aren't already there, so converted code can compile.

Actually the conversion part is done now (at least my simple tests where converted and compiled).

The DEFAULT-BUTTON and CANCEL-BUTTON semantics.

1. If DEFAULT-BUTTON/CANCEL-BUTTON are specified in the frame phrase but the the corresponding BUTTON widgets are not included in the frame widgets' list then the clauses are silently ignored (the corresponding getters return UNKNOWN).
2. The dynamic BUTTON must have DEFAULT attribute == true before its handle can be assigned to the frame's DEFAULT-BUTTON attribute. Otherwise the assigment attempt is rejected with 4078 error.
3. If the dynamic BUTTON is assigned to the DEFAULT-BUTTON frame attribute then but the frame's handle was not assigned to the BUTTON FRAME attribute then at the moment when the frame is enabled the 4040 and 4104 error messages are generated and the button will be neither visible nor working. The same about CANCEL-BUTTON attribute but no error messages are generated.
4. If the DEFAULT-BUTTON or the CANCEL-BUTTON frame attributes are assigned after the frame and the assigned dynamic button has no assigned FRAME attribute is realized the 4040 and 4104 errror mesages are generated. The corresponding attributes will be assigned but the buttons will be neither visible nor operational (even if the FRAME attribute will be assigned to the BUTTON after that).
5. The attempt to assign a handle of the dynamic button to the DEFAULT-BUTTON or the CANCEL-BUTTON frame attributes of the frame is rejected withe 4078 error message if the FRAME attribute of the BUTTON is different from the frame's handle

I have a question. In the 4GL documentation the semantics of the DEFAULT-BUTTON/CANCEL-BUTTON is described using the notion of the frame family. I understand that this means a set of nested frames. Some time ago I've already worked with the situation where nested frames where important. At this time we haven't yet supported nested frames. Do we support them now (I could figure that from the brief code analysis)?
Thank you.

Igor Skornyakov wrote:

I have a question. In the 4GL documentation the semantics of the DEFAULT-BUTTON/CANCEL-BUTTON is described using the notion of the frame family. I understand that this means a set of nested frames. Some time ago I've already worked with the situation where nested frames where important. At this time we haven't yet supported nested frames. Do we support them now (I could figure that from the brief code analysis)?
Thank you.

No, we don't support them yet. See #1798, which will be worked later.

If you find any behavior that is dependent upon frame family support, please add notes to the #1798 task so that we can implement those features are part of that work. In that case, link to those notes from here and explain that the feature has been deferred.

Greg Shah wrote:

If you find any behavior that is dependent upon frame family support, please add notes to the #1798 task so that we can implement those features are part of that work. In that case, link to those notes from here and explain that the feature has been deferred.

OK, I will. Thank you.

Igor Skornyakov wrote:

The attached 4GL program runs OK in 4GL but the conversion doesn't compile. Of course this program is not `correct` (the DEFAULT-BUTTON and CANCEL-BUTTON options are ignored in 4GL as described in note 62). However formally speaking this is an incompatibility.
Should I try to fix this?

Yes, please do if it can be done quickly. The easiest way to do this is to hide the nodes in the tree when you detect that this condition is going to occur. We often do such things in annotations/cleanup.rules.

Greg Shah wrote:

Yes, please do if it can be done quickly. The easiest way to do this is to hide the nodes in the tree when you detect that this condition is going to occur. We often do such things in annotations/cleanup.rules.

OK, I will try. I was thinking about replacing frame.setCancelButtonOption(<button name>); with frame.setCancelButtonOption(null); This is compatible with 4GL behavior. In any case I have added separate setters for CANCEL-BUTTON/DEDAULT-BUTTON@ options and can process null argument correctly.

Setting the LABEL attribute for the dynamic widget causes NPE if the FRAME attribute was not assigned yet.

Setting the LABEL attribute for the dynamic widget causes NPE if the FRAME attribute was not assigned yet.

Please fix it.

Greg Shah wrote:

Please fix it.

I'm working on it now. However it is not trivial as the LABEL setter is not just an assignment. It invokes size check and other frame-dependent logic, so there are at least 3 places where NPE can happen.

I've fixed NPE issue per se. However I'm not sure that my fix is absolutely correct as the additional logic is not invoked now. I guess it should be invoked when the FRAME attribute will be assigned. I will check this later.

We will defer the work until later. I've created #2754 for that work.

Committed NPE fix (note 69) and (partial runtime support for the CANCEL-BUTTON/DEFAULT-BUTTON options/attributes) to the branch 2272b revno 10990.

4GL accepts any widget type as the CANCEL-BUTTON/DEFAULT-BUTTON option value (but not the attribute). Moreover, the semantics is partially supported at least if the widget which is declared as the value of the DEFAULT-BUTTON option is FILL-IN (the ON CHOOSE trigger associated with this widget is invoked).

The argument type for the corresponding option setters was changed.

Committed to the 2272b branch revno 10992.

Igor Skornyakov wrote:

4GL accepts any widget type as the CANCEL-BUTTON/DEFAULT-BUTTON option value (but not the attribute). Moreover, the semantics is partially supported at least if the widget which is declared as the value of the DEFAULT-BUTTON option is FILL-IN (the ON CHOOSE trigger associated with this widget is invoked).

The argument type for the corresponding option setters was changed.

Committed to the 2272b branch revno 10992.

This particular "feature" is low value. Let's defer the work. How about just detecting this non-button case and use UnimplementedFeature.todo() to report it. Also: create a new task in the UI sub-project and make it related to this one. Make sure I am a watcher.

I've noticed two issues with the attached program.

We will work those in #2705 and #2729.

Igor Skornyakov wrote:

The attached 4GL program runs OK in 4GL but the conversion doesn't compile. Of course this program is not `correct` (the DEFAULT-BUTTON and CANCEL-BUTTON options are ignored in 4GL as described in note 62). However formally speaking this is an incompatibility.
Should I try to fix this?
Thank you.

No, we will work that later as documented in note 77.

Greg Shah wrote:

This particular "feature" is low value. Let's defer the work. How about just detecting this non-button case and use UnimplementedFeature.todo() to report it. Also: create a new task in the UI sub-project and make it related to this one. Make sure I am a watcher.

Actually I've changed the signature of setters so now the program which used this "feature" will at least compile. However I will create a new task as it requires more detailed investigation.

Actually I've changed the signature of setters so now the program which used this "feature" will at least compile.

I understand that. My request for "detecting this non-button case and use UnimplementedFeature.todo() to report it" is meant to ensure that any such compiled code that actually executes will report a problem. Clearly the runtime functionality will not be correct. Without this logging, it would be very hard to track down the problem.

I've added runtime support for the CANCEL-BUTTON (event re-routing).

Committed to branch 2272b revno 10995.

However there are two issues.
1. Now pressing the ESCAPE key results in the (almost) immediate exit from the program albeit the key is processed and the FrameGuiImpl.processDefaultButton() is invoked. In 4GL it is possible to intercept such an exit via RETURN NO-APPLY in the ON-CHOOSE trigger.
2. There are some strange issues with the button's ON CHOOSE trigger activation.

Investigating.

Greg Shah wrote:

I understand that. My request for "detecting this non-button case and use UnimplementedFeature.todo() to report it" is meant to ensure that any such compiled code that actually executes will report a problem. Clearly the runtime functionality will not be correct. Without this logging, it would be very hard to track down the problem.

Actually the runtime functionality can be incorrect only regarding the event re-routing. I will add the logging to FrameGuiImpl.processDefaultButton() method where such a routing is initiated.

Added logging mentioned in the note 83.

Committed to branch 2272b revno 10996.

Minor fix in the FrameGuiImpl.

Committed to branch 2272b revno 10997.

More about note 64.
The `setup` method of the <Frame>Def class should use null instead of a widget name (or do not contain setDefaultButtonOption/setCancelButtonOption at all) if the widget is not in a FORM_ITEM's list.

I do not understand how to achieve that so far.

It is my understanding that all conversion items are resolved as of the current 2272b branch 11000.

Please summarize the remaining work on this task in the format of note 59. You can remove WIDGET-ID, USE-DICT-EXPS, IDE-PARENT-HWND and IDE-WINDOW-TYPE, since none of these are going to be worked in this task.

Greg Shah wrote:

It is my understanding that all conversion items are resolved as of the current 2272b branch 11000.

This is correct.

Please summarize the remaining work on this task in the format of note 59. You can remove WIDGET-ID, USE-DICT-EXPS, IDE-PARENT-HWND and IDE-WINDOW-TYPE, since none of these are going to be worked in this task.

The current state is described below:

Attributes

BOX (frame)                                DONE
OVERLAY (frame)                            DONE
TOP-ONLY (frame and window)                Conversion and runtime support for FRAME is done. TODO: Runtime support for WINDOW
BORDER-* (frame and dialog)                Conversion support is done (including config fields/getters). Runtime support to be implemented
BACKGROUND (frame and dialog)              Runtime suppore to be implemented
VIRTUAL-* (frame and dialog)               Seems to be done, more detailed testing is required
DEFAULT-BUTTON (frame and dialog)          Seems to be done.
CANCEL-BUTTON (frame and dialog)           Conversion support is done (including config fields/getters). There are issues with runtime support (see note 82)
SCROLL-BARS (window)                       The conversion support is done (with config field getters/setters), but the semantics seems to be not implemented yet.
SCROLLABLE (frame and dialog)              Seems to be done, more detailed testing is required
THREE-D (frame, dialog and window)         The conversion support is done but the corresponding setters generate an error message.

Options

NO-UNDERLINE           The conversion support is done (with config field getters/setters), but the semantics seems to be not implemented yet.
THREE-D                The conversion support is done but the corresponding setters generate an error message.
SCROLLABLE             Seems to be done, more detailed testing is required
DEFAULT-BUTTON         Seems to be done.
CANCEL-BUTTON          Conversion support is done (including config fields/getters). There are issues with runtime support (see note 82)
NO-VALIDATE            The conversion support is done (with config field getters/setters), but the semantics seems to be not implemented yet.
NO-HELP                The conversion support is done (with config field getters/setters), but the semantics seems to be not implemented yet.
NO-AUTO-VALIDATE       The conversion support is done.
STREAM-IO              The conversion support is done but the corresponding setters generate an error message
TOP-ONLY               Conversion and runtime support for FRAME is done. TODO: Runtime support for WINDOW

I'm still working on issues with CANCEL-BUTTON described in note 82. It appears to be tricky.

Hynek: please propose which of these would be best for you to take.

We also need to identify the priority for these items. Items that have a visual impact or which will be hit in the demo code should be done first.

I would take BORDER-*, VIRTUAL-*, SCROLL-BARS, SCROLLABLE, TOP-ONLY. I think we should prioritize.

Igor, this means your work is for these (complete them in this priority order):

1. Finish CANCEL-BUTTON.
2. BACKGROUND
3. NO-UNDERLINE
4. THREE-D
5. NO-VALIDATE (does this have any real runtime behavior? it may only affect conversion)
6. NO-HELP (does this have any real runtime behavior? it may only affect conversion)
7. NO-AUTO-VALIDATE
8. STREAM-IO (for this one, just make sure that the option can be set without errors, DO NOT implement the feature itself, see #2373)

Greg Shah wrote:

Igor, this means your work is for these (complete them in this priority order):

1. Finish CANCEL-BUTTON.
2. BACKGROUND
3. NO-UNDERLINE
4. THREE-D
5. NO-VALIDATE (does this have any real runtime behavior? it may only affect conversion)
6. NO-HELP (does this have any real runtime behavior? it may only affect conversion)
7. NO-AUTO-VALIDATE
8. STREAM-IO (for this one, just make sure that the option can be set without errors, DO NOT implement the feature itself, see #2373)

OK, thank you.

Hynek, here is the priority for your items:

1. SCROLLABLE
2. BORDER-*
3. VIRTUAL-*
4. SCROLL-BARS
5. TOP-ONLY

Added support for the STREAM-IO (and related SCREEN-IO and USE-TEXT) options for FRAME (including config field ans setters).

Committed to branch 2272b revno 11005.

Please note that SCREEN-IO cannot be used with STREAM-IO and USE-TEXT options - 4GL reports a compilation error. P2J currently doesn't complain about this.

Implemented setters for the THREE-D attribute.

Committed to the branch 2272b revno 11006

ESCAPE is now correctly routed to the ON CHOOSE trigger of the CANCEL-BUTTON.

Committed to branch 2272b revno 11012.

However the application still exits even in the case when the ON CHOOSE trigger of the CANCEL-BUTTON returns NO-APPLY.

CANCEL-BUTTON/DEFAULT-BUTTON runtime support is done.

Committed to the branch 2272b revno 11013.

I suggest to add background flag to the BaseConfig and addBackgroundWidget() method to the CommonFrame interface to solve the problem mentioned in the previous note. Is it OK?
Thank you.

I suggest to add background flag to the BaseConfig

Please describe how it would be used.

and addBackgroundWidget() method to the CommonFrame interface to solve the problem mentioned in the previous note.

Would addBackgroundWidget() be called instead of addWidget() from the initializer of the inner class of the frame definition that is the WidgetList subclass? Perhaps I am misunderstanding things.

Greg Shah wrote:

Please describe how it would be used.

and addBackgroundWidget() method to the CommonFrame interface to solve the problem mentioned in the previous note.

Would addBackgroundWidget() be called instead of addWidget() from the initializer of the inner class of the frame definition that is the WidgetList subclass? Perhaps I am misunderstanding things.

Yes, I suggest to use the new method for background widgets. The method will set the background flag. The FieldGroup which handle is the value of the BACKGROUND readable attribute will be lazyly created by the corresponding getter.

OK, implement your approach.

Greg Shah wrote:

OK, implement your approach.

Thank you. I'm working on this now.

Please do the following:

1. Create a task branch 2038a. Commit your current work there.
2. Commit your testcases to testcases/uast/....
3. Post a summary here of the how BACKGROUND works in the 4GL.
4. Post a summary here of the status of your implementation, both conversion and runtime. I [resume the conversion is long done by now and you must be deep into the runtime.
5. Post an estimate of when you will be done with BACKGROUND.

Greg Shah wrote:

Please do the following:

1. Create a task branch 2038a. Commit your current work there.

The task branch is already exists. Should I just checkout it?

2. Commit your testcases to testcases/uast/....
3. Post a summary here of the how BACKGROUND works in the 4GL.
4. Post a summary here of the status of your implementation, both conversion and runtime. I [resume the conversion is long done by now and you must be deep into the runtime.

The problem with conversion is that there is no information about the BACKGROUND option in the arguments of the process_frame function. I'm trying to understand where and how to add such an information. As of now the BACKGROUND option seems to be effectively ignored in the code generation (at a very early stage). At this moment I'm considering to change the grammar definition (progress.g) file and have spent some time to learn more about antlr.

The task branch is already exists. Should I just checkout it?

I don't think it has anything useful. And it is probably far behind the trunk by now.

Archive it as "dead" and create a new one.

As of now the BACKGROUND option seems to be effectively ignored in the code generation (at a very early stage). At this moment I'm considering to change the grammar definition (progress.g) file and have spent some time to learn more about antlr.

KW_BACKGRND should show up as a child of DEFINE_FRAME. You can deal with it the same way that KW_HEADER is handled. No parser change should be necessary. If you find there is a reason that the parser really does not a change, let's discuss it.

Greg Shah wrote:

The task branch is already exists. Should I just checkout it?

I don't think it has anything useful. And it is probably far behind the trunk by now.

Archive it as "dead" and create a new one.

OK, thank you.

As of now the BACKGROUND option seems to be effectively ignored in the code generation (at a very early stage). At this moment I'm considering to change the grammar definition (progress.g) file and have spent some time to learn more about antlr.

KW_BACKGRND should show up as a child of DEFINE_FRAME. You can deal with it the same way that KW_HEADER is handled. No parser change should be necessary. If you find there is a reason that the parser really does not a change, let's discuss it.

I was looking on how the KW_HEADER is processed and it seems to me that it is done exactly as for BACKGROUND. The arguments of the process_frame function look like the following (the program is testing/uast/background/bgnd.p) :

[{IsRootScope=true, New=false, Disp=true, Down=-1, Def=true, Index={}, Shared=false, Name=fr1, Ref=[FORMAT_PHRASE, FORMAT_PHRASE, FORMAT_PHRASE, FORMAT_PHRASE, FORMAT_PHRASE, FORMAT_PHRASE, FRAME_PHRASE, DEFINE_FRAME, WID_FRAME, FRAME_SCOPE, FRAME_PHRASE, KW_ENABLE], Wid={expr1=EXPRESSION, expr2=KW_SKIP, expr3=WID_RECT, expr4=WID_RECT, expr5=WID_RECT, expr6=WID_RECT, expr7=WID_RECT, expr8=WID_RECT, expr9=WID_RECT}, Expr=0, Scope=FRAME_SCOPE, Processed=false}]

Correct, it isn't passed as a parameter.

Look at is_form_header which processes for a form_item and looks back at previous siblings to see if the kw_header is present. You can also see how we use the add_header_widget template.

Greg Shah wrote:

Correct, it isn't passed as a parameter.

Look at is_form_header which processes for a form_item and looks back at previous siblings to see if the kw_header is present. You can also see how we use the add_header_widget template.

OK, I'm looking on it. Thank you.

Fixed conversion for the background widgets.

Committed to the task branch 2038a revno 10949.

I have a question regarding a BACKGROUND attribute support. The 4GL returns a (handle of a) FIELD-GROUP as a result. So it seems that the `right` solution is to wrap all background widgets into a FIELD-GROUP and add it to the frame widget list. However it seems that at this moment we do not support this approach (in particular the GuiWidgetFactory returns null for the FieldGroup class and I suspect that it is only the beginning).

Should I implement the `right` approach or try to find a workaround where background widgets will not be the parts of the FIELD-GROUP registered with the frame but will be the direct childs of it as it is now?

Thank you.

Should I implement the `right` approach or try to find a workaround where background widgets will not be the parts of the FIELD-GROUP registered with the frame but will be the direct childs of it as it is now?

It is an appropriate question.

We know this logic is in use in the application AND we know that we need to support a related feature CURRENT-ITERATION which also is dependent upon FIELD-GROUP support.

Please read #2162, take special note of the approach we took which can be found starting at #2162-43

Also note that #2490 is dependent on field-groups too, but I hope we are not going to work on that for this project.

To see the core idea of the previous workaround, look at GenericFrame.wrapWidgetsToFieldGroup() and how it is used. If we can implement using a similar "server-side only" approach, I am open to it. However, I worry that BACKGROUND has z-order implications. If so, then we may need some idea of this on the client.

Review the notes and the code and then please post your ideas here.

I've implemented the "server-side only field-group" support for the BACKGROUND attribute and it seems working.

Committed to the task branch 2038a revno 10950.

The updated testcases/uast/background/bgnd.p test was committed as well, revno 1385.

Code Review Task Branch 2038a Revision 10950

1. The is_form_background function is not needed in frame_scoping.rules, I think you can remove all changes from that file.

2. This code in frame_generator.xml:

                  <rule>evalLib("is_form_background", ref.parent)
                      <action>
                         tw.graft("add_background_widget", null, lastId, 
                                  "wname", name,
                                  "pname", pname,
                                  "javaname", name)
                      </action>
                  </rule>
                  <rule>!evalLib("is_form_background", ref.parent)
                      <action>
                         tw.graft("add_widget", null, lastId, 
                                  "wname", name,
                                  "pname", pname,
                                  "javaname", name)
                      </action>
                    </rule>
               </rule>

would be MUCH more efficient (and simpler) like this:

                  <rule>evalLib("is_form_background", ref.parent)
                     <action>
                    tw.graft("add_background_widget", null, lastId, 
                             "wname", name,
                             "pname", pname,
                             "javaname", name)
                 </action>
                 <action on="false">
                    tw.graft("add_widget", null, lastId, 
                             "wname", name,
                             "pname", pname,
                             "javaname", name)
                 </action>
                  </rule>

This way is faster because it only evaluates is_form_background once. Since is_form_background has a loop, it is costly.

3. Per the coding standards, no hard tabs should be used, ever. At least the changes in frame_generator.xml have hard tabs and I haven't checked your other work.

4. Per the recommendations for methods_attributes.rules all non-system-handle attributes and methods should be defined in load_descriptors. Please move kw_backgrnd there.

5. The FrameWidget.background member is missing javadoc. It also needs a blank line following it.

6. Please add a history entry to GenericFrame.

7. Please remove the added imports of java.util.stream and com.goldencode.p2j.util.Stream from GenericFrame.

8. Please remove the added imports of java.util.stream from WidgetListBase.

9. Please remove the JPRM column in the header of WidgetListBase.

10. Please review the customer code I am sending via email. Please check each of the 12 use cases and make sure that our code can handle these properly. That will mean writing additional testcases to simulate these same core functions.

Greg Shah wrote:

Code Review Task Branch 2038a Revision 10950

1. The is_form_background function is not needed in frame_scoping.rules, I think you can remove all changes from that file.

2. This code in frame_generator.xml:

[...]

would be MUCH more efficient (and simpler) like this:

[...]

This way is faster because it only evaluates is_form_background once. Since is_form_background has a loop, it is costly.

3. Per the coding standards, no hard tabs should be used, ever. At least the changes in frame_generator.xml have hard tabs and I haven't checked your other work.

4. Per the recommendations for methods_attributes.rules all non-system-handle attributes and methods should be defined in load_descriptors. Please move kw_backgrnd there.

5. The FrameWidget.background member is missing javadoc. It also needs a blank line following it.

6. Please add a history entry to GenericFrame.

7. Please remove the added imports of java.util.stream and com.goldencode.p2j.util.Stream from GenericFrame.

8. Please remove the added imports of java.util.stream from WidgetListBase.

9. Please remove the JPRM column in the header of WidgetListBase.

Fixed.
Committed to the task branch 2038a revno 10953.

10. Please review the customer code I am sending via email. Please check each of the 12 use cases and make sure that our code can handle these properly. That will mean writing additional testcases to simulate these same core functions.

I'm working on it now.

I can see two different scenarios of using the BACKGROUND attribute in the customer code.

1. Modification of the attributes of the background widget via hFrame:BACKGROUND:FIRST-CHILD.
This should work. The test will be created.

2. Adding dynamic widget to a FRAME backgound via assigning a hFrame:BACKGROUND to the PARENT attribute.
This cannot work currently. To achieve that I need to use an inner class of the GenericFrame which should support addWidget method which will update the widgets lists. The default value of the BACKGROUND attribute should be not the UNKNOWN as now but an empty FIELD-GROUP.

Created test testcases/uast/background/bgnd1.p to check customer use cases.
Committed to revbo 1386.

Fixed BACKGROUND attribute runtime support.
Committed to the task branch 2038a revno 10954.

Code Review Task Branch 2038a Revision 10954

Overall, the changes are good.

1. Please javadoc void addWidget(GenericWidget widget) in GenericFrame. Also add a blank line after the method.

2. How safe is it to return name in GenericWidget.name()? In what circumstances is it valid?

3. The use of method handles and delegated/deferred execution seems useful. But it does make the code harder to understand because the timing/ordering of operations is less clear. Please include some javadoc in FieldGroup and comments in GenericFrame.coreInitialize() to explain the approach. It should also make clear what ordering dependencies of the code are required for this approach to work.

Greg Shah wrote:

1. Please javadoc void addWidget(GenericWidget widget) in GenericFrame. Also add a blank line after the method.

Done.

2. How safe is it to return name in GenericWidget.name()? In what circumstances is it valid?

I've changed this place to avoid leaking the private mutable object. The reason for this change is that w/o it in the backgound/bgng1.p test the id was shown instead of the RECTANGLE name

3. The use of method handles and delegated/deferred execution seems useful. But it does make the code harder to understand because the timing/ordering of operations is less clear. Please include some javadoc in FieldGroup and comments in GenericFrame.coreInitialize() to explain the approach. It should also make clear what ordering dependencies of the code are required for this approach to work.

Comments are added. The execution is not actually deferred. The trick is just a `restricted delegation`: instead of publishing the reference to an object we publish a reference to its single method

Committed to the task branch 2038a revno 10956.

The next in my list is the TREE-D support. The conversion (including getters/setters) is done. Should I implement 3D drawing in the scope of this task? I have no experience with GUI low-level operations and it seems to be a huge task to me.

Thank you.

In a meantime I'm working on the rest of the list (NO-VALIDATE, NO-HELP, and NO-AUTO-VALIDATE).

Fixed formatting.

Committed to the task branch 2038a revno 10959.

Code Review Task Branch 2038a Revision 10959

The changes are good, except for some formatting AND the name of ZeroColumnLayout.isFrameNoUnderline(). I cleaned up the formatting and changed ZeroColumnLayout.isFrameNoUnderline() to ZeroColumnLayout.isFrameUnderline() since that is what the method really does (it returned the value of !fc.noUnderline, so it really reported if isFrameUnderline()).

My changes are checked in as 10960. Please review them.

Greg Shah wrote:

Code Review Task Branch 2038a Revision 10959

The changes are good, except for some formatting AND the name of ZeroColumnLayout.isFrameNoUnderline(). I cleaned up the formatting and changed ZeroColumnLayout.isFrameNoUnderline() to ZeroColumnLayout.isFrameUnderline() since that is what the method really does (it returned the value of !fc.noUnderline, so it really reported if isFrameUnderline()).

My changes are checked in as 10960. Please review them.

Thank you Greg. I agree with you changes.

In a meantime I'm working on the rest of the list (NO-VALIDATE, NO-HELP, and NO-AUTO-VALIDATE).

If you can clear these quickly, it is fine to do those first.

The next in my list is the TREE-D support. The conversion (including getters/setters) is done. Should I implement 3D drawing in the scope of this task? I have no experience with GUI low-level operations and it seems to be a huge task to me.

As always, we must start by examining testcases in Progress. Please use the following existing testcases:

./ask-gui.p
./hello.p
./demo/calc.p
./demo/calc-static.p
./toggle_box/gui/tbx_present.p
./combo_box/combo_box9_1.p
./rectangle/rect_test2.p
./rectangle/rect_test6.p
./rectangle/rect_test7_1.p
./image/image0.p
./image/image10_1.p
./button/gui_btn_test3.p
./button/gui_btn_test4.p
./button/gui_btn_test5.p
./demo/movie-ratings-dynamic.p
./demo/movie-ratings-static.p
./demo/simple_windows.p
./demo/demo_widgets.p
./window_sizing/create_empty_window.p
./window_sizing/default_empty_window.p
./window_sizing/test_runner.p
./simple_alert_box.p
./simpler_alert_box.p
./message-update6.p
./menu/simple_sm.p
./menu/popup_ext.p
./window_parenting/waitfor_2wnd.p
./frame-z-order/zw1.p
./browse/gui/browse-gui-stat1.p

Run each one in 3 modes:

• without THREE-D set
• with THREE-D option set
• with THREE-D attribute set

Document what the drawing differences are. You should probably do some screen captures to show the differences.

Based on that we can discuss the drawing changes needed.

Greg Shah wrote:

In a meantime I'm working on the rest of the list (NO-VALIDATE, NO-HELP, and NO-AUTO-VALIDATE).

If you can clear these quickly, it is fine to do those first.

I'm not sure that I can to this really fast. I've never worked with Data Dictionary and the database layer and I'm looking at them now. However It will be certainly faster than the 3D staff.

I've never worked with Data Dictionary and the database layer

Please do not waste any time on this. These features have nothing to do with database. You won't even need to do anything with the schema.

From the Progress docs:

NO-VALIDATE
Disregards all validation conditions specified in the Data Dictionary for fields
entered in this frame.

NO-AUTO-VALIDATE
Tells ABL to compile into the code all relevant validations it finds in the OpenEdge
Data Dictionary, but to run the validations only when the code for the frame or for
a field-level child-widget of the frame specifically invokes the VALIDATE() method.

NO-HELP
Disregards all help strings specified in the Data Dictionary for fields entered in this
frame.

In the 4GL, you can specify validation expressions and validation messages (VALEXP and VALMSG) and help expressions (KW_HELP) in the ADD FIELD portion of the schema. That is where each database field is defined. BUT these values have nothing to do with database. They are only ever used in the UI.

When Progress creates a frame definition during compilation, any widgets that were defined using a field (or LIKE a field or using a variable that was LIKE a field) will implicitly default their validation and help values to those in the schema. Any locally defined (in the 4GL source code) validations or help will override these defaults. The overrides can be a bit complex because you can add overrides in multiple places in the source file and there is a precedence order to picking which one is honored. The key is that if no overrides are specified, then the expressions specified in the schema are picked up.

We already support all this behavior. During conversion we copy the validation and help expressions into the field or variable references in the 4GL AST. For validation expressions, I believe this occurs during rules/fixups/post_parse_fixups.xml, see schema_validations.rules and schema_validations_post.rules. At that point, the tree looks just the same as if the validation expressions were defined in the 4GL source code as an override. In other words, once copied from the schema into the source code AST, the rest of the conversion occurs without any knowledge of where the validation expressions came from. The help is similar, but I think we copy that even earlier, during parsing using the uast/SymbolResolver and uast/Variable classes. The effect is the same, it is just like the values were defined in the source code. Conversion naturally handles the rest.

NO-VALIDATE and NO-HELP may only affect the conversion. It sounds like these should just disable the copying from the schema. It is important that you write testcases to prove that it is only a conversion thing. In other words, I assume it doesn't actually change how the runtime works. It may just drop the schema expressions out but I assume that any explicitly defined validation or help in the source code would still be honored at runtime.

NO-AUTO-VALIDATE does seem like a runtime thing, but it may also have conversion implications. You will need to write testcases to check the behavior. In particular, I wonder if this also will disable explicitly specified (non-schema) validation expression processing or if it only disables validations that came from the schema. If it differentiates the validation expressions (schema or not schema as the original source), then we will have to "remember" this during conversion and notify the runtime of the difference. We also need to prove that running the VALIDATE() manually actually does cause those suppressed validations to execute. Remember: don't trust the docs.

Greg Shah wrote:

I've never worked with Data Dictionary and the database layer

Please do not waste any time on this. These features have nothing to do with database. You won't even need to do anything with the schema.

From the Progress docs:

[...]

In the 4GL, you can specify validation expressions and validation messages (VALEXP and VALMSG) and help expressions (KW_HELP) in the ADD FIELD portion of the schema. That is where each database field is defined. BUT these values have nothing to do with database. They are only ever used in the UI.

When Progress creates a frame definition during compilation, any widgets that were defined using a field (or LIKE a field or using a variable that was LIKE a field) will implicitly default their validation and help values to those in the schema. Any locally defined (in the 4GL source code) validations or help will override these defaults. The overrides can be a bit complex because you can add overrides in multiple places in the source file and there is a precedence order to picking which one is honored. The key is that if no overrides are specified, then the expressions specified in the schema are picked up.

We already support all this behavior. During conversion we copy the validation and help expressions into the field or variable references in the 4GL AST. For validation expressions, I believe this occurs during rules/fixups/post_parse_fixups.xml, see schema_validations.rules and schema_validations_post.rules. At that point, the tree looks just the same as if the validation expressions were defined in the 4GL source code as an override. In other words, once copied from the schema into the source code AST, the rest of the conversion occurs without any knowledge of where the validation expressions came from. The help is similar, but I think we copy that even earlier, during parsing using the uast/SymbolResolver and uast/Variable classes. The effect is the same, it is just like the values were defined in the source code. Conversion naturally handles the rest.

NO-VALIDATE and NO-HELP may only affect the conversion. It sounds like these should just disable the copying from the schema. It is important that you write testcases to prove that it is only a conversion thing. In other words, I assume it doesn't actually change how the runtime works. It may just drop the schema expressions out but I assume that any explicitly defined validation or help in the source code would still be honored at runtime.

NO-AUTO-VALIDATE does seem like a runtime thing, but it may also have conversion implications. You will need to write testcases to check the behavior. In particular, I wonder if this also will disable explicitly specified (non-schema) validation expression processing or if it only disables validations that came from the schema. If it differentiates the validation expressions (schema or not schema as the original source), then we will have to "remember" this during conversion and notify the runtime of the difference. We also need to prove that running the VALIDATE() manually actually does cause those suppressed validations to execute. Remember: don't trust the docs.

I see. Thank you very much Greg.

Rebased task branch 2038a from P2J trunk revision 10961 (new branch revision 10961).

Rebased task branch 2038a from P2J trunk revision 10950 (new branch revision 10962).

Greg Shah wrote:

Please do not use the Sports2000 database or any other of the Progress sample databases. Please review "Getting Started with P2J Development", especially the section entitled "Using Database (or Not)". There is guidance in there specifically about the non-use of sample databases from Progress. We have a small p2j_test database that you can use instead. For the example below, I created my own using the data dictionary. It took just a few minutes.

Sorry Greg, I forgot about our policy regarding Progress sample databases, Thank you for the sample.

In regard to your findings, I think you are starting with an over-complicated testcase and I don't understand why. It just makes it harder to see the behavior because it is mixed together with so much other functionality.

If you are finding that "the feature doesn't do anything", then almost always it is because your testcase is not written properly to expose that feature. It is a very rare feature in the 4GL that actually does nothing at all. Often the features are pretty poorly implemented and maybe even nearly useless. But usually there is some backing functionality there.

The functionality can clearly be seen with a very simple testcase:

[...]

Run it using the attached "bogus" database. The testcase can be run even in ChUI.

I didn't mean that the NO-VALIDATE is always a noop, I've just reported my findings. Thank you for the sample code.

I didn't mean that the NO-VALIDATE is always a noop, I've just reported my findings.

Fair enough.

In the future, please start with your own simpler testcases and then make it more complicated if and when needed. The code you used looks like it came from some Progress manual or from their samples. It adds many kinds of complexity that just obscures things.

Greg Shah wrote:

In the future, please start with your own simpler testcases and then make it more complicated if and when needed. The code you used looks like it came from some Progress manual or from their samples. It adds many kinds of complexity that just obscures things.

This was indeed from the Progress manual. It is simpler for me at the beginning to start with the textbook example, modifying it step by step. This is what I did when started to work with 4GL UI. After a while I'll be able to write samples from the scratch.

The CHOOSE operation failes in the GUI mode:

java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1285)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
        at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.ClassCastException: com.goldencode.p2j.ui.client.gui.driver.GuiOutputManager cannot be cast to com.goldencode.p2j.ui.client.chui.driver.ChuiOutputManager
        at com.goldencode.p2j.ui.client.ChooseHandler.drawChoice(ChooseHandler.java:463)
        at com.goldencode.p2j.ui.client.ChooseHandler.drawIfActive(ChooseHandler.java:96)
        at com.goldencode.p2j.ui.client.Frame.draw(Frame.java:1803)
        at com.goldencode.p2j.ui.client.widget.AbstractContainer.draw(AbstractContainer.java:326)
        at com.goldencode.p2j.ui.client.widget.Viewport.draw(Viewport.java:64)
        at com.goldencode.p2j.ui.client.widget.AbstractContainer.draw(AbstractContainer.java:326)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl.access$501(BorderedPanelGuiImpl.java:37)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl$1$1.run(BorderedPanelGuiImpl.java:200)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:1923)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl$1.run(BorderedPanelGuiImpl.java:189)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:1923)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl.draw(BorderedPanelGuiImpl.java:136)
        at com.goldencode.p2j.ui.client.gui.ScrollPaneGuiImpl$2.run(ScrollPaneGuiImpl.java:163)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:1923)
        at com.goldencode.p2j.ui.client.gui.ScrollPaneGuiImpl.draw(ScrollPaneGuiImpl.java:157)
        at com.goldencode.p2j.ui.client.widget.AbstractContainer.draw(AbstractContainer.java:326)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl.access$501(BorderedPanelGuiImpl.java:37)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl$1$1.run(BorderedPanelGuiImpl.java:200)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:1923)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl$1.run(BorderedPanelGuiImpl.java:189)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:1923)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl.draw(BorderedPanelGuiImpl.java:136)
        at com.goldencode.p2j.ui.client.Window.draw(Window.java:1360)
        at com.goldencode.p2j.ui.client.gui.WindowGuiImpl.draw(WindowGuiImpl.java:464)
        at com.goldencode.p2j.ui.client.OutputManager.setInvalidate(OutputManager.java:1275)
        at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:13653)
        at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:13552)
        at com.goldencode.p2j.ui.chui.ThinClient.enterChoose(ThinClient.java:3671)
        at com.goldencode.p2j.ui.chui.ThinClient.choose(ThinClient.java:3522)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:257)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1128)
        at com.goldencode.p2j.net.Queue.transact(Queue.java:585)
        at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:223)
        at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:163)
        at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1425)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:97)
        at com.sun.proxy.$Proxy4.standardEntry(Unknown Source)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:277)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:100)
        at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:201)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:398)
        at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:95)
        at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:267)

It is impossible to remove the cast as there are no at methods in the OutputManager (they are defined in ChuiOutputManager but not GuiOutputManager)

Implemented runtime support for the NO-HELP option

Committed to the task branch 2038a revno 10963.

Fixed runtime support for the NO-HELP option (no effect for the BROWSE)

Committed to the task branch 2038a revno 10964.

Implemented runtime support for the NO-VALIDATE/NO-AUTO-VALIDATE options

Committed to the task branch 2038a revno 10965.

The BROWSE widget never validates the input (see previously submitted upbrowse.p program). With 4GL it skips validation only if NO-AUTO-VALIDATE option was specified.

Should I fix it in the scope of this task?

The CHOOSE operation failes in the GUI mode:

1. What testcase are you running?

2. What does Progress do in the GUI for the same code?

The attached program runs OK in 4GL (with the bogus database), but the converted version reports error 4084: Attribute WIDTH must be greater than zero on FILL-IN widget. This happens in GUI mode only. In ChUI it is OK.

Open a new sub-task of 2677 to report this.

Implemented runtime support for the NO-VALIDATE/NO-AUTO-VALIDATE options

Please document here the following:

1. What is the behavior that needs implementation at conversion?
2. What is the behavior that needs implementation at runtime?
3. What testcases should I look at to see your experiments?

The BROWSE widget never validates the input (see previously submitted upbrowse.p program). With 4GL it skips validation only if NO-AUTO-VALIDATE option was specified.

Are you talking about ChUI or GUI?

Greg Shah wrote:

The BROWSE widget never validates the input (see previously submitted upbrowse.p program). With 4GL it skips validation only if NO-AUTO-VALIDATE option was specified.

Are you talking about ChUI or GUI?

I cannot run the test in GUI mode because of the issue mentioned in the note 139, but looking at the code I think that it happns in both modes.

2. What does Progress do in the GUI for the same code?

It just works. The converted program works in ChUI mode.

Can you please explain what you mean by this? Perhaps a screen shot will help explain.

Are you running it with prowin32?

Please see my comment at the end of note 137

You're talking about P2J in that comment, right? I'm asking about what Progress does. Of course, if Progress provides CHOOSE support in GUI we may have to make changes.

On the other hand, the current app we are working on only has 2 places that use it and it may be ChUI code that is dead. So once I know how Progress supports it I can make a call about the priority and approach.

Igor Skornyakov wrote:

Greg Shah wrote:

The BROWSE widget never validates the input (see previously submitted upbrowse.p program). With 4GL it skips validation only if NO-AUTO-VALIDATE option was specified.

Are you talking about ChUI or GUI?

I cannot run the test in GUI mode because of the issue mentioned in the note 139, but looking at the code I think that it happns in both modes.

Do both Progress ChUI and Progress GUI handle browse validation the same way?

Greg Shah wrote:

Do both Progress ChUI and Progress GUI handle browse validation the same way?

As far as I can see they do the same. The browse validation can be suppressed only by setting NO-AUTO-VALIDATE option. The NO-VALIDATE seems to be ignored.

Greg Shah wrote:

1. What is the behavior that needs implementation at conversion?

In the conversion it is required to add corresponding fields to the FrameConfig and implement corresponding setters.

2. What is the behavior that needs implementation at runtime?

I've tested UPDATE statement and updatable BROWSE. In both cases NO-AUTO_VALIDATE suppressed input data validation. The NO-VALIDATE suppresses validation for the UPDATE statement but is ignored by BROWSE

3. What testcases should I look at to see your experiments?

See testcases/uast/update/update.p revno 1395.

1. What is the behavior that needs implementation at conversion?

In the conversion it is required to add corresponding fields to the FrameConfig and implement corresponding setters.

Please be more specific.

Have you reviewed the rulesets that I described in note 128? Based on the 4GL documentation (which is often wrong), it seems like specifying NO-VALIDATE may change which validation expressions are honored. During conversion, validation expressions are copied from the schema into the code AST. Each widget definition can have multiple different validation expressions specified in different places. The conversion then uses a precedence hierarchy to determine which validation expression is actually honored in the 4GL.

Besides the rules/fixups/ schema validation expression processing, there are rules/annotations/validation*.rules which is where we do most of the validation expression preparation during conversion. We even already have some support for KW_NO_VALID there, but I don't know how complete it is.

I don't see anything in your testcase (it is very simple and really doesn't explore too much) or in this task history that suggests you have considered the conversion aspects of this.

Earlier in the task history I asked if what parts of the NO-VALIDATE support was conversion and what parts were runtime. I don't think you have answered that question.

You have implemented NO-VALIDATE at runtime exclusively. If it is set, then no validation expressions will ever be executed for that widget. I think that is wrong and a simple testcase would have shown this:

create bogus.test.
update bogus.test.num validate(num < 0, "num must be negative") with frame f0 no-validate.

In this case, the explicit validation expression does execute. The schema validation does not. Your current runtime implementation would break this.

Please look much deeper into our current processing for validations. Then please create SIMPLE testcases. Ignore BROWSE to start with. Use basic frames and data types. Explore questions such as:

• How multiple validation expressions may be honored or not (how the precedence of expressions works with NO-VALIDATE), if there are multiple expressions, in which cases does the schema expression "win" and is that affected by NO-VALIDATE?
• If the position of NO-VALIDATE on the first frame versus on a subsequent frame phrase has an effect (this is mentioned in comments in our validation_prep.rules?
• Can non-schema validation expressions ever be affected by NO-VALIDATE?
• Does NO-VALIDATE have ANY RUNTIME behavior at all?

My guess would be that if NO-VALIDATE is present in the first statement that defines a frame, that it will ignore any schema validation expressions that would normally be copied in to the widget. But I suspect that otherwise it has no effect. In other words, I suspect this is just a conversion thing. I may be wrong. But I would like you to be very careful in your analysis and I expect the testcases should allow me to see your results clearly.

Greg Shah wrote:

My guess would be that if NO-VALIDATE is present in the first statement that defines a frame, that it will ignore any schema validation expressions that would normally be copied in to the widget. But I suspect that otherwise it has no effect. In other words, I suspect this is just a conversion thing. I may be wrong. But I would like you to be very careful in your analysis and I expect the testcases should allow me to see your results clearly.

Thank you Greg,
Now I understand what you mean by "conversion only". I will examine the NO-VALIDATE more thoroughly.

The conversion doesn't create Validator when both VALIDATE() and NO-VALIDATE is specified in the UPDATE statement. For the BROWSE widget the Validator seems to be never created. Working on fixing this.

The runtime should only check if NO-AUTO-VALIDATE option was set for the FRAME. Another possible approach is to suppress Validator creation/assignment if this option is specified regardless of the source of the validation rule.

Removed runtime check for the FRAME:NO-VALIDATE option. Added conversion support for the BROWSE:NO-VALIDATE option/attribute.

Committed to the task branch 2038a revno 10966.

Another possible approach is to suppress Validator creation/assignment if this option is specified regardless of the source of the validation rule.

If we suppress then Validator creation/assignment, then the explicit validation that is caused by calling the VALIDATE statement would not be possible. Make sure your testcases check to prove that VALIDATE works even when NO-AUTO-VALIDATE is specified. Please also check what happens with VALIDATE, when a widget has schema validation suppressed using NO-VALIDATE. Is the validation expression still missing in that case?

Greg Shah wrote:

Another possible approach is to suppress Validator creation/assignment if this option is specified regardless of the source of the validation rule.

If we suppress then Validator creation/assignment, then the explicit validation that is caused by calling the VALIDATE statement would not be possible. Make sure your testcases check to prove that VALIDATE works even when NO-AUTO-VALIDATE is specified. Please also check what happens with VALIDATE, when a widget has schema validation suppressed using NO-VALIDATE. Is the validation expression still missing in that case?

OK, thank you.

Code Review Task Branch 2038a Revision 10966

1. There is no serialization logic added to BrowseConfig.

2. Your @LegacyAttribute annotations in BrowseInterface have a typo NO-VALIDATER instead of NO-VALIDATE.

The runtime should only check if NO-AUTO-VALIDATE option was set for the FRAME. Another possible approach is to suppress Validator creation/assignment if this option is specified regardless of the source of the validation rule.

When you have written enough testcases to know this is the case, then I wonder if we still need the NO-VALIDATE option setters and config in the standard widget hierarchy? frame_generator.xml sets this values today, but if it is really not needed we should remove that. But make sure we have done enough variations using testcases to know.

Also: please review and test the programs that use validation from testcases/uast/*valid*. Those may give you a better idea of the range of things we need to consider.

Greg Shah wrote:

Code Review Task Branch 2038a Revision 10966

1. There is no serialization logic added to BrowseConfig.

2. Your @LegacyAttribute annotations in BrowseInterface have a typo NO-VALIDATER instead of NO-VALIDATE.

Thank you Greg. I've fixed this.

Committed to the task branch 2038a revno 10968.

Greg Shah wrote:

When you have written enough testcases to know this is the case, then I wonder if we still need the NO-VALIDATE option setters and config in the standard widget hierarchy? frame_generator.xml sets this values today, but if it is really not needed we should remove that. But make sure we have done enough variations using testcases to know.

Yes, I was thinking about this.

Also: please review and test the programs that use validation from testcases/uast/*valid*. Those may give you a better idea of the range of things we need to consider.

I see. Thank you.

In the situation when both VALIDATE expression and NO-VALIDATE option are specified with the UPDATE statement the Validator is not created. This is incorrect.

See uast/update/update1.p at revno 1396.

Please note that FRAME:VALIDATE() method returns UNKNOWN which as incorrect as well.

Plan to fix these issues as part of the task.

Greg Shah wrote:

Plan to fix these issues as part of the task.

OK.

Fixed VALIDATE conversion. Do not support explicit validation even if NO-VALIDATE option is specified.

Committed to the task branch 2038a revno 10969.

Code Review Task Branch 2038a Revision 10969

The changes look good.

Please summarize where you have gotten with your NO-VALIDATE testing and with any additional or changed testcases. I don't see anything added to bzr yet.

At this moment I'm working on the VALIDATE() method implemetation as it is neede to check the NO-AUTO-VALIDATE functionality.
I'm looking on the tests in the testcases/uast which contain both "validate" and "frame" keywords:

./browse/brws-insert-row1.p
./browse_issues.p
./buyer.p
./complex_repeat_with.p
./crew-del.p
./current_changed/x3current-changed_2f.p
./current_changed/x3current-changed_3f.p
./current_changed/x3current-changed.p
./datetime/datetime-in-temp-tables.p
./extent_params_promotion.p
./format-check6.p
./format-check7.p
./GSO168test.p
./implicit-input.p
./like_clause.p
./logical_compared_to_character.p
./pers-addr-emp-num-valexp2.p
./pers-addr-emp-num-valexp3.p
./primes.p
./temp-tables/new-interface-1/p1.p
./temp-tables/new-interface-1/p3.p
./temp-tables/new-interface-2/model-1.p
./temp-tables/new-interface-2/model-3.p
./temp-tables/new-interface-2/view-2.p
./UpdateData1.p
./update/update1.p
./update/update.p
./validate3.p
./validate-a.p
./validate-b.p
./validate_training10.p
./validate_training11.p
./validate_training3.p
./validate_training4.p
./validate_unknown_literal.p
./validation_expression_extent2.i
./validation_expression_extent.i
./widget-options2.p
./write_delay/validate_test1.i
./write_delay/write-test3.i
./write_delay/write-test4.i
./write_delay/write-test5.i
./write_delay/write-test6.i
./write_delay/write-test7.i
./write_delay/write-test8.i
./write_delay/write-test9.i
./write_trigger_fix/validate_test1.p
./write_trigger_fix/write-test5.p
./write_trigger_fix/write-test6.p
./write_trigger_fix/write-test7.p
./write_trigger_fix/write-test8.p
./write_trigger_fix/write-test9.p

So far I see that it is necessary to check the NO-HELP/NO-VALIDATE/NO-AUTO-VALIDATE with TEMP-TABLE.

Does NO-AUTO-VALIDATE disable implicit validation for all types of validation expression? Two things to check in this regard would be:

• explicit overrides of schema validations
• validation expressions on widgets created from variables instead of a database field

Greg Shah wrote:

Does NO-AUTO-VALIDATE disable implicit validation for all types of validation expression? Two things to check in this regard would be:

• explicit overrides of schema validations
• validation expressions on widgets created from variables instead of a database field

The documentation talks about Data Dictionary validation. According to the update1.p NO-AUTO-VALIDATE disable explicit overrides as well. I will check the case with variables. Thank you for pointing this.

NO-AUTO-VALIDATE semantics looks exectly the same with UPDATE for variables. See updated uast/update/update1.p revno 1397.

Igor Skornyakov wrote:

NO-AUTO-VALIDATE semantics looks exectly the same with UPDATE for variables. See updated uast/update/update1.p revno 1397.

To be clear: you are saying that NO-AUTO-VALIDATE has nothing to do with schema validations. Instead, it is best described as a way to disable implicit validation processing no matter what the source of the validation expression (schema or explicit on a field or explicit on a variable).

Is that correct?

Greg Shah wrote:

To be clear: you are saying that NO-AUTO-VALIDATE has nothing to do with schema validations. Instead, it is best described as a way to disable implicit validation processing no matter what the source of the validation expression (schema or explicit on a field or explicit on a variable).

Is that correct?

Well, this is what I see with the test.

UPDATE fails and client crashes when two fields are affected (see uast/update/update1. with @uast/update/bogus.df").

[10/30/2015 05:46:59 EDT] (org.hibernate.engine.jdbc.spi.SqlExceptionHelper:WARN) SQL Error: 42122, SQLState: 42S22
[10/30/2015 05:46:59 EDT] (org.hibernate.engine.jdbc.spi.SqlExceptionHelper:ERROR) Column "NUM1" not found; SQL statement:
insert into test (num, num1, id) values (?, ?, ?) [42122-176]
[10/30/2015 05:46:59 EDT] (com.goldencode.p2j.persist.Persistence$Context:WARNING) [00000002:0000000B:bogus-->local/bogus/primary] rolling back orphaned transaction
[10/30/2015 05:46:59 EDT] (ErrorManager:SEVERE) {00000002:0000000B:bogus} stack trace follows
com.goldencode.p2j.persist.PersistenceException: No active session to flush
        at com.goldencode.p2j.persist.Persistence.flush(Persistence.java:3884)
        at com.goldencode.p2j.persist.Persistence.flush(Persistence.java:3121)
        at com.goldencode.p2j.persist.RecordBuffer.rollback(RecordBuffer.java:5130)
        at com.goldencode.p2j.util.TransactionManager.processRollback(TransactionManager.java:4777)
        at com.goldencode.p2j.util.TransactionManager.rollbackWorker(TransactionManager.java:1735)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1622)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1579)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3314)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3265)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:7087)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:238)
        at com.goldencode.testcases.abl.Update1.execute(Update1.java:38)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1285)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
        at java.lang.Thread.run(Thread.java:745)
[10/30/2015 05:46:59 EDT] (TransactionManager.deferError:SEVERE) {00000002:0000000B:bogus} <depth = 2; trans_level = 1; trans_label = methodScope; rollback_scope = -1; rollback_label = null; rollback_pending = false; in_quit = false; retry_scope = -1; retry_label = null; ignore_err = false> [label = methodScope; type = EXTERNAL_PROC; full = true; trans_level = TRANSACTION; external = true; top_level = true; loop = false; loop_protection = false; had_pause = false; endkey_retry = false; next_or_leave = leave; is_retry = false; needs_retry = false; ilp_count = -1; pending_break = false; properties = 'ERROR, ENDKEY'] Error processing rollback; deferring throw until block/iteration completes
com.goldencode.p2j.util.ErrorConditionException: com.goldencode.p2j.persist.PersistenceException: No active session to flush
        at com.goldencode.p2j.util.ErrorManager.throwError(ErrorManager.java:1343)
        at com.goldencode.p2j.util.ErrorManager.recordOrThrowError(ErrorManager.java:1248)
        at com.goldencode.p2j.persist.RecordBuffer.rollback(RecordBuffer.java:5149)
        at com.goldencode.p2j.util.TransactionManager.processRollback(TransactionManager.java:4777)
        at com.goldencode.p2j.util.TransactionManager.rollbackWorker(TransactionManager.java:1735)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1622)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1579)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3314)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3265)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:7087)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:238)
        at com.goldencode.testcases.abl.Update1.execute(Update1.java:38)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1285)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
        at java.lang.Thread.run(Thread.java:745)
Caused by: com.goldencode.p2j.persist.PersistenceException: No active session to flush
        at com.goldencode.p2j.persist.Persistence.flush(Persistence.java:3884)
        at com.goldencode.p2j.persist.Persistence.flush(Persistence.java:3121)
        at com.goldencode.p2j.persist.RecordBuffer.rollback(RecordBuffer.java:5130)
        at com.goldencode.p2j.util.TransactionManager.processRollback(TransactionManager.java:4777)
        at com.goldencode.p2j.util.TransactionManager.rollbackWorker(TransactionManager.java:1735)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1622)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1579)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3314)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3265)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:7087)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:238)
        at com.goldencode.testcases.abl.Update1.execute(Update1.java:38)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1285)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
        at java.lang.Thread.run(Thread.java:745)
[10/30/2015 05:46:59 EDT] (com.goldencode.p2j.persist.BufferManager$DBTxWrapper:SEVERE) [00000002:0000000B:bogus-->local/bogus/primary] scope 2: rollback error
com.goldencode.p2j.persist.PersistenceException: java.lang.IllegalStateException: [00000002:0000000B:bogus-->local/bogus/primary] no current transaction available to rollback
        at com.goldencode.p2j.persist.Persistence.rollback(Persistence.java:3376)
        at com.goldencode.p2j.persist.Persistence.rollback(Persistence.java:3072)
        at com.goldencode.p2j.persist.BufferManager$DBTxWrapper.rollback(BufferManager.java:2255)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.notifyMasterCommit(TransactionManager.java:5586)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.access$3000(TransactionManager.java:5315)
        at com.goldencode.p2j.util.TransactionManager.processRollback(TransactionManager.java:4794)
        at com.goldencode.p2j.util.TransactionManager.rollbackWorker(TransactionManager.java:1735)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1622)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1579)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3314)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3265)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:7087)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:238)
        at com.goldencode.testcases.abl.Update1.execute(Update1.java:38)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1285)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
        at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.IllegalStateException: [00000002:0000000B:bogus-->local/bogus/primary] no current transaction available to rollback
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4332)
        at com.goldencode.p2j.persist.Persistence.rollback(Persistence.java:3372)
        at com.goldencode.p2j.persist.Persistence.rollback(Persistence.java:3072)
        at com.goldencode.p2j.persist.BufferManager$DBTxWrapper.rollback(BufferManager.java:2255)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.notifyMasterCommit(TransactionManager.java:5586)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.access$3000(TransactionManager.java:5315)
        at com.goldencode.p2j.util.TransactionManager.processRollback(TransactionManager.java:4794)
        at com.goldencode.p2j.util.TransactionManager.rollbackWorker(TransactionManager.java:1735)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1622)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1579)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3314)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3265)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:7087)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:238)
        at com.goldencode.testcases.abl.Update1.execute(Update1.java:38)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1285)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
        at java.lang.Thread.run(Thread.java:745)
[10/30/2015 05:46:59 EDT] (TransactionManager.deferError:SEVERE) {00000002:0000000B:bogus} <depth = 2; trans_level = 1; trans_label = methodScope; rollback_scope = -1; rollback_label = null; rollback_pending = false; in_quit = false; retry_scope = -1; retry_label = null; ignore_err = false> [label = methodScope; type = EXTERNAL_PROC; full = true; trans_level = TRANSACTION; external = true; top_level = true; loop = false; loop_protection = false; had_pause = false; endkey_retry = false; next_or_leave = leave; is_retry = false; needs_retry = false; ilp_count = -1; pending_break = false; properties = 'ERROR, ENDKEY'] Error processing master rollback; deferred error already pending; new error discarded
java.lang.RuntimeException: com.goldencode.p2j.persist.PersistenceException: java.lang.IllegalStateException: [00000002:0000000B:bogus-->local/bogus/primary] no current transaction available to rollback
        at com.goldencode.p2j.persist.BufferManager$DBTxWrapper.rollback(BufferManager.java:2265)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.notifyMasterCommit(TransactionManager.java:5586)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.access$3000(TransactionManager.java:5315)
        at com.goldencode.p2j.util.TransactionManager.processRollback(TransactionManager.java:4794)
        at com.goldencode.p2j.util.TransactionManager.rollbackWorker(TransactionManager.java:1735)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1622)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1579)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3314)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3265)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:7087)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:238)
        at com.goldencode.testcases.abl.Update1.execute(Update1.java:38)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1285)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
        at java.lang.Thread.run(Thread.java:745)
Caused by: com.goldencode.p2j.persist.PersistenceException: java.lang.IllegalStateException: [00000002:0000000B:bogus-->local/bogus/primary] no current transaction available to rollback
        at com.goldencode.p2j.persist.Persistence.rollback(Persistence.java:3376)
        at com.goldencode.p2j.persist.Persistence.rollback(Persistence.java:3072)
        at com.goldencode.p2j.persist.BufferManager$DBTxWrapper.rollback(BufferManager.java:2255)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.notifyMasterCommit(TransactionManager.java:5586)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.access$3000(TransactionManager.java:5315)
        at com.goldencode.p2j.util.TransactionManager.processRollback(TransactionManager.java:4794)
        at com.goldencode.p2j.util.TransactionManager.rollbackWorker(TransactionManager.java:1735)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1622)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1579)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3314)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3265)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:7087)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:238)
        at com.goldencode.testcases.abl.Update1.execute(Update1.java:38)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1285)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
        at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.IllegalStateException: [00000002:0000000B:bogus-->local/bogus/primary] no current transaction available to rollback
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4332)
        at com.goldencode.p2j.persist.Persistence.rollback(Persistence.java:3372)
        at com.goldencode.p2j.persist.Persistence.rollback(Persistence.java:3072)
        at com.goldencode.p2j.persist.BufferManager$DBTxWrapper.rollback(BufferManager.java:2255)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.notifyMasterCommit(TransactionManager.java:5586)
        at com.goldencode.p2j.util.TransactionManager$WorkArea.access$3000(TransactionManager.java:5315)
        at com.goldencode.p2j.util.TransactionManager.processRollback(TransactionManager.java:4794)
        at com.goldencode.p2j.util.TransactionManager.rollbackWorker(TransactionManager.java:1735)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1622)
        at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1579)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3314)
        at com.goldencode.p2j.util.TransactionManager.abnormalEnd(TransactionManager.java:3265)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:7087)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:238)
        at com.goldencode.testcases.abl.Update1.execute(Update1.java:38)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1285)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1767)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1270)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:427)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:163)
        at java.lang.Thread.run(Thread.java:745)
[10/30/2015 05:46:59 EDT] (TransactionManager.handleDeferredError:SEVERE) {00000002:0000000B:bogus} <depth = 1; trans_level = -1; trans_label = null; rollback_scope = -1; rollback_label = null; rollback_pending = false; in_quit = false; retry_scope = -1; retry_label = null; ignore_err = false> [label = startup; type = UNKNOWN; full = false; trans_level = NO_TRANSACTION; external = true; top_level = true; loop = false; loop_protection = false; had_pause = false; endkey_retry = false; next_or_leave = leave; is_retry = false; needs_retry = false; ilp_count = -1; pending_break = false; properties = ''] Throwing deferred error (com.goldencode.p2j.persist.PersistenceException: No active session to flush)

With only one field it works OK.
Investigating.

Incorrect ESCAPE processing (see testcases/uast/update/update1.p).

After pressing ESCAPE when the second frame is active 4GL returns to the first frame and hides the second one. P2J doesn't hide the second frame.

UPDATE fails and client crashes when two fields are affected (see uast/update/update1. with @uast/update/bogus.df").

Are you sure you created your database correctly in P2J? I suspect this is something in your environment and not a P2J bug.

Igor Skornyakov wrote:

Incorrect ESCAPE processing (see testcases/uast/update/update1.p).

After pressing ESCAPE when the second frame is active 4GL returns to the first frame and hides the second one. P2J doesn't hide the second frame.

Have you gotten past the bug in note 176?

Greg Shah wrote:

UPDATE fails and client crashes when two fields are affected (see uast/update/update1. with @uast/update/bogus.df").

Are you sure you created your database correctly in P2J? I suspect this is something in your environment and not a P2J bug.

This is possible of course. However it works with either num or num1 but fails with both. And I do not understand so far why the insert statement refers to the "id" field.

Greg Shah wrote:

Have you gotten past the bug in note 176?

Not yet. I've just commented out the reference to num1 in the first UPDATE statement.

UPDATE fails and client crashes when two fields are affected (see uast/update/update1. with @uast/update/bogus.df").

Are you sure you created your database correctly in P2J? I suspect this is something in your environment and not a P2J bug.

This is possible of course. However it works with either num or num1 but fails with both. And I do not understand so far why the insert statement refers to the "id" field.

Please create a version of the update1.p testcase that operates only with a temp-table. This removes the bogus database from the equation. If the two field update still fails then there is a bug in P2J. If it works, then it is most likely a local environment issue.

Have you gotten past the bug in note 176?

Not yet. I've just commented out the reference to num1 in the first UPDATE statement.

See if you can duplicate this ESC issue using a simpler testcase that only has variable usage (no temp-tables, no create statements). If you can recreate the issue, then open a new bug (subtask of #2677).

Igor Skornyakov wrote:

Started to examine the effect of the THREE-D option/attribute.
1. At the first glance the assiging of SESSION:THREE-D to true has no effect. More elaborate tesing is required.
2. A naive test of the WINDOW:THREE-D = true doesn't work as the value of this attribute cannot be changed after the widget is realized. More elaborate tesing is required.
3. The THREE-D option of the FRAME does have an effect. See screenshots of the ./combo_box/combo_box9_1.p with and w/o this option.

Hynek, some thoughts regarding this.

1. Based on the 4GL docs, SESSION:THREE-D only affects system dialogs (e.g. SYSTEM-DIALOG-GET-FILE) and alert-boxes. This may explain why Igor did not see any behavior. Confirm this and ensure there is a testcase that shows that SESSION:THREE-D does not set THREE-D "globally". In other words, windows, frames and dialogs don't have THREE-D when SESSION:THREE-D is set.

2. Based on the 4GL docs, setting the THREE-D attribute must be done before widget realization. Igor's tests show this to be accurate.

3. The 4GL docs state that setting THREE-D for window does not affect any contained frames. It also states that this just changes the background color of the window to match the "button face" system color. Please confirm this is the case.

4. The 4GL docs state that setting THREE-D for a frame or dialog automatically puts all contained widgets into THREE-D mode as well. This seems correct based on the appearance of the customer GUI code we are testing right now. I imagine that the frame or dialog background will be set to "button face" and from what I can see, this seems to also change the background color of some (all?) widgets to also be "button face". Please create a testcase that has a frame that includes a widget of each type and a dialog that includes a widget of each type. Use a yes/no alert-box to prompt the use for the THREE-D setting. Set this before you display the frame and dialog. This will let you directly compare the setting effect on each widget type in both frame and dialog cases. My sense is that the drawing of each widget is not really different, it just chooses different background colors.

5. Please confirm that the THREE-D attribute operates the same way as the THREE-D option in the frame phrase. Hopefully they are equivalent.

6. Please confirm that THREE-D has no effect on sizing/layout (as stated in the 4GL docs).

7. Please confirm that child frames don't inherit THREE-D from the parent frame (as stated in the 4GL docs).

8. Please read the 4GL documentation for the THREE-D attribute and for the THREE-D option (in the frame phrase).

I would hope this research can be done in less than a day. If my understanding is correct, then the implementation will be quite straightforward.

Greg Shah wrote:

If my understanding is correct, then the implementation will be quite straightforward.

Yes, the potential risk is item 4, in the sense of the draw differences of the individual widgets. Hopefully the differences will be minor.

Greg Shah wrote:

Please create a version of the update1.p testcase that operates only with a temp-table. This removes the bogus database from the equation. If the two field update still fails then there is a bug in P2J. If it works, then it is most likely a local environment issue.

Indeed, it was my fault. After correct H2 database creation there are no errors.

Please create a version of the update1.p testcase that operates only with a temp-table. This removes the bogus database from the equation. If the two field update still fails then there is a bug in P2J. If it works, then it is most likely a local environment issue.

Indeed, it was my fault. After correct H2 database creation there are no errors.

Great! Does that include the ESC issue?

Greg Shah wrote:

Great! Does that include the ESC issue?

Nope. The ESC issue still exists (w/o database ot temp tables). I'm creating a bug now.

It seems that the description of the VALIDATE() method in the Progress documentation is not correct.
As uast/update/update4.p test demonstrates the optional argument is ignored and the method always returns true for fields which are not enabled.

Please provide a detailed list of the findings for NO-VALIDATE. I have previously written several questions needing research (notes 154 and 171). I would like you to explicitly answer each of those (and any other key points you investigated or uncovered). That will help me understand how comprehensive your investigation has been. It will also provide a good reference for people coming later, who are trying to understand how we have implemented NO-VALIDATE.

If I understand correctly, there is no runtime behavior for NO-VALIDATE. If that is correct, have you removed the regular widget getters/setters for it?

In regard to the browse attribute NO-VALIDATE, what is the status? Please document the detailed findings there and what the implementation status is. This one may have runtime behavior for dynamic browse. Which testcases show that behavior?

Finally, although NO-HELP it is not as complicated as NO-VALIDATE, I think it is important to consider similar questions and make sure our approach matches how Progress really works. As above, I would like you to document the investigation results in enough detail that I can confirm that we are not missing some important point.

Greg Shah wrote:

Please provide a detailed list of the findings for NO-VALIDATE. I have previously written several questions needing research (notes 154 and 171). I would like you to explicitly answer each of those (and any other key points you investigated or uncovered). That will help me understand how comprehensive your investigation has been. It will also provide a good reference for people coming later, who are trying to understand how we have implemented NO-VALIDATE.

OK. I will document my findings at the weekend.

If I understand correctly, there is no runtime behavior for NO-VALIDATE. If that is correct, have you removed the regular widget getters/setters for it?

This is correct for the NO-VALIDATE option for FRAME. I haven't removed the setter as the situation is unclear with the NO-VALIDATE option for BROWSE. I think it is natural to do that for both cases.

In regard to the browse attribute NO-VALIDATE, what is the status? Please document the detailed findings there and what the implementation status is. This one may have runtime behavior for dynamic browse. Which testcases show that behavior?

I haven't check this so far as it seems that at this moment p2j doesn't support validation for BROWSE (#2799).

Finally, although NO-HELP it is not as complicated as NO-VALIDATE, I think it is important to consider similar questions and make sure our approach matches how Progress really works. As above, I would like you to document the investigation results in enough detail that I can confirm that we are not missing some important point.

Yes, I remember about that.

I haven't removed the setter as the situation is unclear with the NO-VALIDATE option for BROWSE.

For BROWSE, there needs to be a setter. But it should only be in the BrowseInterface.

I think it is natural to do that for both cases.

For non-browse widgets there is no reason for a setter. We would never use that state at runtime. It would be more code to maintain for no functional benefit. In fact, it is a net-negative because anyone reading the code would be misled that there is some reason for it to be there.

In regard to the browse attribute NO-VALIDATE, what is the status? Please document the detailed findings there and what the implementation status is. This one may have runtime behavior for dynamic browse. Which testcases show that behavior?

I haven't check this so far as it seems that at this moment p2j doesn't support validation for BROWSE (#2799).

I understand. But I'd like you to please investigate and document the capabilities of the NO-VALIDATE attribute for BROWSE. Then please reference the detailed notes in #2799 (link to the notes here from there) so that they can be found and used later.

I have two problems with implementation of the VALIDATE() method.

1. As I've mentioned in the note 189 this method seems to always return true for not-enabled widgets. See uast/update/upodate4.p. This is implemented and works for the fields in the enabled frame. However with the UPDATE statement (see uast/update/upodate4.p) this implementation is incorrect as all the widgets appear to be not enabled if the VALIDATE() is invoked after the UPDATE statement is execution. It is unclear for me now how to deal with that. Contunue investigation.

2. The validation error is displayed at the bottom of the active window in ChUI mode but in the modal dialog box in the GUI one. Is there any standard way to achieve this behavior (e.g. via ErrorManager) or I should implement it from the scratch?

Any advise is highly appreciated.

However with the UPDATE statement (see uast/update/upodate4.p)

I don't understand how you came to this conclusion. uast/update/upodate4.p has no UPDATE statement. Did you forget to check in your changes?

this implementation is incorrect as all the widgets appear to be not enabled if the VALIDATE is invoked after the UPDATE statement is execution. It is unclear for me now how to deal with that. Contunue investigation.

Please be more specific.

After UPDATE occurs, the editing session is done. So there are NO widgets enabled after the UPDATE completes. It is the equivalent of 4 combined statements (DISPLAY + ENABLE + WAIT-FOR + ASSIGN). Because the ENABLE + WAIT-FOR are both hidden inside the UPDATE, after it is done there is no more editing mode.

The validation error is displayed at the bottom of the active window in ChUI mode but in the modal dialog box in the GUI one. Is there any standard way to achieve this behavior (e.g. via ErrorManager)

You should NOT run your GUI testcases from the Progress Procedure Editor. It changes many things about the running program. Error handling is one of the impacts. If you run using prowin32 -p <program_name> you will find that everything goes to the MESSAGE area. Just use the normal ErrorManager functionality and it will be correct.

Greg Shah wrote:

I don't understand how you came to this conclusion. uast/update/upodate4.p has no UPDATE statement. Did you forget to check in your changes?

Sorry, it was I misprint. I meant update2.p.

After UPDATE occurs, the editing session is done. So there are NO widgets enabled after the UPDATE completes. It is the equivalent of 4 combined statements (DISPLAY + ENABLE + WAIT-FOR + ASSIGN). Because the ENABLE + WAIT-FOR are both hidden inside the UPDATE, after it is done there is no more editing mode.

I understand. However the VALIDATE() method returns valid results (see update2). However it doesn't display error message if validation fails.

You should NOT run your GUI testcases from the Progress Procedure Editor. It changes many things about the running program. Error handling is one of the impacts. If you run using prowin32 -p <program_name> you will find that everything goes to the MESSAGE area. Just use the normal ErrorManager functionality and it will be correct.

I see. Thank you.

However the VALIDATE method returns valid results (see update2). However it doesn't display error message if validation fails.

Could you please post the results here. I don't have time to run it myself to see the results.

Greg Shah wrote:

However the VALIDATE method returns valid results (see update2). However it doesn't display error message if validation fails.

Could you please post the results here. I don't have time to run it myself to see the results.

Consider the following TEMP-TABLE.

define temp-table tst
        FIELD num as int initial 0 help "num help" 
        FIELD num1 as int initial 0 help "num1 help" 
.

and the the following statements:

update tst.num validate(num < 0, "num must be negative") tst.num1 validate(num1 < 10, "num1 must be < 10") with frame f2 no-validate no-auto-validate no-help.
message frame f2:validate() tst.num tst.num1.

If both values are 0. The message is: no 0 0
If values are correct the output is: yes -1 9

The FRAME:NO-HELP option suppresses the schema-defined help message but not the explicit one (see uast/update/update2.p revno 1042).
This means that this option should be converted in a way similar to the VALIDATE one and the current runtime support is incorrect.

Igor Skornyakov wrote:

The FRAME:NO-HELP option suppresses the schema-defined help message but not the explicit one (see uast/update/update2.p revno 1042).
This means that this option should be converted in a way similar to the VALIDATE one and the current runtime support is incorrect.

How do you plan to convert NO-VALIDATE and NO-AUTO-VALIDATE? From the tests you posted, I think these frame options just disable the validation, and not remove it, as the validate() method can still evaluate the expression. The way I see this: the frame's validation expressions are always emitted, but there will be some special flag emitted at the frame definition class, which sets the NO-VALIDATE or NO-AUTO-VALIDATE options - so the runtime can decide what to do when validation is required.

Constantin Asofiei wrote:

Igor Skornyakov wrote:

The FRAME:NO-HELP option suppresses the schema-defined help message but not the explicit one (see uast/update/update2.p revno 1042).
This means that this option should be converted in a way similar to the VALIDATE one and the current runtime support is incorrect.

How do you plan to convert NO-VALIDATE and NO-AUTO-VALIDATE? From the tests you posted, I think these frame options just disable the validation, and not remove it, as the validate() method can still evaluate the expression. The way I see this: the frame's validation expressions are always emitted, but there will be some special flag emitted at the frame definition class, which sets the NO-VALIDATE or NO-AUTO-VALIDATE options - so the runtime can decide what to do when validation is required.

I think that the NO-VALIDATE is converted correctly now. Please note the in the update2.p we have explicit only validations which are not suppressed at the conversion. update1.p test shows the schame-defined validation is completely suppressed by NO-VALIDATE and the subsequent VALIDATE() method returns true even if value(s) do not satisfy the schema-defined validation preducate.

How do you plan to convert NO-VALIDATE and NO-AUTO-VALIDATE? From the tests you posted, I think these frame options just disable the validation, and not remove it, as the validate() method can still evaluate the expression.

As far as we can tell, NO-VALIDATE just suppresses the copying of validation expressions from the schema. Any widgets in that frame that were defined from a field that has a validation expression defined, will not have that schema validation expression as its default. This, if you edit the field or if you call validate() it will pass testing by default. However, if you specify validation expressions explicitly in a format phrase for that widget, the explicit validation expression will be honored at edit time and when you call validate(). We have not been able to find any runtime behavior for the NO-VALIDATE frame option. For this reason I have asked for it not to be set as a frame configuration value.

NO-VALIDATE attribute for browse does have runtime behavior since it can be queried for both static/dynamic widgets and set for dynamic widgets. Igor has not yet finished his research on that.

NO-AUTO-VALIDATE frame option suppresses all implicit validation (e.g. in response to a LEAVE or GO), but it does not suppress validate() for whatever validation expressions are there (which depends on the NO-VALIDATE behavior noted above. This is converted as a setter call in the frame def. And the runtime implicit validation processing checks that flag and bypasses validation expression execution if the flag is set.

As far as I understand it, the only open issues with NO-VALIDATE are some behavior with the validate() function and the research for NO-VALIDATE as a browse attribute.

NO-AUTO-VALIDATE is complete.

If both values are 0. The message is: no 0 0
If values are correct the output is: yes -1 9

This is the 4GL results?

What does P2J do?

The FRAME:NO-HELP option suppresses the schema-defined help message but not the explicit one (see uast/update/update2.p revno 1042).

This means that this option should be converted in a way similar to the VALIDATE one and the current runtime support is incorrect.

OK. When can you be ready with that?

Greg Shah wrote:

If both values are 0. The message is: no 0 0
If values are correct the output is: yes -1 9

This is the 4GL results?

What does P2J do?

Yes. this is the 4GL results. At this moment P2J returns "yes" in both cases as widgets are not enabled at the moment when VALIDATE() is invoked.

Greg Shah wrote:

The FRAME:NO-HELP option suppresses the schema-defined help message but not the explicit one (see uast/update/update2.p revno 1042).

This means that this option should be converted in a way similar to the VALIDATE one and the current runtime support is incorrect.

OK. When can you be ready with that?

I'm working on this right now and hope to finish today.

At this moment P2J returns "yes" in both cases as widgets are not enabled at the moment when VALIDATE is invoked.

I suspect this has nothing to do with the UPDATE. In Progress, when you do something like this:

def var num as int init 0.
def var num1 as int init 0.

display num validate(num < 0, "num must be negative") num1 validate(num1 < 10, "num1 must be < 10") with frame f2 no-auto-validate.
message frame f2:validate() num num1.

enable all with frame f2.
wait-for go of frame f2.

The DISPLAY causes the values of num and num1 to be pushed into the widgets. This sets the widget values to 0. The validate() returns false in this case. Notice that at the moment of the validate() call, the widget has never yet been enabled for editing.

The issue has nothing to do with UPDATE. The issue is that validate() should always execute any present validation expression, ignoring whether the widget is in edit mode.

Please fix this. Also make sure it is returning correct results when there is no validation expression registered for the given widget.

Greg Shah wrote:

At this moment P2J returns "yes" in both cases as widgets are not enabled at the moment when VALIDATE is invoked.

I suspect this has nothing to do with the UPDATE. In Progress, when you do something like this:

[...]

The DISPLAY causes the values of num and num1 to be pushed into the widgets. This sets the widget values to 0. The validate() returns false in this case. Notice that at the moment of the validate() call, the widget has never yet been enabled for editing.

The issue has nothing to do with UPDATE. The issue is that validate() should always execute any present validation expression, ignoring whether the widget is in edit mode.

Please fix this. Also make sure it is returning correct results when there is no validation expression registered for the given widget.

I see. Thank you. I've already implemented the logic when VALIDATE() returns true if there is no validation expression. I will resume working on VALIDATE after finishing with NO-HELP.

Code Review Task Branch 2038a Revision 10971

1. It is incorrect to call down to the client to get the screen-buffer. Our system is "effectively" single threaded from the UI perspective. Anytime the processing is on the server, the most up-to-date screen-buffer is on the server. When we call down to the client we must pass the latest screen-buffer as a parameter. When we return from the client, any screen-buffer updates are included and replaced on the server-side. So we never should have to call down to the client to get the screen-buffer. Please use the one the frame already has on the server-side.

2. Doesn't the comparison with "ENABLED-FIELDS" need to be done case-insensitively?

3. Why do you have a protected logical validateFields(boolean enabledOnly) that uses UnimplementedFeature.missing()? This is confusing because future readers will think that we haven't yet implemented this feature. In fact, it will already be complete.

4. When you are the first person to edit a file in your task branch, you should not be adding history entries to previous trunk history entries. For example, you need a history number for your first history entry in GenericFrame, GenericWidget.

5. Please remove the extra blank line you added after GenericFrame.resetFlushed().

6. Please fix the 2 spelling errors in your 20151020 history entry for GenericFrame.

Greg Shah wrote:

Code Review Task Branch 2038a Revision 10971

1. It is incorrect to call down to the client to get the screen-buffer. Our system is "effectively" single threaded from the UI perspective. Anytime the processing is on the server, the most up-to-date screen-buffer is on the server. When we call down to the client we must pass the latest screen-buffer as a parameter. When we return from the client, any screen-buffer updates are included and replaced on the server-side. So we never should have to call down to the client to get the screen-buffer. Please use the one the frame already has on the server-side.

OK. Thank you.

2. Doesn't the comparison with "ENABLED-FIELDS" need to be done case-insensitively?

I'm not sure as it seems not have no effect at all.

3. Why do you have a protected logical validateFields(boolean enabledOnly) that uses UnimplementedFeature.missing()? This is confusing because future readers will think that we haven't yet implemented this feature. In fact, it will already be complete.

I understand you're talking about GenericWidget.validateFields() I will change the message to indicated the this method if not supported for the particular widget type.

4. When you are the first person to edit a file in your task branch, you should not be adding history entries to previous trunk history entries. For example, you need a history number for your first history entry in GenericFrame, GenericWidget.
5. Please remove the extra blank line you added after GenericFrame.resetFlushed().

6. Please fix the 2 spelling errors in your 20151020 history entry for GenericFrame.

OK.

2. Doesn't the comparison with "ENABLED-FIELDS" need to be done case-insensitively?

I'm not sure as it seems not have no effect at all.

Hmmm. That would be unexpected. It should have an effect in 2 places:

1. While in editing mode, there is possibly a subset of widgets that are enabled. The validate() method can be executed at that time, but to do so one must use a trigger. That is the only way to execute use-defined code during editing. Make sure your test only enables some of the widgets in the frame, so you can see the difference that "ENABLED-FIELDS" makes. I see that your update/update4.p should be OK, depending on the validation expressions that are active for test.num1. If you set invalid data in test.num1 during editing and then use the "A" trigger, I would expect that the validate() call will return true but if you enable test.num1 it would return false if the widget held the same invalid data.

2. When not in editing mode, I expect that validate("ENABLED-FIELDS") will never actually call any validation processing since by definition there are no widgets enabled.

Greg Shah wrote:

1. While in editing mode, there is possibly a subset of widgets that are enabled. The validate() method can be executed at that time, but to do so one must use a trigger. That is the only way to execute use-defined code during editing. Make sure your test only enables some of the widgets in the frame, so you can see the difference that "ENABLED-FIELDS" makes. I see that your update/update4.p should be OK, depending on the validation expressions that are active for test.num1. If you set invalid data in test.num1 during editing and then use the "A" trigger, I would expect that the validate() call will return true but if you enable test.num1 it would return false if the widget held the same invalid data.

This is what I expected. However the update/update4.p doesn't prove that.

2. When not in editing mode, I expect that validate("ENABLED-FIELDS") will never actually call any validation processing since by definition there are no widgets enabled.

This is possible. I will double check. Thank you.

This is what I expected. However the update/update4.p doesn't prove that.

There is something wrong with your testing.

Here is a simpler testcase that shows the effect:

def var i as int.
def var j as char.

def frame fr i validate(i > 0, "i must be positive")
             j validate(j ne "bad", "j can't be 'bad'").

on "a" anywhere do:
  message "j:screen-value = " + j:screen-value in frame fr.
  message "fr:validate() =" frame fr:validate("enabled-fields").
end.

message "before first update".
j = "bad".
update i with frame fr.

message "before second update".
j = "bad".
update i j with frame fr.

Progress definitely does honor the "enabled-fields" during an editing session.

Greg Shah wrote:

This is what I expected. However the update/update4.p doesn't prove that.

There is something wrong with your testing.

Here is a simpler testcase that shows the effect:

[...]

Progress definitely does honor the "enabled-fields" during an editing session.

Thank you Greg. I will double check.

I've checked in that testcase as validation/validate_method_with_enabled_fields_during_editing.p.

Please note these things:

1. I have named the file with a descriptive name that will help others to understand the purpose of the testcase.

2. I put it in a validation/ directory so that people needing to find testcases that match that topic can find them grouped there.

3. I did NOT use database, NO-VALIDATE, NO-AUTO-VALIDATE or any other extraneous options or features. This makes it less likely that an unanticipated impact of those options could lead to a mistaken conclusion.

4. My testcase was limited to prove 1 thing and 1 thing only. It did not have multiple purposes.

Please follow these approaches when writing your testcases.

Greg Shah wrote:

I've checked in that testcase as validation/validate_method_with_enabled_fields_during_editing.p.

Please note these things:

1. I have named the file with a descriptive name that will help others to understand the purpose of the testcase.

2. I put it in a validation/ directory so that people needing to find testcases that match that topic can find them grouped there.

3. I did NOT use database, NO-VALIDATE, NO-AUTO-VALIDATE or any other extraneous options or features. This makes it less likely that an unanticipated impact of those options could lead to a mistaken conclusion.

4. My testcase was limited to prove 1 thing and 1 thing only. It did not have multiple purposes.

Please follow these approaches when writing your testcases.

I see. I use short names to reduce typing. But I will commiy them with descriptive names in the future. Sorry.

There is something strange with the VALIDATE() method.

When running uast/validation/validate_method_in_enabled_frame_with_db.p with the bogus database the num1:validate() returns true. However with enabled explicit validation predicate which is the same as the schema-defined one it returns false. num1:validate("enabled-fields") always returns true

If num1 is enabled the behavior is the same with explicit validation and w/o it (num1:validate() returns false for incorrect value)

The behavior described in the previous note doesn't take place with the UPDATE statement. See uast/validation/validate_method_with_enable_fields_during_edition_with_db.p

I need you to do the following before I can help you:

1. Please check in the newer 4GL database files (similar to how I provided the simple_bogus_database.zip). Otherwise no one can run your testcases in the 4GL.

2. Provide a more specific set of recreate steps for me to get the exact results you are referencing. Do NOT rely upon having me edit the testcases to get different results. That is a sign that you should have more than one testcase. Make sure you have different testcases for proving different results. The instructions should include which program to run and exactly what input I should type to see your results.

3. It is very useful to put comments into the testcases:

• to explain the purpose of critical code that forms the important parts of the test
• to explain any critical inputs that have to be executed by the user in order to run the test properly
• to record results (e.g. if you have a MESSAGE statement, then it is often a good idea to put a comment there with the expected output from that MESSAGE statement)

4. Please take great care to confirm that the data in the widgets is what you expect. If you notice, my testcase explicitly sets the data values to cause validation failures. Your tests don't do that and thus you rely upon user editing alone, which can be tricky.

Also: you have test filenames that have typos. I think "edition" is supposed to be "editing". Please fix it.

Greg Shah wrote:

I need you to do the following before I can help you:

1. Please check in the newer 4GL database files (similar to how I provided the simple_bogus_database.zip). Otherwise no one can run your testcases in the 4GL.

2. Provide a more specific set of recreate steps for me to get the exact results you are referencing. Do NOT rely upon having me edit the testcases to get different results. That is a sign that you should have more than one testcase. Make sure you have different testcases for proving different results. The instructions should include which program to run and exactly what input I should type to see your results.

3. It is very useful to put comments into the testcases:

• to explain the purpose of critical code that forms the important parts of the test
• to explain any critical inputs that have to be executed by the user in order to run the test properly
• to record results (e.g. if you have a MESSAGE statement, then it is often a good idea to put a comment there with the expected output from that MESSAGE statement)

4. Please take great care to confirm that the data in the widgets is what you expect. If you notice, my testcase explicitly sets the data values to cause validation failures. Your tests don't do that and thus you rely upon user editing alone, which can be tricky.

Done. See revno 1407. The testcases in question are uast/validation/validate_method_in_enabled_frame_with_db.p and uast/validation/validate_method_in_enabled_frame_with_db_and_explicit_rule.p

Please note that you didn't record the results in comments, so the reader must run the code to see what it does. Please fix that.

I'm running the samples now.

I don't see any output of the screen-value in the message statements. It is not at all clear what the current value is in the widgets, so how can we confirm the behavior of the validation processing.

Please fix it.

Also, please force known values into the widgets before executing any validation processing.

Test updated. All results are as expected expect the last one:
uast/validation/validate_method_in_enabled_frame_with_db.p: test.num1:validate('ENABLED-FIELDS') yes test.num1:validate() yes num1 = 0
uast/validation/validate_method_in_enabled_frame_with_db_and_explicit_rule.p: test.num1:validate('ENABLED-FIELDS') yes test.num1:validate() no num1 = 0

As item 3 in note 218 requested, I wanted the results to be listed in comments in the testcases. Please bzr update to get copies of the 2 testcases with the results added. I expect all of your testcases to be commented in this way. I hope you can see how it helps readers of the testcases.

Greg Shah wrote:

As item 3 in note 218 requested, I wanted the results to be listed in comments in the testcases. Please bzr update to get copies of the 2 testcases with the results added. I expect all of your testcases to be commented in this way. I hope you can see how it helps readers of the testcases.

Thank you Greg. This is really better.

The results from validate_method_in_enabled_frame_with_db_and_explicit_rule.p make more sense than the ones from validate_method_in_enabled_frame_with_db.p.

In regard to the results from validate_method_in_enabled_frame_with_db.p, I wonder if the validation expression is actually being executed by the h1:validate()?

I went into the data dictionary and changed the validation expression for num1 to be false. This will always fail validation. After I did this, I EXITED both the data dictionary and the procedure editor. Then I ran validate_method_in_enabled_frame_with_db.p again. This time it acted exactly like the validate_method_in_enabled_frame_with_db_and_explicit_rule.p. This suggests that something is wrong with the previously encoded validation expression, such that it doesn't actually compile or work correctly. I do see that there is a difference in the data dictionary valexp (num1 > 10) and the explicit valexp (test.num1 > 10). I removed the test. prefix in the version I checked in (I forgot that change was there). But it did not resolve the difference.

Still, I think there is something wrong with the valexp processing from the schema in this case.

Greg Shah wrote:

The results from validate_method_in_enabled_frame_with_db_and_explicit_rule.p make more sense than the ones from validate_method_in_enabled_frame_with_db.p.

In regard to the results from validate_method_in_enabled_frame_with_db.p, I wonder if the validation expression is actually being executed by the h1:validate()?

I went into the data dictionary and changed the validation expression for num1 to be false. This will always fail validation. After I did this, I EXITED both the data dictionary and the procedure editor. Then I ran validate_method_in_enabled_frame_with_db.p again. This time it acted exactly like the validate_method_in_enabled_frame_with_db_and_explicit_rule.p. This suggests that something is wrong with the previously encoded validation expression, such that it doesn't actually compile or work correctly. I do see that there is a difference in the data dictionary valexp (num1 > 10) and the explicit valexp (test.num1 > 10). I removed the test. prefix in the version I checked in (I forgot that change was there). But it did not resolve the difference.

Still, I think there is something wrong with the valexp processing from the schema in this case.

I also was thinking about this. However when num1 is enabled the VALEXP is working. I will try to switch num and num1.

Well, the situaton is the same if only num1 is enabled instead of num

Added comments to the test.
Fixed the implemenation of the VALIDATE() method. Now it seems correct except the problem with schema-defined rules (see note 223).

Commited to the task branch 2038a revno 10972.

I will make final updates of the history records at the merge step.
I cannot see any 2 spelling errors in the 20151020 history entry for GenericFrame

If the issue with the schema-defined rules is real we need to distinguish between schema-defined rules and explicit ones. Should I fix the conversion and runtime accordingly or try to understand what is wrong with the test?
Thank you.

Well, the situaton is the same if only num1 is enabled instead of num

Do you mean that in this case num succeeds in validate() when the validation expression is from the schema but fails when it is explicit?

Greg Shah wrote:

Well, the situaton is the same if only num1 is enabled instead of num

Do you mean that in this case num succeeds in validate() when the validation expression is from the schema but fails when it is explicit?

Exactly.

I will make final updates of the history records at the merge step.

I prefer if you would fix things now, so that I can do a final code review.

I cannot see any 2 spelling errors in the 20151020 history entry for GenericFrame

I should have said GenericWidget:

**     IAS 20151020          setParentHanle() for the case when parent is a FieldGroup
**                           (BACKGROUNG attribute support)

If the issue with the schema-defined rules is real we need to distinguish between schema-defined rules and explicit ones. Should I fix the conversion and runtime accordingly

No. Do not change things yet.

or try to understand what is wrong with the test?

No. I'm going to ask some customer 4GL developers to see if they have any ideas.

For now, please do the known cleanup on branch 2038a. I am going to do a code review as well, right now.

The next task is for you to do the research for THREE-D as I have described in note 184. Check in the testcases and report the full set of found behavior here.

Greg Shah wrote:

For now, please do the known cleanup on branch 2038a. I am going to do a code review as well, right now.

I've fixed the spelling typo in the GenericWidget history.
Committed to the task branch 2038a revno 10973.
I will review and fix history records tomorrow. Please note however that that I've not finished the updated NO-HELP support (purely conversion based). I could not stop thinking about VALIDATE() and switched to it today.

The next task is for you to do the research for THREE-D as I have described in note 184. Check in the testcases and report the full set of found behavior here.

OK. Thank you.

Igor Skornyakov wrote:

Greg Shah wrote:

Well, the situaton is the same if only num1 is enabled instead of num

Do you mean that in this case num succeeds in validate() when the validation expression is from the schema but fails when it is explicit?

Exactly.

Am I correct in assuming that in this case you have commented out the validation expression for num in the validate_method_in_enabled_frame_with_db.p?

Greg Shah wrote:

Am I correct in assuming that in this case you have commented out the validation expression for num in the validate_method_in_enabled_frame_with_db.p?

Actually I've commented out both the validation expression for for num and test.num in the ENABLE statement and uncommented corresponding references to num1.

The next task is for you to do the research for THREE-D as I have described in note 184. Check in the testcases and report the full set of found behavior here.

Please make sure you have a regular rectangle and also one that has GROUP-BOX set.

Also, I recommend using an include file for the common frame definition. You can use a preprocessor named argument to add THREE-D option and change the title text.

Something like this (named all_widgets_frame_def.i):

DEFINE FRAME fr 
   ...
   WITH {&three-d} TITLE {&title}.

You would then use it like this:

{all_widgets_frame_def.i &title = "Window without THREE-D"}

or like this:

{all_widgets_frame_def.i &title = "Window with THREE-D" &three-d = "THREE-D"}

Greg Shah wrote:

The next task is for you to do the research for THREE-D as I have described in note 184. Check in the testcases and report the full set of found behavior here.

Please make sure you have a regular rectangle and also one that has GROUP-BOX set.

Also, I recommend using an include file for the common frame definition. You can use a preprocessor named argument to add THREE-D option and change the title text.

Something like this (named all_widgets_frame_def.i):

[...]

You would then use it like this:

[...]

or like this:

[...]

OK, thank you.

Created THREE-D tests for most widgets (see uast/3d/). As far as I can see there is no different between THREE-D option and attribute for FRAME

What is the status of your THREE-D research?

I don't yet see any answers for items 1, 3 or 6.

I also don't see a detailed list of the drawing differences from item 4. I see that you have posted screenshots, which is good. But I would like you to analyze the differences at the pixel level and report what specific drawing differences there are. In addition, I don't see anything relating to the use of WINDOW:THREE-D.

Instead of writing an individual testcase for each widget, you would have saved time to put one widget of each type into the same frame. It would also help you answer the layout question in item 6.

I also don't see anything here relating to dialog box. Everything you have done so far is only with frame.

Finally, although your use of ENABLE ALL is definitely a good test, we will also need results for the non-enabled case. Just use a DISPLAY or VIEW statement to make the non-enabled frame visible.

I recommend putting all the widget types into a single testcase. If you use the preprocessor arguments approach that I suggested above, then you won't need all the conditional logic for displaying 2 different frames. Instead you would have 1 common include file for defining the frame and then 1 procedure for each case (3d_frame.p, non_3d_frame.p, 3d_dialog.p, non_3d_dialog.p).

Greg Shah wrote:

What is the status of your THREE-D research?

I don't yet see any answers for items 1, 3 or 6.

I also don't see a detailed list of the drawing differences from item 4. I see that you have posted screenshots, which is good. But I would like you to analyze the differences at the pixel level and report what specific drawing differences there are. In addition, I don't see anything relating to the use of WINDOW:THREE-D.

Instead of writing an individual testcase for each widget, you would have saved time to put one widget of each type into the same frame. It would also help you answer the layout question in item 6.

I also don't see anything here relating to dialog box. Everything you have done so far is only with frame.

Finally, although your use of ENABLE ALL is definitely a good test, we will also need results for the non-enabled case. Just use a DISPLAY or VIEW statement to make the non-enabled frame visible.

I recommend putting all the widget types into a single testcase. If you use the preprocessor arguments approach that I suggested above, then you won't need all the conditional logic for displaying 2 different frames. Instead you would have 1 common include file for defining the frame and then 1 procedure for each case (3d_frame.p, non_3d_frame.p, 3d_dialog.p, non_3d_dialog.p).

Thank you Greg. I'm still working on it.

Is 2038a ready for a full code review?

We will continue to wait on the validate() method quirk with schema expressions. So, at this point, there is no other development pending. Is that correct?

Once any cleanups are done from the code review, it seems best to get this into regression testing.

Greg Shah wrote:

Is 2038a ready for a full code review?

We will continue to wait on the validate() method quirk with schema expressions. So, at this point, there is no other development pending. Is that correct?

Once any cleanups are done from the code review, it seems best to get this into regression testing.

The VALIDATE()/NO-VALIDATE/NO-AUTO-VALIDATE support is ready for the final review (not for the BROWSE widget where validation support its not implemented yet). However the updated NO-HELP conversion support is not finished yet.

We've also discussed that setters for NO-VALIDATE and NO-HELP should be removed. This is also not done at this moment.

Greg Shah wrote:

I don't yet see any answers for items 1, 3 or 6.

As far as I can see the documentation describes the behavior metioned in items 3 and 6 correctly.

Instead of writing an individual testcase for each widget, you would have saved time to put one widget of each type into the same frame. It would also help you answer the layout question in item 6.

Regarding the layout. If THREE-D doesn't affect the widget size (which seems to be a fact) but affects the layout we must be extremely lucky to discover this in a single test (actually with any finite number of tests). However if we'll see that in a customer code it will be at least a clue to how such tests should look like.

I don't yet see any answers for items 1, 3 or 6.

As far as I can see the documentation describes the behavior metioned in items 3 and 6 correctly.

Make sure you have testcases that show this.

Instead of writing an individual testcase for each widget, you would have saved time to put one widget of each type into the same frame. It would also help you answer the layout question in item 6.

Regarding the layout. If THREE-D doesn't affect the widget size (which seems to be a fact) but affects the layout we must be extremely lucky to discover this in a single test (actually with any finite number of tests). However if we'll see that in a customer code it will be at least a clue to how such tests should look like.

I understand your point here. It is correct. But if you at least have a non-trivial layout (by having a large number of widgets in the same frame), then we will at least be able to see that one complex case does act as expected.

The customer code will provide more thorough testcases.

Greg Shah wrote:

I understand your point here. It is correct. But if you at least have a non-trivial layout (by having a large number of widgets in the same frame), then we will at least be able to see that one complex case does act as expected.

I have a suggestion. To get more of the multiple widgets' test and do not rely just on the visual impression we can add the following: in a loop change the FONT attribute of each widget (independently) and get its X, Y, HEIGHT-PIXELS and WIDTH-PIXELS attributes' values. The results will be written to a file and we can compare these files for THREE-D on and off. Implementation of this can take me about half a day but it seems worth of this investement.
What do you think about that?
Thank you.

I think this is a good idea. However:

1. We need to finish your current work on 2038a (NO-HELP and the cleanup). This work needs to get tested and then merged with 2677a ASAP.

2. I want to get the rest of the THREE-D research done, before you work on the "other layout checking". The thing is that we can go ahead and implement THREE-D as soon as this core research is done. The other layout checking is just additional confirmation that there are not unexpected layout implications. So it can be done later.

Please work the above two items (in this order). Then do the other layout checking as you have described.

Greg Shah wrote:

I think this is a good idea. However:

1. We need to finish your current work on 2038a (NO-HELP and the cleanup). This work needs to get tested and then merged with 2677a ASAP.

2. I want to get the rest of the THREE-D research done, before you work on the "other layout checking". The thing is that we can go ahead and implement THREE-D as soon as this core research is done. The other layout checking is just additional confirmation that there are not unexpected layout implications. So it can be done later.

Please work the above two items (in this order). Then do the other layout checking as you have described.

OK. Thank you.

When will 2038a be ready for review?

Greg Shah wrote:

When will 2038a be ready for review?

If I will not finish today I will do it at the weekend. I'm still not fluent enough with code generation, sorry. With Java code I can use debugger to understand thge logic but with scripts this provides a limited help.

Hopefully we can make that faster. Ask questions here and I will try to help.

Greg Shah wrote:

Hopefully we can make that faster. Ask questions here and I will try to help.

Thank you. One of my main problems is how to access FRAME option when field is processed. For example I think that the right place to suppress a setHelp() generation is frame_generator.xml line 5051. What I do not understand so far is how check if the FRAME has a NO-HELP option.

The best way to think about the conversion is the following:

1. Each external procedure is an abstract syntax tree (AST). The tree structure is initially created by the Java parser that is created using ANTLR from the progress.g grammar.

2. The TRPL (Tree Processing Language) environment is a pipeline of "rule-sets". Each rule-set contains rules that define pattern matching with the structures in the AST and based on these matches will analyze, calculate, annotate and modify the AST. The process is driven by a PatternEngine which organizes the pipeline and executes a depth-first visit of every node in the AST. This process of walking the AST, will correspond to various "events" that correspond to various ways that a node can be visited. Each rule will be executed as a callback from one of these events. A "walk-rules" gets invoked for each AST node that is visited. At the root of the tree, before any nodes have been specifically visited there is an "init-rules" event. Likewise, there is a "post-rules" for the very end. Then there are "descent-rules" for when the walk moves from a parent to its first child, "ascent-rules" for when the walk moved from its last child back to the parent. There are "next-child-rules" for when the walk moved from one sibling node to another.

Because of the above ideas, you must always be thinking about the AST structure itself when reading any TRPL rules. Since the tree structure and content (the token types of nodes, the parent-child-sibling relationships, the token text, any annotations...) define how the rules execute, it is usually best to generate some ASTs for the representative 4GL code that you are wanting to process. Then look carefully at the AST to understand the context of the nodes you are going to analyze or manipulate.

SO: your first task is to post a snippet of the AST here that shows a DEFINE_FRAME subtree that has a NO-HELP option.

Good. From this snippet, you can see the context that the TRPL rules must expect. The option node is a KW_NO_HELP with a parent.type of FRAME_PHRASE.

To understand where that part of the tree comes from, please look at progress.g line 11522:

frame_phrase [boolean blockHdr, boolean editing]
   :
      w:KW_WITH^ { #w.setType(FRAME_PHRASE); }
      (
         options { generateAmbigWarnings = false; }
         :
           KW_WITH!   // undocumented "recursive" behavior
         | accum_clause
         | at_phrase
         | (KW_ATTR | KW_NO_ATTR) 
         | button_reference
         | KW_CENTER
         | color_spec
         | ui_stuff
         | KW_CTX_H 
         | context_help_file
         | KW_DROP_TAR
         | KW_EXPORT
         | frame_reference[true]
         | (KW_INH_BGC | KW_NO_INHBG)
         | (KW_INH_FGC | KW_NO_INHFG)
         | KW_KEEP_TAB 
         | KW_NO_BOX
         | KW_NO_HIDE 
         | KW_NO_LABEL 
         | KW_USE_DCT
         | KW_NO_VALID 
         | KW_NO_AUTOV 
         | KW_NO_HELP        <---- here is the NO-HELP option
         | KW_NO_UNDL
         | KW_OVERLAY
         | (KW_PAGE_B | KW_PAGE_T) 
         | (KW_SCRN_IO | KW_STRM_IO)
         | KW_SCROLLBL
         | KW_SCROLL_V   // undocumented but found in customer source
         | KW_SIDE_L
         | size_phrase
         | KW_3D 
         | title_phrase
         | KW_TOP_ONLY
         | KW_USE_TXT

           // version_6_frame_clause: the documentation notes that these next 3
           // keywords are dependent on each other but working customer code
           // shows the usage of KW_USE_REV on its own; so instead of calling
           // the version_6_frame_clause sub-rule, it has been separated out
           // here
         | KW_V6FRAME
         | KW_USE_REV
         | KW_USE_UND

         | view_as_dialog_clause
         | in_window_clause
         | misc_frame_opts
         | { LA(1) == KW_STREAM || LA(1) == KW_STRM_HND || LA(1) == KW_BROWSE }?
           lvalue  // used to be stream_reference and browse_reference

           // these last two MUST be last and MUST be in this order since
           // the first one matches a NUM_LITERAL in the first token and
           // the second matches any expression
         | columns_clause

           // note: inner block headers can have a frame phrase followed by
           // the transaction keyword (which can be interpreted as the
           // transaction builtin function) or a loop increment expression
           // (both of which can follow the frame phrase in a block header
           // AND both of which are ambiguous with the down clause since that
           // option starts with an optional expression), so we avoid
           // matching the transaction keyword and anything else that matches
           // will be disambiguated by this rule that can handle both the
           // down clause and the loop increment
           // warning: this can continue to match frame phrase constructs
           // after a loop increment has been matched which can't happen in
           // valid 4GL code but as long as we don't see any such invalid code
           // that behavior won't trigger
         | { blockHdr && LA(1) != KW_TRANS }?
           down_clause_or_loop_incr

           // note: the COLON test is meant to exclude matching editing
           // block label definitions BUT it may inappropriately exclude valid
           // usage of the colon as a dereferencing operator (attrs, methods,
           // com props/methods, object members/props/methods) in a down
           // clause --> watch for this
         | { !blockHdr && (!editing || LA(2) != COLON) }?
           down_clause
      )*
   ;

This shows that KW_NO_HELP will be matched after a KW_WITH so long as the only intervening tokens are other sibling constructs like the frame_reference in your example, or like KW_3D.

If you search on KW_NO_HELP in progress.g, you will only find 2 other references. One defines KW_NO_HELP as a token type and the other adds it as a keyword to our list of valid keywords in the lexer. The only use of this keyword is in the frame_phrase rule of the parser. You can also search on the other places that the parser uses a frame_phrase, to see what the grandparents of the KW_NO_HELP might be.

Please note that for any given frame, there can be many statements including a frame phrase. The frame's configuration is defined by the combination of all of these. Thus the KW_NO_HELP node can be placed in 0 or more possible frame phrases that all refer to the same frame.

What the above means is that anywhere in TRPL rules where you want to detect a match with NO-HELP, you can write something like this:

<rule>type == prog.kw_no_help and parent.type == prog.frame_phrase
   ...do stuff here based on the match...
</rule>

There are multiple issues here that make this option tricky to process:

1. For the default behavior to execute, we must know if ANY of the frame phrases for the given frame contain KW_NO_HELP. It might be specified in 10 places or just in 1 place. It's absence from ALL frame phrases will lead to the default behavior.

2. The effect of the option's presence is to disable the copying of any HELP specification from the schema for those widgets in the frame which were defined using a database field. Correct?

3. Any explicitly defined HELP option in the 4GL source code should still override the schema HELP, whether or not the NO-HELP is specified. Correct?

4. This option has no effect on HELP specified for widgets that were not defined from a database field. Correct?

I think that the right place to suppress a setHelp() generation is frame_generator.xml line 5051.

The problem here is that we don't really want to suppress the setHelp() when the help was specified explicitly in the 4GL code and/or when the widget is not defined from a field.

In addition, at that point in the tree walk we cannot see all the other possible frame phrase locations that may or may not have NO-HELP specified.

Please confirm my understanding in items 1 through 4 above. I will then post about the next steps in the analysis.

I think that the right place to suppress a setHelp() generation is frame_generator.xml line 5051.

I should also point out that this location is only used for browse columns. Search the file for all instances of "setHelp" and you will see what I mean.

Greg Shah wrote:

The problem here is that we don't really want to suppress the setHelp() when the help was specified explicitly in the 4GL code and/or when the widget is not defined from a field.

In addition, at that point in the tree walk we cannot see all the other possible frame phrase locations that may or may not have NO-HELP specified.

As I can see from my previous investigation the schema-define help and the one which is originated from the e.g. HELP uption of the UPDATE statement are represented in a different way in the AST tree so I expected that at the moment when setHelp is emitted we can understand what is the source of the help text. Hopefully at this moment we already know that is NO-HELP option was specifiled for the frame (in any possible place). Initially I was thinking to mimic the logic with NO-VALIDATE but then decided that it will be an overkill. After all if the schema-defined help is overridden is is aleady set correctly.

Please confirm my understanding in items 1 through 4 above. I will then post about the next steps in the analysis.

Thank you very much Greg. I hope that I understand this. However I cannot see which should be the next steps.

Greg Shah wrote:

I think that the right place to suppress a setHelp() generation is frame_generator.xml line 5051.

I should also point out that this location is only used for browse columns. Search the file for all instances of "setHelp" and you will see what I mean.

Thank you. I will look at other places.

Please confirm my understanding in items 1 through 4 above. I will then post about the next steps in the analysis.

Thank you very much Greg. I hope that I understand this. However I cannot see which should be the next steps.

I will help you with the next steps. But first, I need you to confirm if I am correct in my statements from items 1 through 4 in note 259.

Greg Shah wrote:

I will help you with the next steps. But first, I need you to confirm if I am correct in my statements from items 1 through 4 in note 259.

These statements look to be correct for me. Except for the BROWSE widget where the NO-HELP option of the enclosing frame doesn't suppress the schema-originated help.

Except for the BROWSE widget where the NO-HELP option of the enclosing frame doesn't suppress the schema-originated help.

Interesting. Does Progress allow a NO-HELP option to be specified for the BROWSE? It would most likely be placed in the browse_options_phrase (see progress.g) which is similar to a frame phrase but it appears in a DEFINE_BROWSE. It also uses the KW_WITH. NO-HELP is not documented to work for BROWSE, but there are many undocumented features in the 4GL. Please check this.

As I can see from my previous investigation the schema-define help and the one which is originated from the e.g. HELP uption of the UPDATE statement are represented in a different way in the AST tree so I expected that at the moment when setHelp is emitted we can understand what is the source of the help text. Hopefully at this moment we already know that is NO-HELP option was specifiled for the frame (in any possible place).

This is a good point. Yes, I think that is correct. The schema-defined help comes from the "help" annotation. That annotation is copied in during the creation of a field node in the parser. Search on "annotateField" in progress.g. The important case is in the lvalue rule. If you look at the backing logic in SymbolResolver.annotateField() you will see how it pulls the database field's definition out of the schema dictionary. It then uses this definition to create a Variable instance and then call var.annotateOptions(field, true, this); which is the code that will set the "help" annotation into the AST. Look at Variable.annotateOptions() to see the details.

Initially I was thinking to mimic the logic with NO-VALIDATE but then decided that it will be an overkill. After all if the schema-defined help is overridden is is aleady set correctly.

I agree it is overkill. The schema validations are much more complicated. However, we still need to know if NO-HELP was specified in ANY frame phrase for a given frame. Typically, the way to do this is in a stage that is earlier than frame_generator.xml, we would match on the KW_NO_HELP node and set an annotation in the frame in some way to mark that frame as a NO-HELP frame. This needs to be in a place that can be read back later when we need to know if we should bypass setting help for a field or not.

I will look for the right place for this.

Greg Shah wrote:

Interesting. Does Progress allow a NO-HELP option to be specified for the BROWSE? It would most likely be placed in the browse_options_phrase (see progress.g) which is similar to a frame phrase but it appears in a DEFINE_BROWSE. It also uses the KW_WITH. NO-HELP is not documented to work for BROWSE, but there are many undocumented features in the 4GL. Please check this.

OK. I will check it a little bit later.

As I can see from my previous investigation the schema-define help and the one which is originated from the e.g. HELP uption of the UPDATE statement are represented in a different way in the AST tree so I expected that at the moment when setHelp is emitted we can understand what is the source of the help text. Hopefully at this moment we already know that is NO-HELP option was specifiled for the frame (in any possible place).

This is a good point. Yes, I think that is correct. The schema-defined help comes from the "help" annotation. That annotation is copied in during the creation of a field node in the parser. Search on "annotateField" in progress.g. The important case is in the lvalue rule. If you look at the backing logic in SymbolResolver.annotateField() you will see how it pulls the database field's definition out of the schema dictionary. It then uses this definition to create a Variable instance and then call var.annotateOptions(field, true, this); which is the code that will set the "help" annotation into the AST. Look at Variable.annotateOptions() to see the details.

I see. Thank you.

Initially I was thinking to mimic the logic with NO-VALIDATE but then decided that it will be an overkill. After all if the schema-defined help is overridden is is aleady set correctly.

I agree it is overkill. The schema validations are much more complicated. However, we still need to know if NO-HELP was specified in ANY frame phrase for a given frame. Typically, the way to do this is in a stage that is earlier than frame_generator.xml, we would match on the KW_NO_HELP node and set an annotation in the frame in some way to mark that frame as a NO-HELP frame. This needs to be in a place that can be read back later when we need to know if we should bypass setting help for a field or not.

I will look for the right place for this.

Thank you Greg. I had a naive idea that as both setHelp and setNoHelp are emitted in the same @XXXDef.setup() method there is a single place where everyting is already known and tried to find a way to fix a setup method.

OK, I think I see a good way to do this.

Here is the implementation plan:

1. Please review the usage of set_use_text in frame_generator.xml. It is used on line 5924 of the main walk rules. You will see code like this:

         <rule>type == prog.kw_use_txt and parent.type == prog.frame_phrase
            <action>execLib("set_use_text")</action>
         </rule>

You can create a similar rule for kw_no_help. It can call a function named "set_no_help". The code can be at that same point in the walk rules because we just have to set the value before we call process_frame in the post-rules. Before the post-rules execute, all nodes in the tree will have been walked once by the walk rules. That means that we have already "scanned" all frame phrases by that time, so the setting will be known.

2. Look at the set_use_text function on line 5365. You will create a similar version called set_no_help. It should set a value in the frame map in a similar way that frameText is used. Call it frameNoHelp.

3. The frameNoHelp will need to be initialized the same way that frameText is initialized. See lines 387 and most importantly, you must add a line to new_frame just like is done on line 5254. It should set frameNoHelp to false (the default value).

4. Inside the post rules, process_frame is called. process_frame looks up the frame map with all the stored flags, widgets and so forth. It calls gen_code with that map. Near the bottom of gen_code, it calls gen_init_widget with the same map. Inside gen_init_widget on line 4357 is (I think) where the help string will be set for a non-browse widget that was defined from a field and which has not already had an explicit help string emitted (hlps.get(wname) == null). That is the place where you must avoid emitting the setHelp() call if frame.get(frameNoHelp) is true.

Greg Shah wrote:

OK, I think I see a good way to do this.

Here is the implementation plan:

1. Please review the usage of set_use_text in frame_generator.xml. It is used on line 5924 of the main walk rules. You will see code like this:

[...]

You can create a similar rule for kw_no_help. It can call a function named "set_no_help". The code can be at that same point in the walk rules because we just have to set the value before we call process_frame in the post-rules. Before the post-rules execute, all nodes in the tree will have been walked once by the walk rules. That means that we have already "scanned" all frame phrases by that time, so the setting will be known.

2. Look at the set_use_text function on line 5365. You will create a similar version called set_no_help. It should set a value in the frame map in a similar way that frameText is used. Call it frameNoHelp.

3. The frameNoHelp will need to be initialized the same way that frameText is initialized. See lines 387 and most importantly, you must add a line to new_frame just like is done on line 5254. It should set frameNoHelp to false (the default value).

4. Inside the post rules, process_frame is called. process_frame looks up the frame map with all the stored flags, widgets and so forth. It calls gen_code with that map. Near the bottom of gen_code, it calls gen_init_widget with the same map. Inside gen_init_widget on line 4357 is (I think) where the help string will be set for a non-browse widget that was defined from a field and which has not already had an explicit help string emitted (hlps.get(wname) == null). That is the place where you must avoid emitting the setHelp() call if frame.get(frameNoHelp) is true.

Thank you very much Greg!

1. Fixed conversion for the NO-HELP option.
2. Removed NO-HELP and NO-VALIDATE flags from the FrameConfig and fixed conversion accordingly.
Committed to the task branch 2038a revno 10974.
The test for NO-HELP conversion (uast/help/n-help.p) was added (revno 1417)

Code Review Task Branch 2038a Revision 10974

1. frame_scoping.rules and FrameInterface have no changes except for your history entry. Please remove the history entry so that no changes will be merged to trunk for those files.

2. As noted in a previous code review, when you are the first person to edit a file in your task branch, you should not be adding history entries to previous trunk history entries. This affects frame_generator.xml, java_templates.tpl, methods_attributes.rules, CommonFrame, LogicalTerminal and ThinClient.

3. Please do not just comment out the no-help and no-validate setters in frame_generator.xml. We don't need them there, right? Just delete it. Also please remove the extra space you inserted that breaks the column alignment for no-underline.

4. In methods_attributes.rules, the background attribute should be implemented in load_descriptors.

5. Please fix the assignment operator column alignment in line 314 of FrameConfig.

6. The history entries in GenericFrame should be 313 and 314 instead of 314 and 315.

Fixed issues mentioned in the code review.

Committed to the task branch 2038a revno 10975.

Code Review Task Branch 2038a Revision 10975

It looks good.

If I understand correctly, this work is complete. Correct?

I would like you to put this into conversion and runtime regression testing. For the conversion testing, make sure you have a reference build using trunk rev 10950. Then run the conversion with your 2038a rev 10975 and compare the results. If there are any differences at all, please report them here.

If there are no differences, then proceed to runtime testing.

Greg Shah wrote:

Code Review Task Branch 2038a Revision 10975

It looks good.

If I understand correctly, this work is complete. Correct?

I think so.

I would like you to put this into conversion and runtime regression testing. For the conversion testing, make sure you have a reference build using trunk rev 10950. Then run the conversion with your 2038a rev 10975 and compare the results. If there are any differences at all, please report them here.

If there are no differences, then proceed to runtime testing.

OK. The conversion for the 2038a is running now.

There are 3 expected differences in the generated code between 2038a and the trunk.

diff -r trunk/src/aero/timco/majic/ui/fa/FamstrFFamstr.java 2038a/src/aero/timco/majic/ui/fa/FamstrFFamstr.java
418d417
<          frame.setNoValidate(true);
diff -r trunk/src/aero/timco/majic/ui/so/AlertFrameA.java 2038a/src/aero/timco/majic/ui/so/AlertFrameA.java
43d42
<          frame.setNoHelp(true);
diff -r trunk/src/aero/timco/majic/ui/so/AlertFrameB.java 2038a/src/aero/timco/majic/ui/so/AlertFrameB.java
45d44
<          frame.setNoHelp(true);
62d60
<          crewCode.setHelp("Enter the code used to identify the crew");

Fair enough. Please do execute the runtime regression testing.

The status of the regression testing is the following:
1. All CTRL-C tests passed.
2. In the main regression the only test except tc_job_002 is tc_po_item_003 (and dependent tc_po_item_004). failure in step 19: 'timeout before the specific screen buffer became available (Mismatched data at line 20, column 51. Expected ' ' (0x0020 at relative Y 20, relative X 51) and found 'M' (0x004D at relative Y 20, relative X 51), template: screens/tc/po/item/003/po_item_003_step5.txt.)'.

There is an extra "Minimum Buy") label. I cannot understand so far how my changes can cause it. Investigating.

Please DO NOT investigate this issue. I sent an email a few weeks ago explaining that we have a regression in the trunk that causes tc_po_item_003 (and dependent tc_po_item_004) to fail. This is NOT a regression caused by 2038a.

Consequently, you should consider that your testing has passed. Please merge your revisions after 10950 into 2677a.

Greg Shah wrote:

Please DO NOT investigate this issue. I sent an email a few weeks ago explaining that we have a regression in the trunk that causes tc_po_item_003 (and dependent tc_po_item_004) to fail. This is NOT a regression caused by 2038a.

Consequently, you should consider that your testing has passed. Please merge your revisions after 10950 into 2677

Thank you Greg. Sorry, I forgot about that. More precisely I had in mind that there was I single additional test and decided that it was fixed and I have a regression.

I'm merging now.

The task branch has been merged to the trunk revno 10951

Actually, I had requested you merge into 2677a, not the trunk. In the future, please take more care with my instructions.

However, it is too late now. I will deal with the rebase in 2677a.

Please archive 2038a.

Greg Shah wrote:

Actually, I had requested you merge into 2677a, not the trunk. In the future, please take more care with my instructions.

However, it is too late now. I will deal with the rebase in 2677a.

I'm very sorry Greg.

Greg Shah wrote:

Please archive 2038a.

This is done already.

Igor Skornyakov wrote:

Added 3D tests for multiple widgets per frame, dialog-box and window (revno 1420).

1. I see no difference between frame and dialog-box appearance in 3D mode.
2. The WINDOW:THREE-D attribute doesn't affect neither FRAME nor DIALOG-BOX.
3. There is no much difference between standard and 3D look & feel: I can notice only the frame background color and (may be) the difference in the widget border which I cannot verbalize.

The border seems to differ for fill-in and editor. The difference is in the border being drawn with 3D lines vs simple lines I think.

Hynek Cihlar wrote:

The border seems to differ for fill-in and editor. The difference is in the border being drawn with 3D lines vs simple lines I think.

Unfortunatelly I have nothing clever to say on this regards as I've never did a low-level GUI programming. My main skills are at a server-side. However the border of the selection list looks very similr to one of the editor for me. The border of the rectangle with GROUP-BOX option also seems somehow different.

1. Please add fillin and text widgets that have the default bgcolor to your frame. In the customer application, it seems like the fill-ins have their background color changed to match the frame's background.

2. You report that "There is no much difference between standard and 3D look & feel: I can notice only the frame background color and (may be) the difference in the widget border which I cannot verbalize.". However, there is clearly a difference in the bgcolor for toggle-box and rectangle with group-box set.

3. Make sure you look at the drop-down for a combo-box to see if there are any differences.

4. Please consider that you are writing a specification that someone else must implement. Using a statement like "(may be) the difference in the widget border which I cannot verbalize" is not sufficient. Please examine the screenshots more carefully and explain exactly what is different. I recommend using a screen magnifier so that you can see at the pixel level.

5. You have not reported anything yet for SESSION:THREE-D. Make sure you are using SYSTEM-DIALOG-GET-FILE to see the difference, but also test your other window cases.

Greg Shah wrote:

1. Please add fillin and text widgets that have the default bgcolor to your frame. In the customer application, it seems like the fill-ins have their background color changed to match the frame's background.

2. You report that "There is no much difference between standard and 3D look & feel: I can notice only the frame background color and (may be) the difference in the widget border which I cannot verbalize.". However, there is clearly a difference in the bgcolor for toggle-box and rectangle with group-box set.

3. Make sure you look at the drop-down for a combo-box to see if there are any differences.

4. Please consider that you are writing a specification that someone else must implement. Using a statement like "(may be) the difference in the widget border which I cannot verbalize" is not sufficient. Please examine the screenshots more carefully and explain exactly what is different. I recommend using a screen magnifier so that you can see at the pixel level.

5. You have not reported anything yet for SESSION:THREE-D. Make sure you are using SYSTEM-DIALOG-GET-FILE to see the difference, but also test your other window cases.

OK. However I do not understand what you mean saying "test your other window cases". Please clarify.
Thank you.

However I do not understand what you mean saying "test your other window cases". Please clarify.

It is important to make sure that when you set SESSION:THREE-D, that it has no effect on the rendering of WINDOW, FRAME, DIALOG or other widgets. You already have other testcases that show WINDOW, FRAME, DIALOG and other widgets using WINDOW:THREE-D (true/false) and FRAME/DIALOG:THREE-D (true/false). Your current testing implicitly shows the results when SESSION:THREE-D false. These same cases should be tested with SESSION:THREE-D true to ensure that no differences are caused.

Greg Shah wrote:

It is important to make sure that when you set SESSION:THREE-D, that it has no effect on the rendering of WINDOW, FRAME, DIALOG or other widgets. You already have other testcases that show WINDOW, FRAME, DIALOG and other widgets using WINDOW:THREE-D (true/false) and FRAME/DIALOG:THREE-D (true/false). Your current testing implicitly shows the results when SESSION:THREE-D false. These same cases should be tested with SESSION:THREE-D true to ensure that no differences are caused.

Got it. Thank you.

Igor can you also check, whether there are any size differences between 3D and non-3D mode? I am particularly keen about knowing whether the 3D border (vs. a simple line border) adds to the widget's outer size.

Hynek Cihlar wrote:

Igor can you also check, whether there are any size differences between 3D and non-3D mode? I am particularly keen about knowing whether the 3D border (vs. a simple line border) adds to the widget's outer size.

Well, visually I cannot notice any difference. I will investigate deeper when I will analyze what is the 3D-border exactly. At this moment I'm working on SESSION:THREE-D.

Igor Skornyakov wrote:

Greg Shah wrote:

1. Please add fillin and text widgets that have the default bgcolor to your frame. In the customer application, it seems like the fill-ins have their background color changed to match the frame's background.

This is not the fact for the FILL-IN. The TEXT background is the same as for frame in both modes.

I haven't seen screenshots of the non-ENABLED case. I mentioned in note 244 that it was important to check this:

Finally, although your use of ENABLE ALL is definitely a good test, we will also need results for the non-enabled case. Just use a DISPLAY or VIEW statement to make the non-enabled frame visible.

I suspect that is where we will see some additional differences. As mentioned earlier, the customer application shows differences in FILL-IN background color that I think are probably due to the THREE-D attribute.

Greg Shah wrote:

Igor Skornyakov wrote:

Greg Shah wrote:

1. Please add fillin and text widgets that have the default bgcolor to your frame. In the customer application, it seems like the fill-ins have their background color changed to match the frame's background.

This is not the fact for the FILL-IN. The TEXT background is the same as for frame in both modes.

I haven't seen screenshots of the non-ENABLED case. I mentioned in note 244 that it was important to check this:

Finally, although your use of ENABLE ALL is definitely a good test, we will also need results for the non-enabled case. Just use a DISPLAY or VIEW statement to make the non-enabled frame visible.

I suspect that is where we will see some additional differences. As mentioned earlier, the customer application shows differences in FILL-IN background color that I think are probably due to the THREE-D attribute.

Sorry, I will do it right now.

It looks to me like there are significant differences there. I assume you are documenting them fully, here.

During my rebase of 2677a, I have found that you merged the wrong version of 2038a. You have merged 2038a revision 10974. But the final cleaned up version of 2038a was revision 10975. That is the version that passed testing. As far as I know, 10974 was never tested. I am applying those changes as part of my rebase. They will get merged when 2677a is merged to trunk. Do not do anything on this right now.

I am telling you this so that you take greater care in the future, with how you merge your changes.

Greg Shah wrote:

During my rebase of 2677a, I have found that you merged the wrong version of 2038a. You have merged 2038a revision 10974. But the final cleaned up version of 2038a was revision 10975. That is the version that passed testing. As far as I know, 10974 was never tested. I am applying those changes as part of my rebase. They will get merged when 2677a is merged to trunk. Do not do anything on this right now.

I am telling you this so that you take greater care in the future, with how you merge your changes.

I see. Sorry about that. I'll be more careful in the future.
In fact the 10975 differs from 10974 in history records (and two entries in methods_attributes.rules which where just obsolete). This is not an excuse just a comment.

Greg Shah wrote:

It looks to me like there are significant differences there. I assume you are documenting them fully, here.

Well, in the disabled EDITOR, COMBO-BOX RADIO-SET, SELECTION-LIST, FILL-IN, and TOGGLE-BOX the background in the 3D mode is the same as the one of the FRAME. In addition the disabled FILL-IN has no border.

Igor, please make sure the content of uast/3d is compilable by P2J, thanks.

Hynek Cihlar wrote:

Igor, please make sure the content of uast/3d is compilable by P2J, thanks.

Sorry Hynek. It is fixed now. See uast/three-d (revno 1423).

Task branch 2677a revision 11045 add runtime support for THREE-D.

Hynek Cihlar wrote:

Task branch 2677a revision 11045 add runtime support for THREE-D.

There are some known issues:

• FILL-IN widget doesn't draw its 3D border in at least one case, WIP.
• COMBO-BOX widget doesn't draw the 3D border fully. The cause is the inner layout of the widget.

Code Review Task Branch 2677a Revision 11045

The changes look good.

Did you try Igor's 3D testcases? Is everything working except for the two items noted above?

Greg Shah wrote:

Code Review Task Branch 2677a Revision 11045

The changes look good.

Did you try Igor's 3D testcases? Is everything working except for the two items noted above?

I only ran portion of the scripts, there were multiple runtime errors. As a quick workaround I created one-shot test scripts to test individual widgets and I also created tests to cover new cases. I'm putting all the tests together and making them run.

Hynek Cihlar wrote:

I only ran portion of the scripts, there were multiple runtime errors. As a quick workaround I created one-shot test scripts to test individual widgets and I also created tests to cover new cases. I'm putting all the tests together and making them run.

Hynek. What kind of errors you've experinced? I was able to run all the tests w/o problems. May be I've corrupted them after that.
Thank you.

Igor Skornyakov wrote:

Hynek Cihlar wrote:

I only ran portion of the scripts, there were multiple runtime errors. As a quick workaround I created one-shot test scripts to test individual widgets and I also created tests to cover new cases. I'm putting all the tests together and making them run.

Hynek. What kind of errors you've experinced? I was able to run all the tests w/o problems. May be I've corrupted them after that.
Thank you.

Aside from the obvious of unsupported slider I had some abends related to widget focusing. I didn't have chance to explore this further.

Hynek Cihlar wrote:

Aside from the obvious of unsupported slider I had some abends related to widget focusing. I didn't have chance to explore this further.

I see, thank you. Sorry - I understood that there where problems with 4GL. Of course I havent't try to run the converted code.

or try to understand what is wrong with the test?

No. I'm going to ask some customer 4GL developers to see if they have any ideas.

In regard to the problem discussed in notes 216 through 232, the following summarizes the discussions with the customer 4GL developer.

The short story: the lack of an ENABLE (implicit or explicit) for the given field means that its schema validation expression is not copied into the widget at compile time. We already mostly supported this in the past, but some problems were found and fixed as part of #2692. No further work in needed.

-------- Forwarded Message --------
Subject: Re: strange validation behavior
Date: Tue, 17 Nov 2015 13:25:13 -0500
From: Greg Shah ...
Reply-To: ...
To: Guy ...
CC: ...

Guy,

Yes, I think you are exactly right. Well done!

I think the compiler looks to see if a field is enabled (using the ENABLE statement, it seems). If it is, then the schema validation is baked into the program (as we expected). If it isn't, then it doesn't bother.

I should have seen this myself. This is one of the core findings of Eric's work in #2692.

Only ENABLE, SET, PROMPT-FOR, UPDATE and INSERT have this behavior (of causing the schema validation expression to be copied if there is no explicit validation expression in the source code). SET, PROMPT-FOR, UPDATE and INSERT all have an implicit ENABLE hidden inside.

As you note, SENSITIVE=TRUE does not have the same behavior as ENABLE in this regard.

Would you mind double-checking this theory? - I've just figured it at the end of the day, I think it's right. If you agree,

I agree.

I'm not sure what the best way forward is. I guess you could do the same awful logic - look to see if the field could be ENABLEd in the code, and then include the schema validation if it is?

We have long since had functionality that only copies schema validations for fields present in ENABLE, SET, PROMPT-FOR, UPDATE and INSERT. Our implementation was flawed in 2 ways:

- As Eric also found in #2692, the schema validations must be parsed in the context of the business logic at the point at which each ENABLE, SET, PROMPT-FOR, UPDATE and INSERT exists that references a field that has a schema validation expression.

- ENABLE ALL was not being handled properly (we had to keep track of all widgets in the frame that were defined based on a field so that at the ENABLE ALL we could copy out the associated schema validation expression and parse it).

Yak.

Yep. Eric is just now finishing the work to implement the improved version. It will be checked in this week sometime. As you note, it is nasty.

On a "positive" note, we already implemented this so there is no extra work for this "feature".

Thanks for helping with this mystery.

Greg

On 11/17/2015 01:08 PM, Guy wrote:

Hi Greg,

I "think" I've cracked it - thus explaining the inconsistencies.

I think the compiler looks to see if a field is enabled (using the ENABLE statement, it seems). If it is, then the schema validation is baked into the program (as we expected). If it isn't, then it doesn't bother.

With the explicit form VALIDATE phrase, the validation is baked in regardless of whether the field is enabled or not.

So, with your examples, if you make num1 enabled, using SENSITIVE = TRUE, you will find the schema validation passes!

I guess Progress is just trying to be clever - by not baking in code that it thinks it doesn't need. However I think it's a bug, since if you use SENSITIVE = TRUE, it should then validate.

Would you mind double-checking this theory? - I've just figured it at the end of the day, I think it's right. If you agree, I'm not sure what the best way forward is. I guess you could do the same awful logic - look to see if the field could be ENABLEd in the code, and then include the schema validation if it is? Yak.

Regards,
Guy

On 16 November 2015 at 21:49, Greg Shah wrote:

Guy,

I am reattaching the original testcases. It is very hard for me to read your code because the appbuilder has filled it with defines and broken everything up into little pieces. So, I will reference the original code here. If you can tell me where we have gone wrong or misunderstood, that will be great.

If you compare the two files, you will see they only differ in two places:

diff validate_method_in_enabled_frame_with_db.p validate_method_in_enabled_frame_with_db_and_explicit_rule.p

12c12
< test.num1 /*validate(test.num1 > 10, "num1 must > 10" )*/
---

test.num1 validate(num1 > 10, "num1 must > 10" )

36c36
< /* test.num1:validate('ENABLED-FIELDS') yes test.num1:validate() yes num1 = 0 */
---

/* test.num1:validate('ENABLED-FIELDS') yes test.num1:validate() no num1 = 0 */

Difference 1: The validate_method_in_enabled_frame_with_db.p has no explicit validation expression for test.num1 and in validate_method_in_enabled_frame_with_db_and_explicit_rule.p there is an explicit validation expression.

Difference 2: our comments that describe the output of the message statements. This is not a functional difference, but it does in fact record the unexplainable behavior we have found.

The problem here is not with the use of "ENABLED-FIELDS". The message statements that show the difference are as follows.

validate_method_in_enabled_frame_with_db.p line 36:

/* test.num1:validate('ENABLED-FIELDS') yes test.num1:validate() yes num1 = 0 */
message "test.num1:validate('ENABLED-FIELDS')" h1:validate("ENABLED-FIELDS") "test.num1:validate()" h1:validate() "num1 = " h1:screen-value .

validate_method_in_enabled_frame_with_db_and_explicit_rule.p line 36:

/* test.num1:validate('ENABLED-FIELDS') yes test.num1:validate() no num1 = 0 */
message "test.num1:validate('ENABLED-FIELDS')" h1:validate("ENABLED-FIELDS") "test.num1:validate()" h1:validate() "num1 = " h1:screen-value .

The message statements in both cases are identical. Only the comments differ. In both cases we are outputting the results of both validate("ENABLED-FIELDS") and validate(). The ENABLED-FIELDS results are consistent and understandable. The issue is with the VALIDATE case. Both forms of the method are being executed against the handle for the test.num1 field. In both cases, the screen-value of test.num1 is 0 which would fail validation.

The only "code" difference is in one case test.num1 as a schema-derived validation expression and in the other case it does not. The thing we can't explain is that the validate() call yields YES when there is a schema-derived validation expression and yields NO for the same expression set using explicit validation.

This suggests that if there is a schema-derived validation expression, it is never executed in the VALIDATE case, while the explicit validation expression from the source code is executed.

Thanks,
Greg

On 11/13/2015 09:52 AM, Guy wrote:

​Hi Greg,

Although I thought I had replicated the issue when I emailed you last week, when I came to create my own example, I can't reproduce it.

What I hadn't noticed, was that you were using VALIDATE within your original code (I had assumed you were just using VALIDATE.

If you use VALIDATE on a disabled field, then it will return TRUE even if the SCREEN-VALUE is invalid. This seems to be the case, regardless of whether it's from the schema or not.

So, is it not just the case, that you were disabling field with schema validation, but not the field with explicit validation?

In my tests, if I disabled the explicit validation, and use VALIDATE, this too returns TRUE.

I've attached my test window. It's been created using the AppBuilder, but I added explicit validation via an additional FORM statement in the main block.

Regards,
Guy

On 4 November 2015 at 18:28, Greg Shah wrote:

Guy,

I am surprised at this. I would have thought that the schema validation code would have "magically" compiled into the same r-code as if a VALIDATE had been explicitly defined. Then the behaviour would have been exactly the same.

This was our expectation and reaction too.

One clue might be that there is a VALIDATE option - which only validates enabled fields [not surprisingly]. So maybe the bug/feature is that if there is no explicit validation, the VALIDATE that is called is somehow a VALIDATE call (or accidentally calls this flavour).

Yes, I suspect you are right. This only occurs in the following case:

- the validate() method is being called directly on a field-level widget
- the field level widget is NOT enabled
- the validation expression was copied from the schema

That no-parameter version is supposed to actually execute the validation expression in that case, but it seems like they somehow bypass that behavior and treat it like the "enabled-fields" case.

I would say this is a bug though, because the Progress help states: " If this option [ENABLED-FIELDS] does not appear, the VALIDATE method validates all fields, whether enabled or not.

Yes, I agree it seems to be a bug.

Hmm. I guess this scuppers my suggestion that implicit schema validation could be preprocessed into the source code? - i.e. to turn it into explicit VALIDATE statements?

No, your assessment was correct. Eric has written testcases that show that the schema validation expressions can directly reference variables or other code and resources that only exists in the business logic. He is working (in task #2692) to push our copying logic back into the parsing stage where we will properly resolve these business logic references.

However you could preprocess it to VALIDATE...

My original idea was to simply remember that the source of the validation expression was the schema and to "honor" the bug in our runtime implementation of the validate() method. Although your idea has some promise, it would leave that bug hard coded into your converted application. In the future, should the bug be fixed in Progress, we could fix it (or conditionally fix it when we act like a specific OpenEdge version).

Do you want me to collar a Progress engineer whilst I'm here (there are plenty about), with this one? Or I can raise a case with Progress?

Either one sounds good. Feel free to share the sample code and database with them as needed.

Thanks,
Greg

On 11/04/2015 12:47 PM, Guy wrote:

Hi Greg,

I am surprised at this. I would have thought that the schema validation code would have "magically" compiled into the same r-code as if a VALIDATE had been explicitly defined. Then the behaviour would have been exactly the same.

One clue might be that there is a VALIDATE option - which only validates enabled fields [not surprisingly]. So maybe the bug/feature is that if there is no explicit validation, the VALIDATE that is called is somehow a VALIDATE call (or accidentally calls this flavour). I would say this is a bug though, because the Progress help states: " If this option [ENABLED-FIELDS] does not appear, the VALIDATE method validates all fields, whether enabled or not."

Hmm. I guess this scuppers my suggestion that implicit schema validation could be preprocessed into the source code? - i.e. to turn it into explicit VALIDATE statements? However you could preprocess it to VALIDATE...

Do you want me to collar a Progress engineer whilst I'm here (there are plenty about), with this one? Or I can raise a case with Progress?

Regards,
Guy

On 3 November 2015 at 22:05, Greg Shah wrote:

Alex/Scott,

In our work to get validation processing finished, we have found a strange behavior in the VALIDATE method, which we cannot explain. Before we blindly implement it, I was hoping one of you might take a look to see if there is an explanation that we are missing.

Consider the attached two testcases. These programs are the same, except for a frame definition.

validate_method_in_enabled_frame_with_db.p:

DEF FRAME f
test.num validate(test.num > 0, "num must be positive")
test.num1 /*validate(test.num1 > 10, "num1 must > 10" )*/
WITH NO-AUTO-VALIDATE.

validate_method_in_enabled_frame_with_db_and_explicit_rule.p:

DEF FRAME f
test.num validate(test.num > 0, "num must be positive")
test.num1 validate(num1 > 10, "num1 must > 10" )
WITH NO-AUTO-VALIDATE.

In the first, the validation expression is commented out and comes from the schema (see bogus.zip). In the schema, the test.num1 has the validation expression set as num1 > 10.

Later in the code the test.num widget is enabled in the frame, while test.num1 is left disabled. The values for both test.num and test.num1 are set to 0, which will fail any validation that occurs. The VALIDATE method is then called on the test.num1 widget. The results differ. When the validation expression for the disabled field is copied from the schema, VALIDATE returns true. When the same validation expression on the disabled field is provided explicitly in the source code, the VALIDATE method returns false (which is expected).

We can certainly implement this "feature" for disabled widgets but it seems quite arbitrary. I can't see any utility that Progress may have been trying to implement. Perhaps it is a bug. Or perhaps we are somehow missing something.

Thanks in advance for any insight.

Greg

Please note that the diff attached with note 314 was also posted to #2876 as it also contains fixes related to #2876, these fixes are a prerequisite for the FILL-IN border fixes.

Hynek Cihlar wrote:

The attached diff improves drawing of FILL-IN border. It utilizes the Keep3DFillinBorder environment variable, when true the FILL-IN border is drawn even when the widget is not enabled.

The change was checked in to task branch 2677b revision 10955.

Rebased task branch 2038b from P2J trunk revision 10964 (new branch revision 10966).

In progress issues:

1. Improve runtime support for BOX attribute for DIALOG-BOX.
This is the cause of the unexpected ALERT-BOX mentioned in #2795 note 58.

2. Finish TOP-ONLY attribute runtime support for frame widget.

3. Runtime support for frame VIRTUAL* attributes.
Lack of proper VIRTUAL* support (and SCROLLABLE attribute which has been resolved in 2038b) causes the unexpected scroll bars in User Defined Alerts mentioned in #2795 note 58.

In terms of estimate, 1+2 1 MD remaining. 3 is 2 MD remaining, however I am not sure yet how much of the frame layout logic is affected, this may increase the estimate.

What is the level of risk?

Greg Shah wrote:

What is the level of risk?

I think the risk is at low to moderate level.

Is it a reasonable expectation to have this checked in by Friday evening?

Greg Shah wrote:

Is it a reasonable expectation to have this checked in by Friday evening?

Yes.

I've been attempting to come up with a working FRAME:TOP-ONLY test case. However I had no luck in getting any visible effect of different values of the attribute, whether in ChUI or GUI.

Here is a sample code where I get the same behavior regardless of ChUI/GUI, the TOP-ONLY attribute value, DEFAULT-WINDOW:KEEP-FRAME-Z-ORDER or OVERLAY values. In all the cases I can move frame 1 on top of frame 2 and frame 2 on top of frame 1. I would expect that the TOP-ONLY attribute would prevent me from overlaying the frame with another frame missing TOP-ONLY.

DEFAULT-WINDOW:KEEP-FRAME-Z-ORDER = FALSE.

DISPLAY "Hello world" with frame f1 TITLE "f1" TOP-ONLY.
DISPLAY "Hello world 2" with frame f2 TITLE "f2".

FRAME f1:OVERLAY = TRUE. 
FRAME f2:OVERLAY = TRUE. 

MESSAGE "f1:OVERLAY:" FRAME f1:OVERLAY "TOP-ONLY:" FRAME f1:TOP-ONLY.
MESSAGE "f2:OVERLAY:" FRAME f2:OVERLAY "TOP-ONLY:" FRAME f2:TOP-ONLY.

MESSAGE "Let's go!".
PAUSE.

FRAME f2:ROW = 2.
MESSAGE "after FRAME f2:ROW = 2". 
PAUSE.

FRAME f1:MOVE-TO-TOP().
MESSAGE "after f1:MOVE-TO-TOP()". 
PAUSE.

FRAME f2:MOVE-TO-TOP().
MESSAGE "after f2:MOVE-TO-TOP()". 
PAUSE.

Greg/Constantin, any ideas?

DEFAULT-WINDOW:KEEP-FRAME-Z-ORDER = FALSE.

DISPLAY "Hello world" with OVERLAY frame f1 TITLE "f1" TOP-ONLY SIZE 20 BY 20 .
DISPLAY "Hello world 2" with OVERLAY frame f2 TITLE "f2" SIZE 40 BY 20 ROW 2.

MESSAGE "f1:OVERLAY:" FRAME f1:OVERLAY "TOP-ONLY:" FRAME f1:TOP-ONLY.
MESSAGE "f2:OVERLAY:" FRAME f2:OVERLAY "TOP-ONLY:" FRAME f2:TOP-ONLY.
VIEW FRAME f1.
VIEW FRAME f2.

ON '1':U ANYWHERE 
DO:
   VIEW FRAME f1.
   RETURN.
END.

ON '2':U ANYWHERE
DO:
   VIEW FRAME f2.
   RETURN.
END.

WAIT-FOR CLOSE OF CURRENT-WINDOW.

1. f1 and f2 do not have the TOP-ONLY attribute: when pressing 1 and 2 triggers, both f1 and f2 remain visible, they just change their z-order
2. both f1 and f2 are set TOP-ONLY: when pressing 1 and 2 triggers, only one of the frame remains visible (the other is hidden).
3. only f1 or f2 has TOP-ONLY: when pressing 1 and 2 triggers, the frame with the TOP-ONLY gets hidden (so, if f1 has TOP-ONLY, when pressing 2, f2 is displayed and f1 is hidden).

1. f1 and f2 do not have the TOP-ONLY attribute: 1/2 triggers do not change z-order
2. both have TOP-ONLY attribute: pressing 1 hides f2 and viceversa (same behaviour as KEEP-FRAME-Z-ORDER = FALSE).
3. only f1 or f2 has TOP-ONLY: the one with TOP-ONLY remains on top, when using 1/2 triggers (same behaviour as KEEP-FRAME-Z-ORDER = FALSE)

In your case, you were using move-to-top() - I'm assuming this (and move-to-bottom()) do not use the TOP-ONLY attribute.

Constantin Asofiei wrote:

Hynek, see this test:

Constantin, very helpful. Thanks.

Rebased task branch 2038b from P2J trunk revision 10966 (new branch revision 10972).

Hynek, about this comment in FrameConfig:

    * TODO: The default is true only when frame is dynamic, the default for
    *       static frame is false.

def var i as int.
form i with frame f1.
message frame f1:scrollable. /* no */
frame f1:width-chars = 100.
message frame f1:scrollable. /* no */

update i with frame f1.

message frame f1:scrollable.  /* yes */

Constantin Asofiei wrote:

Hynek, about this comment in FrameConfig:
[...]
As you probably noticed (after seeing the other code changes in 2038b), frame:scrollable for static frames can be yes, also. This depends on the frame's SIZE option, because if this is missing, then it defaults to no. Also, if SIZE is missing, this stays on no until the frame is realized:
[...]

Constantin, thanks for pointing this out. This an interesting case by the way. The state of scrollable depends on the size value. When the size is smaller/equal than max visible size (terminal window for example) then scrollable is set to no, assigned visible size is corrected accordingly.

def var i as int.
form i with frame f1.
message frame f1:scrollable. /* no /
frame f1:width-chars = 100.
message frame f1:scrollable. / no */

update i with frame f1.

message frame f1:scrollable. /* yes /
message frame f1:width frame f1:virtual-width. / 80 100 for terminal window of width 80 */

def var i as int.
form i with frame f1.
message frame f1:scrollable. /* no /
frame f1:width-chars = 20.
message frame f1:scrollable. / no */

update i with frame f1.

message frame f1:scrollable. /* no /
message frame f1:width frame f1:virtual-width. / 20 20 */

Hynek, another interesting fact is that if you move the frame from a smaller window (where the frame is scrollable) to a larger window (where it should fit fully), then the frame's scrollable state is preserved. I guess scrollable state is resolved when frame is realized and maintained forever, even if the frame is resized via its attributes, later on.

Code Review Task Branch 2038b Revision 10978

I'm fine with the changes so far.

Constantin Asofiei wrote:

I guess scrollable state is resolved when frame is realized and maintained forever, even if the frame is resized via its attributes, later on.

Yes, it seems that a frame keeps its layout and actual scrollable state even when its visible size is changed to be equal to its virtual size and even when SCROLLABLE set to false.

def var i as int.
form i with frame f1.
message frame f1:scrollable. /* no */
frame f1:width-chars = 300.
message frame f1:scrollable. /* no */

update i with frame f1.

message frame f1:scrollable. /* yes */
message frame f1:width frame f1:virtual-width. /* 80 100 for terminal window of width 80 */

frame f1:virtual-width  = 20.
frame f1:scrollable = false.

message frame f1:scrollable. /* no */
message frame f1:width frame f1:virtual-width. /* 20 20 */

/* scrollable is false, virtual size equal to visible size, but the frame has horizontal scrollbar visible */

The actual scrollable state (whether scroll bars are visible or not) is determined based on the virtual and visible size. This doesn't seem to be correct, instead we should probably use the virtual size calculated during initial layout.

Hynek Cihlar wrote:

The actual scrollable state (whether scroll bars are visible or not) is determined based on the virtual and visible size. This doesn't seem to be correct, instead we should probably use the virtual size calculated during initial layout.

And this is different depending on the UI type. In ChUI the scroll-bar is visible but not in GUI.

Code Review Task branch 2038b Revision 10979

The changes are functionally fine. Please do the final cleanups (e.g. removing log4j and extra logging) and history entries ASAP.

Constantin, is it expected for the Frame.verticalStep to be equal to the size of base unit when the frame has no widgets? See Frame.calcFixedRect() where verticalStep is calculated.

I have a frame that is initially laid out with no widgets, then widgets are added. The non-zero height however is causing some unexpected behavior, frame shows scroll bars and attempts to make the added widgets visible which causes scrolling.

Please review 2038b revision 10983. The branch adds or improves runtime support of SCROLLABLE, VIRTUAL-*, SCROLL-BARS and TOP-ONLY attributes, and also improves related frame layout logic.

The branch is rebased against the latest trunk, revision 10969.

There is one known unresolved issue, see note 334. Constantin, there is also a TODO with your name in the code, please have a look.

ChUI regression testing is in progress.

Hynek Cihlar wrote:

ChUI regression testing is in progress.

The test run finished with regressions.

Code Review Task branch 2038b Revision 10983

I'm fine with the changes.

How bad are the regressions?

Greg Shah wrote:

Code Review Task branch 2038b Revision 10983

I'm fine with the changes.

How bad are the regressions?

There are several occurrences of an incorrectly calculated frame virtual size causing scroll bar not shown (although the items are laid out ok). I am not yet sure whether this is a single or multiple bugs.

Hynek Cihlar wrote:

Constantin, is it expected for the Frame.verticalStep to be equal to the size of base unit when the frame has no widgets? See Frame.calcFixedRect() where verticalStep is calculated.

I have a frame that is initially laid out with no widgets, then widgets are added. The non-zero height however is causing some unexpected behavior, frame shows scroll bars and attempts to make the added widgets visible which causes scrolling.

You are talking about a dynamic frame, correct? In case of dynamic frames, there is no concept of "down frame" or "down body". Do you have an example which shows the issue?

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Constantin, is it expected for the Frame.verticalStep to be equal to the size of base unit when the frame has no widgets? See Frame.calcFixedRect() where verticalStep is calculated.

I have a frame that is initially laid out with no widgets, then widgets are added. The non-zero height however is causing some unexpected behavior, frame shows scroll bars and attempts to make the added widgets visible which causes scrolling.

You are talking about a dynamic frame, correct? In case of dynamic frames, there is no concept of "down frame" or "down body". Do you have an example which shows the issue?

The frame's config has dynamic set, so I assume this is a dynamic frame. Still verticalStep is calculated and it is actively used during layout processing in this case. And since the effective frame's scroll state doesn't depend on SCROLLABLE but on the actual inner and outer sizes (which also depend on verticalStep), the scroll state is negatively affected.

ChUI regression tests for 2038b restarted.

Ctrl-C part passed, Main part has failed. Overall a great improvement from the previous ChUI regression test run. There are total of 30 failing tests.

After the last ChUI regression test run 2038b has three unresolved issues:

• wrong report page size, this is a confirmed regression
• frame layout logic in some cases calculates a wrong virtual size, this is a latent issue made apparent by the changes in 2038b and manifests as frame scroll bars made visible when they should not
• the unexpected verticalStep from note 334

I will continue tomorrow.

2038b revision 10990 contains regression fixes with one yet unresolved issue, the unexpected verticalStep from note 334.

Please review or wait for the unexpected verticalStep from note 334 to be resolved.

Code Review Task branch 2038b Revision 10990

I'm fine with the changes.

I will review again when the last fix is in.

Constantin: Any concerns?

Hynek, about the question in Frame.setOuterSize(): the updateContentWidth call is no longer needed, as you've changed the logic so that the content pane (a SensitiveScrollContainer) sets its dimension when the virtual size attributes are changed.

If you are not aware yet, see test popup_ext.p - with your changes, borders are being drawn. Is this related to verticalStep issue in note 334?

Also, from some reason, with 2038b, the WidgeBrowser no longer identifies correctly (for frames) the Viewport child (a SensitiveScrollContainer instance). It appears as <null> <> instead of SensitiveScrollContainer<null> <>.

If you are not aware yet, see test popup_ext.p - with your changes, borders are being drawn. Is this related to verticalStep issue in note 334?

I meant scrollbars for the frame, not borders.

Constantin Asofiei wrote:

Hynek, about the question in Frame.setOuterSize(): the updateContentWidth call is no longer needed, as you've changed the logic so that the content pane (a SensitiveScrollContainer) sets its dimension when the virtual size attributes are changed.

Good, I will remove the unused updateContentWidth().

If you are not aware yet, see test popup_ext.p - with your changes, borders are being drawn. Is this related to verticalStep issue in note 334?

It is likely, I will test it when the verticalStep is resolved.

Also, from some reason, with 2038b, the WidgeBrowser no longer identifies correctly (for frames) the Viewport child (a SensitiveScrollContainer instance). It appears as <null> <> instead of SensitiveScrollContainer<null> <>.

Yes, this is because the SensitiveScrollContainer is inherited in Frame in a form of anonymous class. I will fix WidgetBrowser to properly handle names of anonymous classes.

Please review 2038b revision 10994. This resolves all known issues plus some minor improvements to Widget Browser.

Constantin Asofiei wrote:

If you are not aware yet, see test popup_ext.p - with your changes, borders are being drawn. Is this related to verticalStep issue in note 334?

This is resolved as of 2038b revision 10994. The issue was not related to the verticalStep.

Hynek Cihlar wrote:

Please review 2038b revision 10994. This resolves all known issues plus some minor improvements to Widget Browser.

Forgot to mention that ChUI regression test is in progress.

Hynek Cihlar wrote:

Hynek Cihlar wrote:

Please review 2038b revision 10994. This resolves all known issues plus some minor improvements to Widget Browser.

Forgot to mention that ChUI regression test is in progress.

The regression test failed with unrelated technical errors. Restarted.

The ChUI regression test run ended with a regression, an NPE in FillIn.draw(). This was fixed in 2038b revision 10995 and ChUI regression test restarted.

Constantin, please review.

Hynek Cihlar wrote:

Constantin, please review.

I'm OK with the changes. I assume some cleanup is still needed, as some history entries are missing.

Have you checked the GUI project screens?

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Constantin, please review.

I'm OK with the changes. I assume some cleanup is still needed, as some history entries are missing.

I will add the missing entries, otherwise no cleanup should be needed.

Have you checked the GUI project screens?

Yes, I didn't find any regressions there. What is the official test scenario that should work?

Hynek Cihlar wrote:

Yes, I didn't find any regressions there. What is the official test scenario that should work?

• the User Defined Alerts window should not have scrollbars
• the "Unable to set WIDTH of the DIALOG-BOX ... (4070)" error should not appear (it appears before the User Defined Alerts window is shown)

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Yes, I didn't find any regressions there. What is the official test scenario that should work?

All tabs in test 454 should work OK, plus check the following existing problems:

• the User Defined Alerts window should not have scrollbars
• the "Unable to set WIDTH of the DIALOG-BOX ... (4070)" error should not appear (it appears before the User Defined Alerts window is shown)

All work OK.

Hynek Cihlar wrote:

All work OK.

Great!

Have you checked the standalone GUI tests?

Constantin Asofiei wrote:

Hynek Cihlar wrote:

All work OK.

Great!

Have you checked the standalone GUI tests?

Yes, but I am rerunning them to cover the last change. Also, while Ctrl-C has passed, the main part has already rendered some delay. I think there is still something going on there.

2038b revision 10999 passes ChUI regression tests, GUI regression tests and the supported customer GUI demo screens. The revision also adds missing file header entries.

Please review.

1. Frame.calculateWidth - line 1619 has some commented code, remove it if is no longer needed.
2. Frame:4110 - same, remove the commented code
3. FrameGuiImpl.getMaxFrameSize and Frame.getMaxFrameSize - if the frame is attached to another frame (i.e. is an inner frame), I think this should return the virtual dimension of the parent frame. This can be worked in another branch.

Constantin Asofiei wrote:

Hynek, about 2038b revision 10999:

1. Frame.calculateWidth - line 1619 has some commented code, remove it if is no longer needed.
2. Frame:4110 - same, remove the commented code

Fixed in revision 11000.

1. FrameGuiImpl.getMaxFrameSize and Frame.getMaxFrameSize - if the frame is attached to another frame (i.e. is an inner frame), I think this should return the virtual dimension of the parent frame. This can be worked in another branch.

Ok.

Are you ok with merging 2038b revision 11000 to trunk?

Hynek Cihlar wrote:

Are you ok with merging 2038b revision 11000 to trunk?

Yes, go ahead and merge to trunk.

Task branch 2038b was merged to trunk as revision 10971.

It resolves the runtime support for FRAME/WINDOW attributes:
SCROLLABLE
VIRTUAL-*
SCROLL-BARS
TOP-ONLY.

Unfinished task is the inner frame layout in respect to the parent's virtual size. See the method Frame.getMaxFrameSize() which should most likely return the virtual size of its parent frame or window, but this must be confirmed with additional tests.

Created task branch 2038c from trunk 10982.

Rebased task branch 2038c from P2J trunk revision 10983 (new branch revision 10984).

Rebased task branch 2038c from P2J trunk revision 10984 (new branch revision 10986).

2038c revision 10990 contains final changes for improvements of runtime support of frame VIRTUAL* attributes and related changes/fixes. The unfinished task mentioned in note 364 has been also resolved.

The revision has passed GUI regression testing. ChUI regression testing is currently in progress. Refs #2038.

Please review.

2038c rev 10990 has passed ChUI regression testing.

Hynek, I'm OK with the changes in 10990. You can merge to trunk.

BTW, are all the tabs working OK in test 454 (upper and lower regions), in both Swing and Web clients?

Constantin Asofiei wrote:

Hynek, I'm OK with the changes in 10990. You can merge to trunk.

BTW, are all the tabs working OK in test 454 (upper and lower regions), in both Swing and Web clients?

I had tested Swing and Web and the tabs work OK in 454, both upper and lower regions.

Task branch 2038c was merged to trunk as revision 10985 and archived.