FWD » Core Development » User Interface

Implement the following EDITOR widget methods (conversion and runtime):

SEARCH
MOVE-TO-EOF
EDIT-CLEAR
INSERT-FILE

This work is associated with #3257-11.

Code Review Task Branch 3279a Revision 11158

Generally, this is quite good.

1. The search literals should be implemented by a server-side class. Today they are imported from com.goldencode.p2j.ui.client.Editor.* which is a dependency that we never want to expose to the converted application code.

2. FillinWidget is missing a history entry.

3. Please search in the ui/ subdirectory structure for <true>true</true> and fix those javadoc issues.

4. The RadioSetWidget.invalidDeleteArg() used to use error number 4068 but now the replacement usage of invalidArgument() uses 4095. Which is correct?

5. The only change in ComboBoxImpl is a blank line added to the end. Please revert that file.

6. In ThinClient.removeEditSelection(), I would prefer all these widgets to implement a common interface for removeSelectedText() so that the TC doesn't have to encode widget-specific behavior using instanceof. I know there is quite a bit of that already there. I'm trying to reduce that usage.

7. In Editor, history entries 054 and 055 should be merged together.

Greg Shah wrote:

Code Review Task Branch 3279a Revision 11158
Generally, this is quite good.

Thank you for the review. I updated the branch 3279a a couple of days ago with requested changes. They are described below, along with some comments/answers:

1. The search literals should be implemented by a server-side class. Today they are imported from com.goldencode.p2j.ui.client.Editor.* which is a dependency that we never want to expose to the converted application code.

The constants were moved to server-side class EditorWidget.

2. FillinWidget is missing a history entry.

Added.

3. Please search in the ui/ subdirectory structure for <true>true</true> and fix those javadoc issues.

Done.

4. The RadioSetWidget.invalidDeleteArg() used to use error number 4068 but now the replacement usage of invalidArgument() uses 4095. Which is correct?

From my tests, 4095. Here is the 'syntax' from ABL help:
**The radio button in the <attribute> attribute is invalid for <widget id>. (4095)
The other error message looks like this:
**The third argument for <attribute> on the <widget id> should not be of type <arg type>. (4068)
I guess this is wrong, attempting to provide more that one argument to delete will result in compile error 3407 instead:
Invalid number of arguments for attribute DELETE.

5. The only change in ComboBoxImpl is a blank line added to the end. Please revert that file.

Done.

6. In ThinClient.removeEditSelection(), I would prefer all these widgets to implement a common interface for removeSelectedText() so that the TC doesn't have to encode widget-specific behavior using instanceof. I know there is quite a bit of that already there. I'm trying to reduce that usage.

Added the new interface com.goldencode.p2j.ui.client.TextSelection. It groups basic selection operations: select, invalidate, removeSelectedText. It is implemented by editors, fill-ins and comboboxes.

7. In Editor, history entries 054 and 055 should be merged together.

There are a few files with history entries that 'skip' a number. This is because I estimated that 2676a branch will be merged to trunk before this and allow a simpler merging.

Code Review Task Branch 3279a Revision 11160

1. The SUB-TYPE members added to ComboBoxWidget need javadoc.

2. In ComboBoxWidget.setSubType(character value), should null be passed also in the case where value.isUnknown() is true?

3. ThinClient.insertEditorFile() should use the existing FSD instance (fsd) instead of creating a new one. It is meant to be singleton-like.

4. EditorGuiImpl line 643 and lines 714-716 need to be indented.

5. How confident are you in the combo-box drawing changes?

The task branch 3279a was rebased and updated. Current revision 11165.

Greg Shah wrote:

Code Review Task Branch 3279a Revision 11160
1. The SUB-TYPE members added to ComboBoxWidget need javadoc.
2. In ComboBoxWidget.setSubType(character value), should null be passed also in the case where value.isUnknown() is true?
3. ThinClient.insertEditorFile() should use the existing FSD instance (fsd) instead of creating a new one. It is meant to be singleton-like.
4. EditorGuiImpl line 643 and lines 714-716 need to be indented.

All done.

5. How confident are you in the combo-box drawing changes?

