FWD » Core Development » Base Language

The ges_upd20130302b.zip passed conversion testing and checked in as bzr revision 10233.

Suggestion at this time.

Yes the approach #2 above looks preferable. However I see the discussion points here. The existed map that theoretically can be used now is apisToAttrOrMthd in the handle.java class. But:
1. We probably need reverse map like attributeName -> API call.
2. We need probably two maps, one for setter and one for getter API or to have the map like apisToAttrOrMthd where we can store setAttr... and getAttr... for single attribute. So we can use semantic analysis to check if the CAN-QUERY is possible(the API method begins with "get" prefix) or CAN-SET is possible(the API method begins with "set" prefix).
3. Every widget has its own set of the attributes available to query and|or set. So we need such map for every widget type. Then after resolving handle to widget(because the first parameter of the CAN-QUERY and CAN-SET is handle) it will be possible to find out if the attribute is available to set or query.

After some additional thought, I have come up with a 3rd option that I think is even better. Consider these annotation definitions:

package com.goldencode.p2j.util;

import java.lang.annotation.*;

/**
 * Annotation definition to mark methods as the replacements for Progress 4GL attributes. This
 * annotation stores the legacy resource type, attribute name and a flag to note if this is a getter
 * or setter.
 */
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface LegacyAttribute
{
   String resource();
   String name();
   boolean setter() default false;
}

package com.goldencode.p2j.util;

import java.lang.annotation.*;

/**
 * Annotation definition to mark methods as the replacements for Progress 4GL methods. This
 * annotation stores the legacy resource type and the method name.
 */
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface LegacyMethod
{
   String resource();
   String name();
}

With these defined, we could edit our code and annotate it like this (from FileSystemOps):

