FWD » Core Development » User Interface

Full Image phrase support for DEFINE BUTTON and DEFINE IMAGE widgets:

size-phrase({SIZE | SIZE-CHARS | SIZE-PIXELS} width BY height)
CONVERT-3D-COLORS
STRETCH-TO-FIT
RETAIN-SHAPE
TRANSPARENT

Please add CREATE IMAGE support by adding the widget to the list DynamicWidgetFactory.java.

Greg,

I've found I've missed one rule file update for image widget conversion support: convert/frame_generator.xml. Without this we can not handle/convert DEFINE IMAGE statement. I had this working just forgot to include into respective update. Sorry. I have recovered this from my memory but one issue is still remaining:

The simple code:

...
message "Hit a key to start".
pause.

def image moon_pic file "man_on_moon.png".
...

...
            message("Hit a key to start");
            pause();
            "\"man_on_moon.png\""; <--there must be no this line

            FrameElement[] elementList0 = new FrameElement[]
            {
               new WidgetElement(fFrame.widgetMoonPic())
            };
...

Can you tell me what rule file I need to change to get rid of this line? Of course if it will not take significant time.

I've found I've missed one rule file update for image widget conversion support: convert/frame_generator.xml. Without this we can not handle/convert DEFINE IMAGE statement. I had this working just forgot to include into respective update.

No problem. Please include it with your next update.

Can you tell me what rule file I need to change to get rid of this line?

See this in frame_generator.xml:

         <!-- Hide static data of DEFINE BUTTON statement -->
         <rule>(type == prog.define_button or 
                type == prog.define_rectangle) and 
               parent.type == prog.statement
            <action>copy.parent.setHidden(true)</action>
         </rule>

Add type == prog.define_image to that and I think the problem will be resolved.

Add type == prog.define_image to that and I think the problem will be resolved.

Thank, this works.

Question. Do we need th support for FROM {X n Y n} | {ROW n COLUMN n} clause of the image phrase?

This sets the origin inside image data to start display image, so we can cut some data from the loaded image. Currently there are no even proper runtime methods to convert this option.

Yes, please implement this.

Code Review evl_upd20150211b.zip

This is really good.

1. I suspect the IMAGE-SIZE/IMAGE-SIZE-CHARS should be decimal type, not integer. This has to be honored all the way from the frame defs to the drawing. The fractions are actually meaningful.

2. Although it is probably OK, I find it confusing that the image phrase generates different setters for the same options, depending on whether it is on a button or image widget.

3. frame_generator.xml has some hard coded tabs.

4. ImageGuiImpl has 2 history entries that should be merged into a single entry.

5. ImageWidget needs the copyright date to be updated.

Is there anything left to do for this task?

Is there anything left to do for this task?

The following options need to be implemented:
CONVERT-3D-COLORS
STRETCH-TO-FIT
RETAIN-SHAPE
TRANSPARENT

Eugenie Lyzenko wrote:

2. Although it is probably OK, I find it confusing that the image phrase generates different setters for the same options, depending on whether it is on a button or image widget.

There are two considerations here I've considered:
1. Button widget can have three different images embedded, while image widget - only one. For every of 3 button image we need to have separate setters.

Instead of having 3 sets of setters, you could have 1 set and generate an enum value that distinguishes between ButtonWidget.UP_IMG/ButtonWidget.DOWN_IMG/ButtonWidget.INSENSITIVE_IMG.

Of course, that still doesn't fix the confusion with ImageWidget emitting set[Width|Height]Chars() instead of setImage[Down|Up|Ins]Chars().

Just leave it for now.

2. Button has size setter that separate from image phrase, so we can have 4 points where button size can be defined, one related to general button size, other 3 - came from used images sizes. We need to have all and on runtime we have to choose the size that is maximum of them.

I understand. This is required.

Code Review evl_upd20150212a.zip

1. All row/col values need to be decimal/double as well. I should have seen this last night. For example: your new ImageWidget.setImageOffset*().

2. Doesn't the client-side drawing layer need changes to handle the decimal/double changes properly?

Eugenie Lyzenko wrote:

Is there anything left to do for this task?

The following options need to be implemented:
CONVERT-3D-COLORS
STRETCH-TO-FIT
RETAIN-SHAPE
TRANSPARENT

OK, please get the current update ready and into testing. Then you can work on these as a separate update.

Instead of having 3 sets of setters, you could have 1 set and generate an enum value that distinguishes between ButtonWidget.UP_IMG/ButtonWidget.DOWN_IMG/ButtonWidget.INSENSITIVE_IMG.

To do this the call:

...
execLib("set_attr",...)
...

Can you point me the place to read to implement 2 parameters setter or the example for something already implemented this way.

Eugenie Lyzenko wrote:

Instead of having 3 sets of setters, you could have 1 set and generate an enum value that distinguishes between ButtonWidget.UP_IMG/ButtonWidget.DOWN_IMG/ButtonWidget.INSENSITIVE_IMG.

To do this the call:
[...]
will not work, right?

Can you point me the place to read to implement 2 parameters setter or the example for something already implemented this way.

Don't worry about this for now. Just leave it as it is.

For future reference, you can look at how we handle the view-as alert-box button type (search on BTN_OK in ui_statements.rules.

Code Review evl_upd20150212b.zip

It looks good, get it both conversion and runtime regression tested.

Greg Shah wrote:

Code Review evl_upd20150212b.zip

It looks good, get it both conversion and runtime regression tested.

The conversion testing completed without regressions. Generated code is identical. Continue with the runtime testing.

The runtime testing completed without regressions(separate run for CTRL-C 3 way tests). The results are in 10754_5c96d7c_20150216_evl.zip.

So the update evl_upd20150212b.zip is ready to be committed and distributed.

Check it in.

Committed in bzr as 10757.

Code Review evl_upd20150218a.zip

It looks good.

Code Review evl_upd20150219a.zip

It is good.

Code Review evl_upd20150220a.zip

1. In Convert3DColorsFilter, are the input colors to be converted really hard coded in the 4GL?

2. In ImageGuiImpl, the checking of the .ico in the filename seems fragile. The filename doesn't have to have that, but can still be loaded as an icon. Likewise, if we load from a server-side resource, this filename extension would not be visible. Don't we already go through a special path to load an icon? Can't we just set the isIcon as part of that process?

1. In Convert3DColorsFilter, are the input colors to be converted really hard coded in the 4GL?

I think so, from OpenEdge: ABL Reference, page 412.

2. In ImageGuiImpl, the checking of the .ico in the filename seems fragile. The filename doesn't have to have that, but can still be loaded as an icon. Likewise, if we load from a server-side resource, this filename extension would not be visible. Don't we already go through a special path to load an icon? Can't we just set the isIcon as part of that process?

Yes, this is weak place. And yes, we already have isIcon == true when we load icon by LOAD-*ICON. But what if we will use LOAD-IMAGE() or define image and just use "some/path/filename.ico" file. This means we working with image but really the data will be icon. And for these cases we can check the filename extension to have info about file data internals.

OK. I don't think this needs full runtime testing. The GUI stuff won't get loaded in MAJIC testing. Even the server-side Image*.java should not be loaded. I think this is safe enough to check in.

Have you run your testcases in both windev01 and in P2J? Does our implementation match exactly?

OK. I don't think this needs full runtime testing. The GUI stuff won't get loaded in MAJIC testing. Even the server-side Image*.java should not be loaded. I think this is safe enough to check in.

Agreed. I just would like to make conversion test. I've changed the rule file and want to make sure there will be no regression her. Is it OK?

Have you run your testcases in both windev01 and in P2J? Does our implementation match exactly?

Yes, I'm comparing windev01 and P2J versions layout for every test. There are some mismatches when the units are characters. For pixel based units the match is OK.

And I have found regression from recent size related option changes to the previously implemented button relates IMAGE-SIZE options. So need to debug and fix it.

I've changed the rule file and want to make sure there will be no regression her. Is it OK?

Good point. Yes, conversion testing is needed.

Eugenie Lyzenko wrote:

Have you run your testcases in both windev01 and in P2J? Does our implementation match exactly?

Yes, I'm comparing windev01 and P2J versions layout for every test. There are some mismatches when the units are characters. For pixel based units the match is OK.

And I have found regression from recent size related option changes to the previously implemented button relates IMAGE-SIZE options. So need to debug and fix it.

Please do fix both of these issues (character unit mismatches and the regression for IMAGE-SIZE options) as part of this task.

Please do fix both of these issues (character unit mismatches and the regression for IMAGE-SIZE options) as part of this task.

I have one question here. Looks like the character units mismatches are caused the fact we have different character width and height in pixels for windev01 and P2J. And I think this can be not always a bug. For different systems we have different font metrics. The only we can verify here is proportions, meaning coordinates in character units should correspond with total window size in character units. Moreover we can not provide for all possible systems to have exact character size in pixels, we can request the system to have 8x16 font but this will not guarantee the final font will be 8x16, we get the best match instead. What do you think about our strategy here?

The conversion testing completed, generated codes are identical. No regressions.

So the update is ready to be committed if we separate character size fix to another task.

Yes, go ahead and commit it.

Greg Shah wrote:

Yes, go ahead and commit it.

Committed in bzr as 10771.

The remaining thing I'm going to implement is having getters/setters for all attributes in image and button widgets that can be query set outside of the DEFINE IMAGE or DEFINE BUTTON statements:

CONVERT-3D-COLORS
STRETCH-TO-FIT
RETAIN-SHAPE
TRANSPARENT

For example:

...
bVar = button:convert-3d-colors.
button:convert-3d-colors = not bVar.
...

Code Review evl_upd20150223a.zip

The changes look good.

Normally, I would ask you to get these tested, but Igor has overlapping changes being tested in #2498 and he has had difficulty getting them through testing. I'd like to see if we can get his changes checked in before your update gets tested.

Thanks.

Normally, I would ask you to get these tested, but Igor has overlapping changes being tested in #2498 and he has had difficulty getting them through testing. I'd like to see if we can get his changes checked in before your update gets tested.

Thanks.

I see. Let's wait while Igor's changes become committed.

The only thing I see is to unify CONVERT-3D-COLORS attribute for button and image widgets. Currently button has NO-CONVERT-3D-COLORS option and uses it to support CONVERT-3D-COLORS attribute. Looks like it makes sense to have the attribute CONVERT-3D-COLORS for button too and use it to store current NO-CONVERT-3D-COLORS option as NO-CONVERT-3D-COLORS == !CONVERT-3D-COLORS. What do you think?

Eugenie Lyzenko wrote:

The only thing I see is to unify CONVERT-3D-COLORS attribute for button and image widgets. Currently button has NO-CONVERT-3D-COLORS option and uses it to support CONVERT-3D-COLORS attribute. Looks like it makes sense to have the attribute CONVERT-3D-COLORS for button too and use it to store current NO-CONVERT-3D-COLORS option as NO-CONVERT-3D-COLORS == !CONVERT-3D-COLORS. What do you think?

Agreed.

Code Review evl_upd20150224a.zip

Are ButtonGuiImpl.draw() and ImageGuiImpl.draw() really the proper locations to calculate location for a dynamic widget? I'm worried that:

• we will have to implement this separately for every widget, I'd prefer an approach that is entirely (or mostly) implemented in a base class
• this doesn't cover the ChUI case (for button)
• it seems like we may have other layout or state changes needed besides where we draw

Constantin/Hynek: please review and comment on the changes in ButtonGuiImpl and ImageGuiImpl and BaseEntity.

One more possible TODO change is rework for image phrase setters for button related images we have discussed previously. To have single setter for all Up, Down, Insensitive images and setters for offset(chars|pixels)/size(chars|pixels). Do we need this rework now?

Eugenie Lyzenko wrote:

One more possible TODO change is rework for image phrase setters for button related images we have discussed previously. To have single setter for all Up, Down, Insensitive images and setters for offset(chars|pixels)/size(chars|pixels). Do we need this rework now?

No, I don't want to do this now.

Greg Shah wrote:

Code Review evl_upd20150224a.zip

Are ButtonGuiImpl.draw() and ImageGuiImpl.draw() really the proper locations to calculate location for a dynamic widget? I'm worried that:

• we will have to implement this separately for every widget, I'd prefer an approach that is entirely (or mostly) implemented in a base class
• this doesn't cover the ChUI case (for button)
• it seems like we may have other layout or state changes needed besides where we draw

Constantin/Hynek: please review and comment on the changes in ButtonGuiImpl and ImageGuiImpl and BaseEntity.

Ideally the draw() method should not perform any positional logic. The COL/ROW attributes are synced with the client widget instance as part of BaseConfig.applyConfig() execution - eventually AbstractWidget.setLocation() is called and I think this is the location where the conversion to native/physical units should happen (and vice versa in AbstractWidget.setPhysicalLocation()). Then it should be enough to only call physicalLocation() in draw().

Is there anything special on the widget position when it is dynamic?

Code Review evl_upd20150225a.zip

I'm fine with the changes.

Hynek: please look at the BaseConfig change and confirm if it is OK.

Greg Shah wrote:

Code Review evl_upd20150225a.zip

I'm fine with the changes.

Hynek: please look at the BaseConfig change and confirm if it is OK.

I think it is good. Just a minor thing - the call to setPhysicalLocation in IGI.initialize should be unnecessary as you already set the physical location thanks to the change in BC.applyConfig.

the call to setPhysicalLocation in IGI.initialize should be unnecessary as you already set the physical location thanks to the change in BC.applyConfig.

Eugenie: please confirm that it is not needed. If so, please remove it. Then go ahead with both conversion and runtime regression testing.

The conversion testing completed. Codes are identical, no regression. Continue with the runtime testing.

The main part of the regression tests completed OK. Waiting for CTRL-C to be done.

Testing completed. The results file: 10778_5c96d7c_20150226_evl.zip.

So the update is ready to be committed and distributed.

Yes, please check it in.

The update evl_upd20150225b.zip has been committed in bzr as 10780.