The most affected is the SIMPLE combo-box, which is badly drawn in current trunk revision. As far as I can see, there are no occurrences of SIMPLE sub-type of widget in any of customer's code. I added some tests in testcases/uast/gui/fillin-and-combos.p.

Please rebase the branch.

Eugenie: after the rebase, please do a code review. Focus on the combo-box changes.

Greg Shah wrote:

Eugenie: after the rebase, please do a code review. Focus on the combo-box changes.

OK.

Revision 11165 above is based on trunk 11155.

Greg Shah wrote:

Eugenie: after the rebase, please do a code review. Focus on the combo-box changes.

I have two notes for combo-box GUI changes:
1. What about SIMPLE mode selection list border painting? I have not found it in 3279a rev 11165.
2. The entry field height is now not taking into account 3D style layout. This potentially can produce wrong height calculations for classic theme 3D frames. Was this change necessary?

Also some code was commented out but left inside the branch. I think if it is not TODO work - we need to clean up them.

There are the set of testcases for combo-box simple mode. I guess they must be run to verify there are no regressions with new changes.

I found two testcases with SIMPLE comboboxes. After a few tests, the UI is just fine, the only differences are the fonts and subtle color shades. What I've seen wrong is the SCREEN-VALUE in combo_box20.p, where FWD prints only the first character in the case of FORMAT "x(1)" combo-box.
I will use the legacy fonts and enhance the test with 3D tests and do pixel-based comparisons.

Ovidiu Maxiniuc wrote:

I found two testcases with SIMPLE comboboxes. After a few tests, the UI is just fine, the only differences are the fonts and subtle color shades.

Great!

What I've seen wrong is the SCREEN-VALUE in combo_box20.p, where FWD prints only the first character in the case of FORMAT "x(1)" combo-box.

This is certainly the regression from some moment. Yes, the 4GL ignores the FORMAT option in this case. This feature was implemented in 10959 trunk, with committing 2832a branch. As of 12/13/2015.

Code Review Task Branch 3279a Revision 11167

I'm fine with the changes.

Unless Eugenie has any objections, I'm OK with you merging this to trunk when you have completed sufficient testing.

Greg Shah wrote:

Code Review Task Branch 3279a Revision 11167

I'm fine with the changes.

Unless Eugenie has any objections, I'm OK with you merging this to trunk when you have completed sufficient testing.

I'm OK with changes. Just to make sure the FORMAT "x(1)" regression is not starting with this branch. We need to document and fix the regression somewhere else if it is in trunk as well.

Eugenie Lyzenko wrote:

I'm OK with changes. Just to make sure the FORMAT "x(1)" regression is not starting with this branch. We need to document and fix the regression somewhere else if it is in trunk as well.

From my tests, the regression is in trunk; it is fixed in 3279a/11167. The regression test on devsrv01 is clean. Where should this fix documented?

Ovidiu Maxiniuc wrote:

Eugenie Lyzenko wrote:

I'm OK with changes. Just to make sure the FORMAT "x(1)" regression is not starting with this branch. We need to document and fix the regression somewhere else if it is in trunk as well.

From my tests, the regression is in trunk; it is fixed in 3279a/11167. The regression tests on devsrv01 is clean. Where should this fix documented?

I think if the regression is fixed the notes already done is good enough documentation for the fix and you do not need to make additional steps.

Greg, do you agree?

Agreed.

I added uast/combo_box/combo_box20_2.p as revision 1626 in testcases. Use this to see the regression in both GUI and ChUI. They are a little bit different in trunk 11155: the ChUI should take the format into consideration for all subtypes of comboboxes, while in GUI clients only the DROP-DOWN-LIST should do that.

Task branch 3279a was committed to trunk as revision 11156.
The test-results and task branch were archived. Standard notification message was sent to team members.

I will add the gap marking updates to 1514a (I forgot to ask you to make these).

I plan to mark INSERT-FILE(), MOVE-TO-EOF() and SEARCH() as fully supported in both conversion and runtime.

In regard to EDIT-CLEAR(), I think it is fully supported for conversion and runtime in editor, combo-box and fill-in. Correct?

ScrollableSelectionListGuiImpl is also missing a history entry. Please put it in 1514a after we rebase that branch.

1514a rev 11390 contains the gap marking changes.