@LegacyAttribute(resource = "FILE-INFO", name = "TYPE") // this is the getter for FILE-INFO:TYPE
public static character fileInfoGetType()
{
...

@LegacyAttribute(resource = "FILE-INFO", name = "FILE-NAME", setter = true) // this is the setter for FILE-INFO:FILE-NAME
public static void initFileInfo(String name)
{
...

@LegacyAttribute(resource = "FILE-INFO", name = "FILE-NAME") // we could write "setter = false" here too, this is the getter for FILE-INFO:FILE-NAME
public static character getFileName()
{
...

or like this for methods (in ProcedureManager):

@LegacyMethod(resource = "SESSION", name = "ADD-SUPER-PROCEDURE")
public static logical addSuperProcedure(handle h)
{
...

Then at runtime, we can use the Class and Method instances (reflection APIs) to access the annotations and implement the CAN-SET/CAN-QUERY.

OK. This is very clean approach and we can implement incrementally only what we currently need. The question I have is what will be the cost of this solution in term of application performance? The CAN-SET/CAN-QUERY will have to obtain method instance and enumerate all annotation for given instance every time this call is invoking. Do you think we do not need to warn about slow down the performance?

The question I have is what will be the cost of this solution in term of application performance?

Yes, it is slower than using maps.

But these are features that are VERY RARELY used in code. I don't anticipate that the slower implementation will be a problem. If it turns out that it is an issue, we will address it later.

OK understood. Do we have the list of attributes we need to implement this way for now? Only "FRAME" and "FILE-TYPE" as it was in current handle.java class? Or may be everything we currently support?

Please implement the annotations for all currently supported methods and attributes. This way, it will just be a standard part of adding a new method or attribute. Please go through the rules/convert/methods_attributes.rules to find each one. Please note that there can often be multiple attribute setter signatures AND multiple methods backing a 4GL method (since the 4GL method may have optional parameters and even the non-optional ones may need to have variants for Java primitives and the P2J wrappers).

The DOM attributes/methods are currently being heavily modified, so ignore them for now.

Constantin: is this design sufficient to support the needs of #2123 and the LIST-QUERY-ATTRS/LIST-SET-ATTRS? Do you see any usefulness in this data for #1984 or for processing of readOnlyError()?

Constantin: is this design sufficient to support the needs of #2123 and the LIST-QUERY-ATTRS/LIST-SET-ATTRS?

Yes, this will solve both #2123 (this has access to the API being invoked) and LIST-QUERY-ATTRS/LIST-SET-ATTRS (here we should look in entire interface hierarchy, and return only the methods which have the appropriate annotations).

Do you see any usefulness in this data for #1984 or for processing of readOnlyError()?

readOnlyError() is something which is emitted at conversion time. #1984 problem is a conversion problem too (that case needs to convert to a readOnlyError() call also). But, I think we can use these annotations at conversion time too, to get rid of the read_only_attribute function in common-progress.rules. For each attribute/method we will specify its interface too, beside the associated method name (sometimes the hwrap var is the method name, sometimes not, so we can't use this). Having the interface and method name, we can than use reflection to determine if the 4GL attr/method is read-only or not (we can cache the read-only attrs as they are being computed, as reflection access is expensive). And thinking about it: we could build this cache upfront, as the HandleCommon interface hierarchy has all implemented attributes/methods; once this is built, is just a matter of querying a map for details about that attribute/method, and we don't need to maintain the interface name in methods_attributes.rules; I think this solution is much easier to put in place. Also, this map can be used at runtime too.

For each attribute/method we will specify its interface too, beside the associated method name (sometimes the hwrap var is the method name, sometimes not, so we can't use this).

What do you mean by "we will specify its interface too"? Where would this be specified?

Greg Shah wrote:

For each attribute/method we will specify its interface too, beside the associated method name (sometimes the hwrap var is the method name, sometimes not, so we can't use this).

What do you mean by "we will specify its interface too"? Where would this be specified?

The initial idea was to specify the interface name in the methods_attributes.rules, but this is not needed if we build the upfront map described in the second solution.

Constantin: If I understand correctly, no changes are needed to the annotations design to support these other use cases. Correct?

Greg Shah wrote:

Constantin: If I understand correctly, no changes are needed to the annotations design to support these other use cases. Correct?

The only change is that we need to add a LegacyAttribute.readonly field, to be able to mark the attributes as read-only (and as I think the writeable set of attributes is smaller, we can use a LegacyAttribute.writeable field instead).

Please document the change you propose (show the LegacyAttribute annotation definition and examples of use).

How the LegacyAttribute annotation should look:

package com.goldencode.p2j.util;

import java.lang.annotation.*;

/**
 * Annotation definition to mark methods as the replacements for Progress 4GL attributes. This
 * annotation stores the legacy resource type, attribute name, a flag to note if this is a getter
 * or setter and a flag to node if this attribute is writeable.
 */
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface LegacyAttribute
{
   String resource();
   String name();
   boolean setter() default false;
   boolean writeable() default false;
}

Example of writeable attribute PRIVATE-DATA:

@LegacyAttribute(resource = "common", name = "PRIVATE-DATA", setter = true, writeable = true)
public void setPrivateData(character data)
{

@LegacyAttribute(resource = "common", name = "PRIVATE-DATA", writeable = true)
public character getPrivateData()
{

And I keep forgetting but fortunately I reminded in time: attributes/methods can be shared by more than one resource type; the resource annotation should be optional, and not write code which depends on this annotation. If needed, the resource type should be determined at runtime (via the CommonHandle.getResourceType() API).

And thinking about the read-only case (yes, again): we can get this info out of the setter annotation; if for an attribute there is no associated java method marked with setter = true, than we can assume it is read-only. With this, the writeable annotation is no longer needed.

My conclusion is: to use the annotations, walk the up the interface tree starting with HandleCommon, collect all the info from the annotations and build a map with the details of each attribute/method. At runtime or conversion time, interrogate this map to find other details (beside the attribute name). For conversion time, the attribute/method name is known, and for server runtime, the attribute/method name must be determined from the Java Method's annotations. If more details are needed, check the built map.

OK, so we don't need to add the writable flag to the annotation.

And I keep forgetting but fortunately I reminded in time: attributes/methods can be shared by more than one resource type; the resource annotation should be optional, and not write code which depends on this annotation. If needed, the resource type should be determined at runtime (via the CommonHandle.getResourceType() API).

Do you mean that we should remove the "resource" field of LegacyAttribute/LegacyMethod?

Do you mean that we should remove the "resource" field of LegacyAttribute/LegacyMethod?

Yes, as it will only confuse the reader.

While I'm modifying the attribute related classes you can review canQuery/canSet implementation proposal(HandleOps.java).

...
   public static logical canQuery(handle h, String attr)
   {
      Method[] methods = h.get().getClass().getMethods();
      for (Method method : methods)
      {
         LegacyAttribute legacyAttrAnno = method.getAnnotation(LegacyAttribute.class);
         if (legacyAttrAnno != null)
         {
            if (legacyAttrAnno.name().equalsIgnoreCase(attr))
            {
               new logical(true);
            }
         }
      }

      return new logical(false);
   }
...
   public static logical canSet(handle h, String attr)
   {
      Method[] methods = h.get().getClass().getMethods();
      for (Method method : methods)
      {
         LegacyAttribute legacyAttrAnno = method.getAnnotation(LegacyAttribute.class);
         if (legacyAttrAnno != null)
         {
            if (legacyAttrAnno.name().equalsIgnoreCase(attr) && legacyAttrAnno.setter())
            {
               new logical(true);
            }
         }
      }

      return new logical(false);
   }
...

Thoughts:

1. The code Method[] methods = h.get().getClass().getMethods(); is not safe to chain this way. h.get() can return null when a handle is unknown.

2. Please check if this code will work when the annotations are not directly in the specific class. For example, when the annotations are on an interface that this class extends.

3. The code is almost the same for both cases. It seems that a common worker method can implement the logic for both.

Constantin: are you suggesting putting all the annotations on interface method definitions instead of on the implementation methods?

Greg Shah wrote:

Constantin: are you suggesting putting all the annotations on interface method definitions instead of on the implementation methods?

Yes, is better to put it there, else we will not be able to build the global attribute/method map. But I'm not sure if Class.getMethods on a class returns the methods from the implemented interface(s) too. I think is best to get the list of implemented interfaces using Class.getInterfaces and call getMethods on each interface.

Some additional thoughts about annotation:

1. I would leave resource() field untouched. This will contain additional info we probably will able to use to separate objects by type.

2. We do not need the writeable() field. Actually the annotation we are planning to use is the feature of the java method. The java method can not be "writeable". It can be either setter or getter. Read-only means we have no setter but we have getter. On the other hand if we have annotated method as not writeable - this does not mean we have no other method as setter.

And one more question:

For example consider the following attribute:

...
               <rule>ftype == prog.kw_adm_data
                  <action>hwrap = "ADMData"</action>
                  <action>methodText = "getADMData"</action>
                  <rule>isAssign
                     <action>methodText = "setADMData"</action>
                  </rule>
               </rule>
...

This refers to the interface ADMData as the subject of modification. This means the interface itself ADMData.java and ALL implementation files where the get|setADMData are encountered should be modified to make the approach working. Correct?

1. I would leave resource() field untouched. This will contain additional info we probably will able to use to separate objects by type.

I believe that we don't need this. The same information is available from CommonHandle.getResourceType() API.

2. We do not need the writeable() field. ... Read-only means we have no setter but we have getter.

Yes, that is what is documented in note 18. The annotation definitions thus look like this:

package com.goldencode.p2j.util;

import java.lang.annotation.*;

/**
 * Annotation definition to mark methods as the replacements for Progress 4GL attributes. This
 * annotation stores the legacy attribute name and a flag to note if this is a getter
 * or setter.
 */
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface LegacyAttribute
{
   String name();
   boolean setter() default false;
}

Actually the annotation we are planning to use is the feature of the java method. The java method can not be "writeable". It can be either setter or getter.

The "writable" flag was intended to document the legacy behavior for an attribute. It was not anything to do with the Java method implementation. Anyway, this is no longer needed so don't worry about it.

This means the interface itself ADMData.java and ALL implementation files where the get|setADMData are encountered should be modified to make the approach working. Correct?

No. Only the methods in the interface need to be annotated.

And the annotation for method should be:

package com.goldencode.p2j.util;

import java.lang.annotation.*;

/**
 * Annotation definition to mark methods as the replacements for Progress 4GL methods. This
 * annotation stores the legacy resource type and the method name.
 */
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface LegacyMethod
{
   String name();
}

Yes, except the javadoc should read like this:

/**
 * Annotation definition to mark methods as the replacements for Progress 4GL methods. This
 * annotation stores the legacy method name.
 */

Need another clarification. Consider the following rule:

...
<rule>ftype == prog.kw_buf_name
   <action>hwrap = "BufferField"</action>
   <action>methodText = "bufferName"</action>
</rule>
...

 <action>hwrap = "FieldInterface"</action>

Eugenie Lyzenko wrote:

Need another clarification. Consider the following rule:
[...]
There are no BufferField class/interface. We have FieldInterface.java class with bufferName() method declared as conversion for BUFFER-NAME attribute. Does this mean we need to fix methods_attributes.rules to change:[...]? And the real file to modify is FieldInterface.java? Or may be I've missed for something?

No, please don't change the hwrap values in methods attributes. To identify the interface defining the attribute/method, go to the handle class and check the return type of the handle.unwrap<hwrap> method (where <hwrap> is replaced with the value from methods_attributes.rules for that attribute/method).

No, please don't change the hwrap values in methods attributes. To identify the interface defining the attribute/method, go to the handle class and check the return type of the handle.unwrap<hwrap> method (where <hwrap> is replaced with the value from methods_attributes.rules for that attribute/method).

Exactly. Thanks for note. I've knew this but just forget.

Another point:

...
<rule>ftype == prog.kw_def_bufh
   <action>methodText = "defaultBufferHandle"</action>
   <action>hwrap = "Buffer"</action>
</rule>
...

The Buffer.java does not have defaultBufferHandle method. This method is moved into BufferHandle.java. The unwrap class for Buffer is Buffer(.java). And looks like we have to modify the rule.

Eugenie Lyzenko wrote:

Another point:
[...]

The Buffer.java does not have defaultBufferHandle method. This method is moved into BufferHandle.java. The unwrap class for Buffer is Buffer(.java). And looks like we have to modify the rule.

As Buffer extends BufferHandle, the rule and handle.unwrapBuffer are OK.

Another question.

The attribute PERSISTENT has described in Progress docs as readable/writable. But in P2J conversion we have only getter for keyword kw_persist: isPersistent() in PersistentProcedure class. So we do not support PERSISTENT attribute changing on conversion level and have no setter for it. Is it OK and knows issue?

The additional notes at this time:

NUM-BUFFERS r P2JQuery - (Changed javadoc, bacause it is attribute, not a method)
KEEP-Z-ORDER rw CommonWindow - (artificial attribute? The real one - KEEP-FRAME-Z-ORDER is missing in conversion rule)
MAX-WIDTH rw CommonWindow - (artificial attribute? The real one - MAX-WIDTH-CHARS is missing in conversion rule)
MAX-HEIGHT rw CommonWindow - (artificial attribute? The real one - MAX-HEIGHT-CHARS is missing in conversion rule)
SERVER rw Remotable - (we do not have the setter for the writeable part of this attribute)

Currently the remaining class are CommonWidget, FileInfo and some other classes.

The attribute PERSISTENT has described in Progress docs as readable/writable. But in P2J conversion we have only getter for keyword kw_persist: isPersistent() in PersistentProcedure class. So we do not support PERSISTENT attribute changing on conversion level and have no setter for it. Is it OK and knows issue?

I think the docs are wrong; testing showed that the PERSISTENT attribute for procedure handles is read-only.

NUM-BUFFERS r P2JQuery - (Changed javadoc, bacause it is attribute, not a method)

OK

KEEP-Z-ORDER rw CommonWindow - (artificial attribute? The real one - KEEP-FRAME-Z-ORDER is missing in conversion rule)

If you look in progress.g line 27684, KW_KEEP_ZOR is associated with keep-frame-z-order - so I don't think there is an issue here. Note that the associated API doesn't allways follow the legacy attribute/method name.

MAX-WIDTH rw CommonWindow - (artificial attribute? The real one - MAX-WIDTH-CHARS is missing in conversion rule)
MAX-HEIGHT rw CommonWindow - (artificial attribute? The real one - MAX-HEIGHT-CHARS is missing in conversion rule)

These ones look like they are abreviations for MAX-WIDTH-CHARS and MAX-HEIGHT-CHARS. Anyway, if KW_MAX_H_C/KW_MAX_W_C are not treated in methods_attributes.rules, don't worry about them.

SERVER rw Remotable - (we do not have the setter for the writeable part of this attribute)

For current purposes, SERVER attribute is read-only (from docs, only Call object handle has this attribute as readable).

The attribute PERSISTENT has described in Progress docs as readable/writable. But in P2J conversion we have only getter for keyword kw_persist: isPersistent() in PersistentProcedure class. So we do not support PERSISTENT attribute changing on conversion level and have no setter for it. Is it OK and knows issue?

I think the docs are wrong; testing showed that the PERSISTENT attribute for procedure handles is read-only.

OK.

KEEP-Z-ORDER rw CommonWindow - (artificial attribute? The real one - KEEP-FRAME-Z-ORDER is missing in conversion rule)

If you look in progress.g line 27684, KW_KEEP_ZOR is associated with keep-frame-z-order - so I don't think there is an issue here. Note that the associated API doesn't allways follow the legacy attribute/method name.

I see. The problem is we have the methods for both:
KEEP-Z-ORDER:
isKeepZOrder();
setKeepZOrder(logical logical);

and for KEEP-FRAME-Z-ORDER:
isKeepFrameZOrder();
setKeepFrameZOrder(boolean keep);
setKeepFrameZOrder(logical keep);

So you think we have to mark all 5 methods above as getter|setter for KEEP-FRAME-Z-ORDER? Note according to conversion rule *KeepFrameZOrder() methods will not be in converted code.

MAX-WIDTH rw CommonWindow - (artificial attribute? The real one - MAX-WIDTH-CHARS is missing in conversion rule)
MAX-HEIGHT rw CommonWindow - (artificial attribute? The real one - MAX-HEIGHT-CHARS is missing in conversion rule)

These ones look like they are abreviations for MAX-WIDTH-CHARS and MAX-HEIGHT-CHARS. Anyway, if KW_MAX_H_C/KW_MAX_W_C are not treated in methods_attributes.rules, don't worry about them.

The same as for KEEP-FRAME-Z-ORDER. We have (get|set)Max(Width|Height)Chars and (get|set)Max(Width|Height). We need to assign MAX_(WIDTH|HEIGHT)_CHARS to both subsets. Correct? Note the (get|set)Max(Width|Height)Chars subset will never happen in converted code because we have no rule that assign this method to code.

SERVER rw Remotable - (we do not have the setter for the writeable part of this attribute)

For current purposes, SERVER attribute is read-only (from docs, only Call object handle has this attribute as readable).

OK.

I see. The problem is we have the methods for both:
KEEP-Z-ORDER:
isKeepZOrder();
setKeepZOrder(logical logical);

and for KEEP-FRAME-Z-ORDER:
isKeepFrameZOrder();
setKeepFrameZOrder(boolean keep);
setKeepFrameZOrder(logical keep);

Greg, any idea why we have both KEEP-Z-ORDER and KEEP-FRAME-Z-ORDER? IMO, only KEEP-FRAME-Z-ORDER is correct, as this is the only one which appears in the docs.

Eugenie: please annotate only the java methods which are emitted by the methods_attributes.rules. The others, if they don't get emitted, there is no reason to annotate them.

And BTW, I was thinking: there might be cases when an attribute is writeable for a resource, and for other resource is read-only. If there are such cases, we should mark the implementation java method(s) with an annotation, and when determining which annotation to use, these would have higher priority than the method definition at the interface.

Greg, any idea why we have both KEEP-Z-ORDER and KEEP-FRAME-Z-ORDER? IMO, only KEEP-FRAME-Z-ORDER is correct, as this is the only one which appears in the docs.

I don't know why. KEEP-Z-ORDER should not be there.

Moving to the static calls(SessionUtils, ProcedureManager, etc.) The new notes found for CommonWidget:
The attribute are not implemented(we have conversion names but appropriate methods are missing in target interface)
FGCOLOR rw CommonWidget - (get|set)FgColor
HANDLE r CommonWidget - getHandle
IMAGE r CommonWidget - getImage
LOAD-IMAGE CommonWidget - loadImage
THREE-D rw CommonWidget - (is|set)ThreeD
VIRTUAL-HEIGHT-CHARS rw CommonWidget - (get|set)VirtHeight
VIRTUAL-WIDTH-CHARS rw CommonWidget - (get|set)VirtWidth

Additional question. Consider the situation when several interfaces extends the same one, like CommonWidget, CommonWindow, CommonFrame. So they are not related with each other. If some getter/setter is encountering not in only one - we need to declare every occurrence, not only one that declared in rule file, correct? An example - the both CommonWidget and CommonFrame have isVisible() method referring to the VISIBLE attribute. Do we need to add annotation for both classes? Event is the rule file refers to the CommonWidget only class?

Eugenie Lyzenko wrote:

Moving to the static calls(SessionUtils, ProcedureManager, etc.)

For these too, you will need to annotate the associated interface(s), and not their static implementation. See the asHandle method from those classes, for the implemented interface(s).

The new notes found for CommonWidget:

The attribute are not implemented(we have conversion names but appropriate methods are missing in target interface)
FGCOLOR rw CommonWidget - (get|set)FgColor
HANDLE r CommonWidget - getHandle
IMAGE r CommonWidget - getImage
LOAD-IMAGE CommonWidget - loadImage
THREE-D rw CommonWidget - (is|set)ThreeD
VIRTUAL-HEIGHT-CHARS rw CommonWidget - (get|set)VirtHeight
VIRTUAL-WIDTH-CHARS rw CommonWidget - (get|set)VirtWidth

Ignore them for now.

Additional question. Consider the situation when several interfaces extends the same one, like CommonWidget, CommonWindow, CommonFrame.

When their "handle" flavor is needed, access to them will be done via CommonWidget interface. Thus, follow the same rule: annotate the interface returned by the handle.unwrap API emitted by the conversion rules.

Moving to the static calls(SessionUtils, ProcedureManager, etc.)

For these too, you will need to annotate the associated interface(s), and not their static implementation. See the asHandle method from those classes, for the implemented interface(s).

Let me clarify this point. For example consider the rule:

...
<rule>ftype == prog.kw_disp_tz
   <rule>isAssign
      <action on="false">methodText = "SessionUtils.getDisplayTimeZone"</action>
      <action on="true" >methodText = "SessionUtils.setDisplayTimeZone"</action>
   </rule>
</rule>
...

You are talking it is not correct to annotate SessionUtils.getDisplayTimeZone and SessionUtils.setDisplayTimeZone. Correct?

Instead I should see SessionUtils.asHandle() to see what class instance will be really invoked here and what interface it implements. Then the method in interface should be annotated. Correct?

Code Review 0917b

Generally it looks really good. Some feedback:

1. Since NumberType isn't a WrappedResource, it won't ever be referenced via a handle. Instead, we use static methods as a replacement for some SESSION:xxxxxx attibutes. I wonder if we have issues with the determining the fact that these are implementing features of the SESSION resource. We will have similar problems with other system handles that are backed by static methods. Constantin - what do you think about this?

2. You have some major merging to do with the recent sockets code. And there are massive changes to the DOM (and SAX) files that will be available soon. Be prepared to merge there too.

3. I think the HandleOps.getLegacyAttribute() code will need some improvements. For example, don't forget this comment by Constantin:

I think is best to get the list of implemented interfaces using Class.getInterfaces and call getMethods on each interface.

Constantin: please look at that method and comment.

4. It seems like you can remove the TODOs in HandleOps.

2. You have some major merging to do with the recent sockets code.

Done.

And there are massive changes to the DOM (and SAX) files that will be available soon. Be prepared to merge there too.

OK.

4. It seems like you can remove the TODOs in HandleOps.

Done.

3. I think the HandleOps.getLegacyAttribute() code will need some improvements. For example, don't forget this comment by Constantin:

I think is best to get the list of implemented interfaces using Class.getInterfaces and call getMethods on each interface.

OK. The modified method will be:

...
   private static LegacyAttribute getLegacyAttribute(handle h, String attr)
   {
      Object objFromHandle = h.get();

      if (objFromHandle != null)
      {
         // Getting iterfaces from object
         Class[] classes = objFromHandle.getClass().getInterfaces();
         if (classes != null)
         {
            for (Class class_curr : classes)
            {
               // For each interface implementation get the methods and check each one
               Method[] methods = class_curr.getMethods();
               if (methods != null)
               {
                  for (Method method : methods)
                  {
                     LegacyAttribute legacyAttrAnno = method.getAnnotation(LegacyAttribute.class);
                     if (legacyAttrAnno != null &&
                         legacyAttrAnno.name().equalsIgnoreCase(attr))
                     {
                        return legacyAttrAnno;
                     }
                  }
               }
            }
         }
      }

      return null;
   }
...

1. Since NumberType isn't a WrappedResource, it won't ever be referenced via a handle. Instead, we use static methods as a replacement for some SESSION:xxxxxx attibutes. I wonder if we have issues with the determining the fact that these are implementing features of the SESSION resource. We will have similar problems with other system handles that are backed by static methods. Constantin - what do you think about this?

NumberType shouldn't have been annotated. The proper interface for the SESSION resource is the CommonSession interface. The changes in SessionUtils, NumberType, ProcedureManager are not needed (let me know if you have annotations in other non-interface files). More, as the SESSION resource is a static proxy, we will not have access to the API's implementation via reflection, and we'll need to interrogate the StaticProxy class, if we need such info).

3. I think the HandleOps.getLegacyAttribute() code will need some improvements. For example, don't forget this comment by Constantin:
Constantin: please look at that method and comment.

The changes in note #47 look OK. But we need to keep in mind that IIRC there are some attributes which are settable for some resources, and not read-pnly for others (although I can't put my finger on an example now). If we have such cases, we will need to check the implementation methods too (and an annotation from an implementation method would have higher precedence than a corresponding annotation from its interface definition).

NumberType shouldn't have been annotated. The proper interface for the SESSION resource is the CommonSession interface. The changes in SessionUtils, NumberType, ProcedureManager are not needed (let me know if you have annotations in other non-interface files)....

SessionUtils.java, ProcedureManager.java, NumberType.java are involved in this issue. You mean the annotation should be removed from these classes because they will never be found. And we will not have the annotations for attributes/methods residing in these classes. Correct?

Eugenie Lyzenko wrote:

NumberType shouldn't have been annotated. The proper interface for the SESSION resource is the CommonSession interface. The changes in SessionUtils, NumberType, ProcedureManager are not needed (let me know if you have annotations in other non-interface files)....

SessionUtils.java, ProcedureManager.java, NumberType.java are involved in this issue. You mean the annotation should be removed from these classes because they will never be found. And we will not have the annotations for attributes/methods residing in these classes. Correct?

These 3 classes are statically implementing the proxy defined by the CommonSession interface - so you are correct, annotations in these classes are not needed.

These 3 classes are statically implementing the proxy defined by the CommonSession interface - so you are correct, annotations in these classes are not needed.

So we have lost the following attributes:

FIRST-SERVER r SessionUtils
LAST-SERVER r SessionUtils
TIME-SOURCE r SessionUtils (but we have setter in SessionExports class)
TIMEZONE r SessionUtils (but we have setter in SessionExports class)
DISPLAY-TIMEZONE r SessionUtils (but we have setter in SessionExports class)
HANDLE r (SessionUtils)

I assume you still have these attributes annotated in their CommonSession interface?

I assume you still have these attributes annotated in their CommonSession interface?

No, there are no such attributes in CommonSession interface. We need to add method definitions into CommonSession interface to be able to annotate them there.

Are they defined in some other interface?

Are they defined in some other interface?

The following attributes are not defined at all:
FIRST-SERVER r
LAST-SERVER r
HANDLE r

For the following attributes the setter is defined in SessionExports, the getter is not defined anywhere.
TIME-SOURCE rw
TIMEZONE rw
DISPLAY-TIMEZONE rw

OK, I think I understand now. If we have the backing implementations for the missing attributes, please do add the 6 getters into CommonSession (with the annotations, of course).

The changes look good. When you get back, you can merge up with the latest code, implement the DOM/SAX attributes/methods and finish improving the getLegacyAttribute() algorithm.

Eugenie: there are some cases when the interface defining the resource is not directly implemented by the P2J class, but by one of its superclasses. See the P2JQuery cases: AbstractQuery implements P2JQuery, and the same legacy resource (query) can have multiple implementations in P2J: FindQuery, CompoundQuery, etc. So, you need to retrieve the interfaces from the super classes, too.

Eugenie: there are some cases when the interface defining the resource is not directly implemented by the P2J class, but by one of its superclasses. See the P2JQuery cases: AbstractQuery implements P2JQuery, and the same legacy resource (query) can have multiple implementations in P2J: FindQuery, CompoundQuery, etc. So, you need to retrieve the interfaces from the super classes, too.

Does not the call objFromHandle.getClass().getInterfaces() retrieve ALL interfaces implementing in the current class? Including superclasses as well. In your example FindQuery and CompoundQuery both implements P2JQuery interface. So the P2JQuery interface should be the member of the interfaces array returned by getInterfaces() call. Is this correct or not?

Does not the call objFromHandle.getClass().getInterfaces() retrieve ALL interfaces implementing in the current class? Including superclasses as well. In your example FindQuery and CompoundQuery both implements P2JQuery interface. So the P2JQuery interface should be the member of the interfaces array returned by getInterfaces() call. Is this correct or not?

Your assumption is incorrect. object.getClass().getInterfaces() will always return the directly implemented interfaces for the runtime type of that object; it doesn't search up the superclasses or up the interface hiearchy - you need to do this by hand.

Your assumption is incorrect. object.getClass().getInterfaces() will always return the directly implemented interfaces for the runtime type of that object; it doesn't search up the superclasses or up the interface hiearchy - you need to do this by hand.

Yes, you are right. I have found confirmation for this in official Java tutorials. OK, understood. But this is pretty strange.

But we need to keep in mind that IIRC there are some attributes which are settable for some resources, and not read-only for others (although I can't put my finger on an example now). If we have such cases, we will need to check the implementation methods too (and an annotation from an implementation method would have higher precedence than a corresponding annotation from its interface definition).

Please clarify this. You mean the attribute can be settable in one case and read-only in another?

We annotate only interfaces. How can we check implementations to find annotation there if they are not annotated at all?

The following is the new suggest for getLegacyAttribute including superclasses check for review:

...
   private static LegacyAttribute getLegacyAttribute(handle h, String attr)
   {
      Object objFromHandle = h.get();

      if (objFromHandle != null)
      {
         // Getting the base class
         Class current_base = objFromHandle.getClass(); 
         // Getting iterfaces from current class
         while (current_base != null)
         {
            Class[] classes = current_base.getInterfaces();
            if (classes != null)
            {
               for (Class iface_curr : classes)
               {
                  // For each interface implementation get the methods and check each one
                  Method[] methods = iface_curr.getMethods();
                  if (methods != null)
                  {
                     for (Method method : methods)
                     {
                        LegacyAttribute legacyAttrAnno =
                           method.getAnnotation(LegacyAttribute.class);
                        // Check annotation for name match(ignoring letter case)
                        if (legacyAttrAnno != null &&
                            legacyAttrAnno.name().equalsIgnoreCase(attr))
                        {
                           return legacyAttrAnno;
                        }
                     }
                  }
               }
            }
            // If we are here the annotation is not found, try superclass if exists
            current_base = current_base.getSuperclass(); 
         }
      }

      return null;
   }
...

The question about recent rule changes. According to the methods_attributes.rule the attribute PUBLIC-ID is rw and has the setter:

...
<rule>ftype == prog.kw_pub_id
   <action>hwrap = "XDocument"</action>
   <action>methodText = "getPublicId"</action>
   <rule>isAssign
      <action>methodText = "setPublicId"</action>
   </rule>
   <action>xmlimport = true</action>
</rule>
...

What is correct in this point?

Evgeny Kiselev: Please respond to the question in note 64.

Eugenie Lyzenko wrote:

The question about recent rule changes. According to the methods_attributes.rule the attribute PUBLIC-ID is rw and has the setter:
[...]
But in 4GL doc it is read-only. And moreover we do not have setter method setPublicId() defined in XDocument interface.

What is correct in this point?

Yes, it's a bug, but it's affects only methods_attributes.rule, changes in the code is not needed. Do I need to fix it ?

Yes, it's a bug, but it's affects only methods_attributes.rule, changes in the code is not needed. Do I need to fix it ?

No problem. EVL can fix it as part of this task.

Greg Shah wrote:

Yes, it's a bug, but it's affects only methods_attributes.rule, changes in the code is not needed. Do I need to fix it ?

No problem. EVL can fix it as part of this task.

I rembered why I've added setter for this method.

/*test public-id attribute*/
function getPublicId returns int (err-num as int):
   def var hDoc as handle.
   def var hRoot as handle.
   def var hNode1 as handle.
   def var res as logical init false.
   def var c as character.
   def var unk as character.
   def var namespaceURI as character.
   def var rootNodeName as character.
   def var systemId as character.
   def var publicId as character.

   namespaceURI = "http://www.w3.org/1999/xhtml".
   rootNodeName = "h:html".
   systemId = "http://www.w3.org/TR/html4/strict.dtd".
   publicId = "-//W3C//DTD HTML 4.01//EN".

   create x-document hDoc.

   c = "asd".
   c = hDoc:public-id.
   if not c eq ? then "Unexpected error1: public-id doesn't equals ? string".
   c = ?.
   hDoc:public-id = c no-error.
   if error-status:error or not error-status:get-number(1) eq 4083
            then message "Unexpected error2: " error-status:get-number(1) " " error-status:get-message(1).

   hDoc:public-id = "test test" no-error.
   if error-status:error or not error-status:get-number(1) eq 4052
      then message "Unexpected error3: " error-status:get-number(1) " " error-status:get-message(1).

   hDoc:initialize-document-type(namespaceURI, rootNodeName, publicId, systemId).
   if not hDoc:public-id eq publicId
            then message "Unexpected error public-id4: " error-status:get-number(1) " " error-status:get-message(1).

   hDoc:initialize-document-type(namespaceURI, rootNodeName, ?, systemId).
   if not hDoc:public-id eq "" 
               then message "Unexpected error public-id5: " error-status:get-number(1) " " error-status:get-message(1).

   return 0.
end.

getPublicId(-1).

c = ?.
hDoc:public-id = c no-error.

Code Review 1002a

Everything looks OK to me except that the SaxWriter.endElement() methods are marked as END-DOCUMENT instead of END-ELEMENT.

I'll let Constantin take a closer look at HandleOps.getLegacyAttribute().

What tests are you using to check the functionality?

1. LABEL: For the LAST-EVENT handle, this attribute is readable only.
2. MODIFIED: For browse columns, this attribute is readable only
3. COLUMN: For browse cells, field groups, and the LAST-EVENT handle, it is readable only.
4. NO-VALIDATE: This attribute is readable by both static and dynamic browses, and writeable only by dynamic browses.

There may be more cases, didn't check all attributes. So, we need to change the HandleOps.getLegacyAttribute() code so that an annotation for a method at an implementation class has precedence over an annotation at an interface method definition.

This is the universal testcase from Progress docs(with little modifications and improvement). Possible to check each widget and each attribute:

...
DEFINE VARIABLE attribute AS CHARACTER NO-UNDO FORMAT "x(24)" LABEL "Attribute".
DEFINE VARIABLE queryable AS LOGICAL NO-UNDO LABEL "Query".
DEFINE VARIABLE setable AS LOGICAL NO-UNDO LABEL "Set".
DEFINE VARIABLE temp-handle AS HANDLE NO-UNDO.
DEFINE VARIABLE widget-type AS CHARACTER NO-UNDO FORMAT "x(24)" LABEL "Widget".

FORM widget-type attribute setable queryable.

REPEAT:
   UPDATE widget-type attribute.
   CREATE VALUE(widget-type) temp-handle.
   queryable = CAN-QUERY(temp-handle, attribute).
   setable   = CAN-SET(temp-handle, attribute).
   DISPLAY queryable setable.
   pause.
   DELETE WIDGET temp-handle.
END.
...

Converted and working.

The new question. We have separate interface for frame widget - CommonFrame. This means the similar methods in this interface should also be annotated as in CommonWidget. Another word we have two independent interfaces with same attribute/method names and this is OK. Correct?

The second question. public handle asWidgetHandle(); in CommonWidget and CommonFrame interfaces gets the HANDLE attribute of the widget and frame respectively. Correct?

The changes look good in 1003a.

About the tests: note that CREATE VALUE(...) can't be used with non-widget resource types; something like CREATE VALUE("SERVER") doesn't work in 4GL. Do you know otherwise?

More, can you think of a way to automate this? i.e. build a file with (resource, attribute, settable) records and a file with all attribute names, and use these to determine how P2J reports a certain attribute. It should work the same for methods.

The new question. We have separate interface for frame widget - CommonFrame. This means the similar methods in this interface should also be annotated as in CommonWidget.

Our current implementation of GenericFrame and CommonFrame which assumes they are the legacy 4GL resource is wrong. I have an update WIP which will address this; in P2J, the legacy frame resource is mapped by FrameWidget class.

Another word we have two independent interfaces with same attribute/method names and this is OK. Correct?

See above. CommonFrame must not be annotated, as it isn't used to define a legacy resource.

The second question. public handle asWidgetHandle(); in CommonWidget and CommonFrame interfaces gets the HANDLE attribute of the widget and frame respectively. Correct?

The HANDLE attribute is not explicitly emitted by the conversion rules, and code should not rely on this attribute. asWidgetHandle() is something used internally by P2J and should not be annotated. This attribute AFAIK applies to all resources, and is read only.

Code Review 1003a

I am fine with the code. Please do add the following text to the javadoc for the HandleOps.getLegacyAttribute() method:

    * <p>
    * The search algorithm for this method uses reflection to read annotations from various sources in the
    * following precedence order:
    * <ol>
    *   <li> The class of the resource itself.
    *   <li> The superclass of the resource or the superclass of the superclass... the entire parent 
    *        hierarchy is walked from the resource to the root.  The first match found ends the
    *        upward search.
    *   <li> All interfaces of the resource itself.
    *   <li> All interfaces of the superclass of the resource or the superclass of the superclass...
    *        the entire parent hierarchy is walked from the resource to the root.  The first superclass
    *        with an interface that has a matching attribute will end the upward search.
    * </ol>
    * <p>
    * Where possible, we put annotations on the interface, not the class for the resource.  But there
    * are some attributes where the configuration is not universal for all resources implementing the
    * same attribute.  For example, the following attributes are readable for certain resources even
    * though for most other resources the same attribute is writable:
    * <ul>
    *   <li> LABEL for LAST-EVENT
    *   <li> MODIFIED for browse columns
    *   <li> COLUMN for browse cells, field groups, and LAST-EVENT
    *   <li> NO-VALIDATE for both static and dynamic browses (it is writeable only for dynamic browses)
    * </ul>
    * <p>
    * Keeping the above precedence order is critical to allow attribute configurations of more-specific
    * resources to properly override the more general configuration in the parent hierarchy or the
    * interface hierarchy.

About the testcase code:

Please do continue with the 4GL approach. That gives us a much better overall test. Other feedback:

About the tests: note that CREATE VALUE can't be used with non-widget resource types; something like CREATE VALUE doesn't work in 4GL. Do you know otherwise?

Each resource will need custom code to create it.

More, can you think of a way to automate this? i.e. build a file with (resource, attribute, settable) records and a file with all attribute names, and use these to determine how P2J reports a certain attribute. It should work the same for methods.

Please do make the 4GL testcase completely non-interactive. I like the idea of generating an output report (using DISPLAY) to shows the test results.

This part:

REPEAT:
   UPDATE widget-type attribute.
   CREATE VALUE(widget-type) temp-handle.
   queryable = CAN-QUERY(temp-handle, attribute).
   setable   = CAN-SET(temp-handle, attribute).
   DISPLAY queryable setable.
   pause.
   DELETE WIDGET temp-handle.
END.

should be turned into a function:

DEFINE FRAME widget-type attribute setable queryable WITH frame f0.

function check-attribute returns logical (input h as handle):
   queryable = CAN-QUERY(h, attribute).
   setable   = CAN-SET(h, attribute).
   DISPLAY queryable setable WITH frame f0.
   return true.
end.

Then just call this from inside the loop that reads the input file.

Good work. I like the approach. When it is working in the 4GL, go ahead and check it in to the testcases project.

The only change I would like to see: put all of these files (the .p files, the ini and a captured log of the 4GL output) into a testcases/uast/can_set_can_query/ subdirectory.

... When it is working in the 4GL, go ahead and check it in to the testcases project.

The INI file and captured 4GL log should also be committed in bzr. Correct?

The INI file and captured 4GL log should also be committed in bzr. Correct?

Yes.

Please do merge and then re-run the tests.

Also note in INI file some resources has "?" as SetExp value. This means the attribute is supported in 4GL but not yet implemented in P2J.

I don't understand what SetExp of yes and no mean.

There are many deviations. Are these problems with our CAN-SET/CAN-QUERY implementation or is it some other issue?

I don't understand what SetExp of yes and no mean.

SetExp is Setter Expected

yes - means the attribute should be write-able
no - means the attribute should be read-only
? - means the attribute exists but not yet annotated in P2J

OK, please work your way through the debugging and report your findings here.

It can be both CAN-SET/CAN-QUERY implementation and places where we put annotation or may resource unwrapping.

Or it could be that Progress reports something customized rather than the simple fact of whether there is a getter/setter or not. I hope this is not the case, but so many times we have found deviations between what the Progress docs say and what the Progress code actually does.

Code Review 1009a

1. Please add javadoc for the name and setter members of LegacyAttribute.java.

2. Please add javadoc for the name member of LegacyMethod.java.

Upload these changes and then go ahead and check them into Bazaar and send them out via email. I believe that all of these changes are safe enough that we can avoid testing, since virtually all the changes just add some metadata to the classes/methods. And those changes that aren't just annotations are not in active use right now.

After you have checked in your changes, you can continue the debugging. Since these changes affect so many files, I want to get them into the trunk.

Also: when you send out the email, you will have to explain the additional step people must always take when adding an attribute or method. Please explain how to create the annotations and where (interface vs implementation class...). Please also explain why we are doing this.

Eugenie, be aware there is a known case to convert badly in P2J: h = temp-table tt1:handle (getting the handle of a static temp-table) will convert to h.assign(tt1) (i.e. the buffer) instead of assigning the temp-table resource.

1. Please add javadoc for the name and setter members of LegacyAttribute.java.

2. Please add javadoc for the name member of LegacyMethod.java.

OK.

Upload these changes and then go ahead and check them into Bazaar and send them out via email. I believe that all of these changes are safe enough that we can avoid testing, since virtually all the changes just add some metadata to the classes/methods. And those changes that aren't just annotations are not in active use right now.

OK.

2. TEMP-TABLE resource should be Errorable. Otherwise we have no support for ERROR rw attribute.

This issue can easily be fixed by changing the TempTableBuilder.java:

...
public class TempTableBuilder
extends HandleChain
implements TempTable,
           Deletable,
           Errorable
...

2. TEMP-TABLE resource should be Errorable. Otherwise we have no support for ERROR rw attribute.

Yes, please fix it.

2. The attribute HANDLE is missed everywhere

This is expected. We don't have a special getter/setter for this attribute because in all the cases where we are asking for a resource:HANDLE, the Java reference to the resource is in fact the handle that is being requested. I don't really understand why Progress needs this either, but they do.

Anyway, I think you will have to put a special case in for this one because it can't be looked up. So we need to pretend that we looked it up and respond back from CAN-QUERY/SET appropriately.

1. The attribute PREV-TAB-ITEM is not implemented. This is strange because NEXT-TAB-ITEM is OK.
3. The attribute NUM-ITEMS is missed in combo-box and selection-list
4. The attribute DYNAMIC is missed in all widgets
5. The attribute DBNAME is missed in all widgets
6. The attributes NAMESPACE-PREFIX and NAMESPACE-URI are missed for temp-table resource. Looks like we need separate class for NAMESPACE-PREFIX and include both in extends list of the TempTable class.

Are any of these methods that we are supposed to have implemented? If they are missing because we just haven't built them, it is OK.

Code Review 1009d

I am fine with all the changes.

Code Review 1010a

I am OK with the changes.

OK. I'll work on solution.

This should be the last change for this task.

So looks like it is OK for now because we just do not have appropriate implementation. And for now we can leave these issues untouched. Correct?

Yes, unless you have some reason to believe they should exist.

It looks good. Check it in and distribute it.

1010b has been committed as 10398.

Looks like there is a case I've overlooked: for CommonLastEvent interface (defining the LAST-EVENT resource in P2J), we have attributes which do not belong to LAST-EVENT, and these are implemented by the KeyReader class by calling handle.readOnlyError for setters which should not be there (as the attribute is read-only or does not belong to this resource) or handle.invalidAttribute for getters which do not belong to LAST-EVENT.

Idea is, I think currently a CAN-QUERY(LAST-EVENT, "format") will pass in P2J. We should enhance the search with a "not my attribute" annotation set at the implementing method for these cases. The reason we built it like this initially was to reduce the number of interfaces, when an attribute/method belongs to more than one resource. To make sure all cases are treated, a search for handle.invalidAttribute and handle.readOnlyError (throughout the P2J project) can be used to provide a list. More, handle.getAttributeName should be fixed to use the LegacyAttribute/Method annotations and the handle.apisToAttrOrMthd should be removed.

And something related to the tests: currently, I think they check only for cases like "this attribute is used by resource X", but it doesn't check that an attribute A which is not supported by resource R in 4GL is reported correctly in P2J (i.e. CAN-QUERY must report "no" both in 4GL and P2J for all attributes/methods which are not supported for a certain resource).

Sorry for coming with this at the end of the party, but it didn't occure to me until now, when I glanced a look at the KeyReader class.

Looks like there is a case I've overlooked: for CommonLastEvent interface (defining the LAST-EVENT resource in P2J), we have attributes which do not belong to LAST-EVENT, and these are implemented by the KeyReader class by calling handle.readOnlyError for setters which should not be there (as the attribute is read-only or does not belong to this resource) or handle.invalidAttribute for getters which do not belong to LAST-EVENT.

Idea is, I think currently a CAN-QUERY will pass in P2J....

Yes, this is true.

We should enhance the search with a "not my attribute" annotation set at the implementing method for these cases. The reason we built it like this initially was to reduce the number of interfaces, when an attribute/method belongs to more than one resource.

May be the current interfaces class tree does not properly reflect the resource dependency? Don't we need to change interfaces inheritance?

To make sure all cases are treated, a search for handle.invalidAttribute and handle.readOnlyError (throughout the P2J project) can be used to provide a list.

Please expand what are you talking about. The list of what? May be an example can help.

... More, handle.getAttributeName should be fixed to use the LegacyAttribute/Method annotations and the handle.apisToAttrOrMthd should be removed.

Greg, do we need this change now?

May be the current interfaces class tree does not properly reflect the resource dependency? Don't we need to change interfaces inheritance?

We chose this path to reduce the number of interfaces; there are many attributes (like FORMAT) which belong to more than one resource, but we can't pair it with other attributes/methods and add them to another interface, as the resources' attrs/methods sets don't intersect. So, instead of making an interface for each attribute which belongs to more than one resource, we chose to bundle them by their semantic and if a resource doesn't use it, implement it by calling i.e. handle.invalidAttribute.

Please expand what are you talking about. The list of what? May be an example can help.

• build a file with all attributes/methods defined in 4GL
• for each resource, use CAN-QUERY/CAN-SET against the all attributes/methods list; the result off these two method calls will be checked with the file which holds the expected result for each resource. If the attribute/method is not in the list for that resource, it is assumed the attribute/method is not supported and both return false; else, the values must match.

One example. Consider the FORMAT attribute for LAST-EVENT resource.

The handle constructed for this resource will be KeyReader.asHandle(), right?

Taking the handle in HandleOps.canQuery(handle h, String attr) and running h.get().getClass() we have the KeyReader class(implementation), correct?

Enumerating the KeyReader implementation methods we have the array of the methods including KeyReader.getFormat()/setFormat(), right?

And question: can we annotate the static method of the KeyReader class like this?

...
   @LegacyAttribute(name = "FORMAT", implemented = false)
   public static character getFormat()
...

implemented is the new LegacyAttribute member indicating the attribute is really included in the given class/interface. The default is true. Another word will annotated implementation methods be obtained by method.getAnnotation(LegacyAttribute.class) call in HandleOps.getLegacyAttributeInClass()?

BTW, there is another bug in the code: canQuery/canSet must check if the handle is unknown; in this case these methods return unknown, not false.

If you re-look the HandleOps code for canQuery/canSet wrappers you will find the code that handles this case:

...
   public static logical canQuery(handle h, character attr)
   {
      if (h == null || h.isUnknown() || attr == null || attr.isUnknown())
      {
         return new logical();
      }
...
   public static logical canSet(handle h, character attr)
   {
      if (h == null || h.isUnknown() || attr == null || attr.isUnknown())
      {
         return new logical();
      }
...

So I have thought about the the case you describe and I do not see the bug here. It is not necessary to perform this checking twice because the real workers for canQuery/canSet are called from these wrappers.

If you re-look the HandleOps code for canQuery/canSet wrappers you will find the code that handles this case:

As long as you need to expose to the public all versions of canQuery/canSet, you need to make sure the parameters are fully validated in all cases. I.e. something like l = can-query(h, "type") should convert to l.assign(HandleOp.canQuery(hm, "type")), which calls the HandleOps.canQuery(handle, String) version; and in this case, h will not be validated.

Code Review 1014a

1. Please change "belonged" to "ignore". Of course, the meaning of the boolean is reversed in this case (from "belonged"). The reason: this whole idea is already quite complex and the name "belonged" makes it even more confusing. I think "ignore" is more obvious in its meaning.

2. Please put the new private methods in HandleOps down in the section of the code that has private methods instead of mixing it into the public section.

In regard to this:

To identify all cases, a testcase to check for "this resource doesn't support these attribute/method list" can be used also; the testcase can be built like this:

build a file with all attributes/methods defined in 4GL
for each resource, use CAN-QUERY/CAN-SET against the all attributes/methods list; the result off these two method calls will be checked with the file which holds the expected result for each resource. If the attribute/method is not in the list for that resource, it is assumed the attribute/method is not supported and both return false; else, the values must match.

I don't think we have time for this right now. Please add a task to Redmine and describe what to do there. Make that task "related" to this one.

Please do add at least a handful of attributes that are supposed to fail the lookup to your current testcases, just to prove that the failure case does seem to work. The part to postpone is where we check every possible attribute.

• HandleOps - needs history enry
• LegacyAtribute - change history entry to reference ignore() and not belonged()

About the conflicts you found: their date counterparts need to be made package-private, so they will not be seen when building the StaticProxy. Please fix this; but note that there are some other problems related to these attributes: I think CommonSession is missing definitions for the setters associated with the getters you mention, but this can be fixed later.

Code Review 1015a

1. LogicalTerminal.getHeightChars(), getWidthChars(),getHeightPixels(), getWidthPixels() have had their annotations removed. Was that intentional?

2. If I understand correctly, I think it is the date version of these which needs to be package private:

public static integer getSessionTimeZone()
public static character getTimeSource()
public static integer getDisplayTimeZone()

There should not be a change to SessionUtils.

1. LogicalTerminal.getHeightChars(), getWidthChars(),getHeightPixels(), getWidthPixels() have had their annotations removed. Was that intentional?

Yes, according to 4GL doc the attributes HEIGHT/WIDTH-CHARS, HEIGHT/WIDTH-PIXELS are read-only for SESSION resource. If we annotate these calls the attributes become ignored istead.

2. If I understand correctly, I think it is the date version of these which needs to be package private:

> public static integer getSessionTimeZone()
> public static character getTimeSource()
> public static integer getDisplayTimeZone()
> 

There should not be a change to SessionUtils.

If we change the date class the DatetimeFormat must also be changed:

...
            // use the SESSION:DISPLAY-TIMEZONE if set
            integer sdt = date.getDisplayTimeZone(); Line 608
...

2. If I understand correctly, I think it is the date version of these which needs to be package private:

Correct, the date versions need to be package private.

If we change the date class the DatetimeFormat must also be changed:

integer sdt = date.getDisplayTimeZone(); Line 608

Please change this call to SessionUtils.getDisplayTimeZone().

It looks good. Get it tested.

Also, please make sure you add the additional Redmine task to make a more complete set of testcases.

It looks good. Get it tested.

Will the runtime regression be enough or we need conversion testing too?

Runtime testing is enough.

Runtime testing is enough.

The runtime testing has been passed. The results can be found at 10398_4e69e3d_20131017_evl.zip.

So I think it is ready to be committed and distributed.

Go ahead.

Committed to bzr as 10400.