FWD » Core Development » User Interface

In SIMPLE mode ComboBoxGuiImpl creates multiple sibling widgets (a FillIn entry field and a selection list) on the same level of widget tree hierarchy. These widgets are physically disconnected, but logically belong to the combo-box widget. ComboBoxGuiImpl attempts to manage these disconnected widgets as one logical piece but it brakes on other places where this structure is rightfully unexpected. An example is ENABLE ALL statement where the number of focusable widgets is greater than the expected. Other places where this breaks is during frame layout, size reporting, etc.

The proper solution would be (at least for the SIMPLE mode) to have the combo-box widget class inherit from AbstractContainer and attach all the individual widgets (entry field and selection list) to the container.

DROP-DOWN mode is affected in the same way. In DROP-DOWN mode the fill-in entry field is used the same way as in SIMPLE.

The investigation shows the SIMPLE mode internal logic is almost all broken. I've recovered most of all but not yet completed.

My plan is first to recover complete functionality that worked before. Then we can think whether we need to completely rework the combo-box GUI mode widget to be a kind of AbstractContainer or not.

Eugenie Lyzenko wrote:

The investigation shows the SIMPLE mode internal logic is almost all broken. I've recovered most of all but not yet completed.

My plan is first to recover complete functionality that worked before. Then we can think whether we need to completely rework the combo-box GUI mode widget to be a kind of AbstractContainer or not.

Eugenie, if you recover the original design, the implementation will be still broken. Multiple places in the framework do not expect one widget to span multiple Widget implementations. There will be abends and unexpected behaviors.

Hynek Cihlar wrote:

Eugenie, if you recover the original design, the implementation will be still broken. Multiple places in the framework do not expect one widget to span multiple Widget implementations. There will be abends and unexpected behaviors.

I do not expect everything will be working fine after recover. But to estimate the efforts I need to exactly know what and where is wrong and to select the best approach to implement. Moreover the FillInGuiImpl has some changes that out of the scope of this point and are the issues itself. We will have to fix it anyway for every class that will be inherited from FillInGuiImpl.

The task 2832a has been created based on trunk revision 10956.

The task branch 2832a updated for review at revision 10957.

The combo-box functionality restored in SIMPLE mode. There are some remaining GUI issues, working on, so the code is not cleaned from comments. But the basic implementation is here. Yes, there are two issues with this approach.
1. Early painting causes NPE.
2. Size calculation to be used by layout manager is dependent on the mode the combo-box is running. The SIMPLE mode has special approach, where inner-lines attribute is ignoring. We have to always set the size explicitly(4GL does not allow auto calculation based on inner-lines attribute).

So all two above issues are now fixed. So if there are no objection I would prefer to stay with this implementation to not over-complicate the widget. Or I need to know the cases when this approach will fail. Using AbstractContainer gives the same issues to resolve I guess.

Rebased task branch 2832a from P2J trunk revision 10957, new branch revision is 10958.

Eugenie Lyzenko wrote:

Or I need to know the cases when this approach will fail.

As an example, you can test the ENABLE ALL case as mentioned in note 1. Create a frame with a combo box in simple mode and use ENABLE ALL on the frame.

I checked the problematic cases and the SIMPLE mode works fine.

I just noticed an abend when a simple-mode selection list item is clicked, see the exception callstack below. This can be reproduced in demo/demo_widgets.p, just convert the combo-box there to simple mode.

Thread [main] (Suspended (exception StringIndexOutOfBoundsException))    
    String.substring(int, int) line: 1951    
    EntryFieldGuiImpl(FillInGuiImpl).drawSelectionInt(String, int, ColorRgb, ColorRgb) line: 1012    
    EntryFieldGuiImpl.drawSelection(String, int, ColorRgb, ColorRgb) line: 302    
    FillInGuiImpl$2$2.run() line: 869    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    FillInGuiImpl$2.run() line: 857    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    EntryFieldGuiImpl(FillInGuiImpl).draw(Color) line: 804    
    EntryFieldGuiImpl(FillIn<O,C>).draw() line: 1074    
    SensitiveScrollContainer<O>(AbstractContainer<O>).draw() line: 410    
    Viewport<O>.draw() line: 64    
    BorderedPanelGuiImpl(AbstractContainer<O>).draw() line: 410    
    BorderedPanelGuiImpl.access$4(BorderedPanelGuiImpl) line: 1    
    BorderedPanelGuiImpl$1$1.run() line: 200    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    BorderedPanelGuiImpl$1.run() line: 189    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    BorderedPanelGuiImpl.draw() line: 136    
    ScrollPaneGuiImpl$2.run() line: 159    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    ScrollPaneGuiImpl.draw() line: 150    
    FrameGuiImpl$2.run() line: 443    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    FrameGuiImpl.draw(Point, Dimension) line: 429    
    FrameGuiImpl(Frame<O>).draw() line: 1903    
    NativeScrollContainer<O>(AbstractContainer<O>).draw() line: 410    
    Viewport<O>.draw() line: 64    
    BorderedPanelGuiImpl(AbstractContainer<O>).draw() line: 410    
    BorderedPanelGuiImpl.access$4(BorderedPanelGuiImpl) line: 1    
    BorderedPanelGuiImpl$1$1.run() line: 200    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    BorderedPanelGuiImpl$1.run() line: 189    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    BorderedPanelGuiImpl.draw() line: 136    
    ScrollPaneGuiImpl$2.run() line: 159    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    WindowWorkSpace(ScrollPaneGuiImpl).draw() line: 150    
    WindowWorkSpace.draw() line: 87    
    BorderedPanelGuiImpl(AbstractContainer<O>).draw() line: 410    
    BorderedPanelGuiImpl.access$4(BorderedPanelGuiImpl) line: 1    
    BorderedPanelGuiImpl$1$1.run() line: 200    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    BorderedPanelGuiImpl$1.run() line: 189    
    SwingGuiDriver(AbstractGuiDriver<F>).draw(NativePoint, NativeRectangle, Runnable) line: 1988    
    BorderedPanelGuiImpl.draw() line: 136    
    WindowGuiImpl(Window<O>).draw() line: 1373    
    WindowGuiImpl.draw() line: 469    
    GuiOutputManager(OutputManager<P>).setInvalidate(Widget<?>, boolean, boolean) line: 1275    
    ThinClient.eventDrawingBracket(Widget<?>, boolean, boolean, Runnable) line: 13840    
    MouseHandler.handleMouseEvent(int, MouseEvent) line: 248    
    SwingGuiDriver(AbstractGuiDriver<F>).handleMouseEvent(int, MouseEvent) line: 2393    
    WindowGuiImpl(TopLevelWindow<O>).processEvent(Event) line: 669    
    WindowGuiImpl.processEvent(Event) line: 1315    

Hopefully the non-standard widget structure will not cause more problems in the future.

Task branch 2832a for review updated to revision 10961.

This is the fix for StringIndexOutOfBoundsException recently reported by Hynek. We need to use screen value to detect the last editable char, not value itself because it can have padding spaces to match the format value.

Task branch 2832a for review updated to revision 10962.

Several fixes to improve EntryField text painting to match 4GL. So far the implementation for SIMPLE and DROP-DOWN modes completed and if no objection it is ready to be merged into trunk.

Eugenie Lyzenko wrote:

Task branch 2832a for review updated to revision 10962.

Several fixes to improve EntryField text painting to match 4GL. So far the implementation for SIMPLE and DROP-DOWN modes completed and if no objection it is ready to be merged into trunk.

The code looks ok to me.

Only small thing. I noticed the number of visible items in the simple list differs from 4GL. Is this expected?

Hynek Cihlar wrote:

The code looks ok to me.

Only small thing. I noticed the number of visible items in the simple list differs from 4GL. Is this expected?

What the test that shows this diff? Can you provide the both screens here for me to see this?

Only small thing. I noticed the number of visible items in the simple list differs from 4GL. Is this expected?

In SIMPLE mode the number of visible items defined by comb-box size and used font. The INNER-LINES attribute is ignoring. So I need to know particular condition for the deviation you see to tell if this is expected or not.

Eugenie Lyzenko wrote:

Only small thing. I noticed the number of visible items in the simple list differs from 4GL. Is this expected?

In SIMPLE mode the number of visible items defined by comb-box size and used font. The INNER-LINES attribute is ignoring. So I need to know particular condition for the deviation you see to tell if this is expected or not.

Try the following code sample.

DEF VAR cb AS CHAR FORMAT "x(8)" VIEW-AS COMBO-BOX simple LIST-ITEMS 
   "item1", "item2", "item3", "item4", "item5" size 10 by 5.

ENABLE cb WITH FRAME f SIDE-LABELS.
WAIT-FOR GO OF FRAME f.

Different sizes produce various size differences between P2J and 4GL.

Another issue I just noted is that 4GL seems to ignore the format specification for COMBO-BOX. Although this is not directly related to SIMPLE-LIST, it does affect it. Try the following code sample.

DEF VAR cb AS CHAR FORMAT "x(1)" VIEW-AS COMBO-BOX simple LIST-ITEMS 
   "item1", "item2", "item3", "item4", "item5" size 10 by 5.

ENABLE cb WITH FRAME f SIDE-LABELS.
WAIT-FOR GO OF FRAME f.

Hynek Cihlar wrote:

[...]

Different sizes produce various size differences between P2J and 4GL.

Have you used the USE-3D-SIZE global option as "yes" or "no" in 4gl? In p2j?

Eugenie Lyzenko wrote:

Hynek Cihlar wrote:

[...]

Different sizes produce various size differences between P2J and 4GL.

Have you used the USE-3D-SIZE global option as "yes" or "no" in 4gl? In p2j?

Tested with USE-3D-SIZE set to yes for both 4GL and P2J.

Eugenie Lyzenko wrote:

Is it the difference you noted?

Yes. Let's take the combo 'cb' from the screen shots. While in P2J the font height in the simple list seems to be greater than in 4GL, the height of the P2J simple list is smaller than in 4GL. I would expect the height of the simple list in P2J proportionally greater.

Yes. Let's take the combo 'cb' from the screen shots. While in P2J the font height in the simple list seems to be greater than in 4GL, the height of the P2J simple list is smaller than in 4GL.

I think the font size is the same for both p2j and 4gl, check the height in pixels for both cases.

I would expect the height of the simple list in P2J proportionally greater.

The problem I see here is the list portion of the P2J test is really smaller than in 4GL. The interesting is the real size for 4GL testcases is one pixel smaller than in HEIGHT-PIXELS attribute. So there is a weird widget height calculation I need to find out to eliminate this change.

The only thing that constant is the height in chars and height in pixels(that is computed from char->pixel conversion). And visible part of the widget cannot be more than height in pixels(but can be smaller, 1 or even 4 pixels).

Eugenie Lyzenko wrote:

The only thing that constant is the height in chars and height in pixels(that is computed from char->pixel conversion).

This is because the conversion uses SESSION:PIXELS-PER-ROW/SESSION:PIXELS-PER-COLUMN.

Eugenie Lyzenko wrote:

Task branch 2832a for review updated to revision 10963.

This is the fix for visible items mismatch. However new issue discovered with this change. Looking at the picture attached the scrollbar is wrong when the visible items are smaller than total. This is caused by the ability to have partial visible item in the list. But scrollbar is defined by visible rows/total rows data. Need to find out more precise pixel based approach to compute scrolling for this case. Continue working.

IMHO the simplest solution is to change the scrolling unit from lines to pixel-line-height for ScrollableSelectionListGuiImpl. So instead of increment/decrement scroll position by 1 it would be the pixel height of one line. I think it should be enough to override getScrollDimension(), getVisibleDimension() and getStep() in ScrollableSelectionListGuiImpl and to convert the pixel value to rowTop in ScrollableSelectionListGuiImpl.scroll() as y-position/pixel-line-height.

Task branch 2832a for review updated to revision 10964.

Fixes for partial item drawing. Unfortunately approach you mentioned(and really what I thought about too) to change to pixels does not work good enough. We need the item row logic to draw the list and scrolling. The alternative can be the change scroll dimension related api return values from NativeRectangle(int, int) to Dimension(double, double) and keep measure everything in char units.

Let me know if this update has objections, I'll think for how to rework if this approach is not acceptable.

Eugenie Lyzenko wrote:

Task branch 2832a for review updated to revision 10964.

Fixes for partial item drawing. Unfortunately approach you mentioned(and really what I thought about too) to change to pixels does not work good enough. We need the item row logic to draw the list and scrolling.

By "item row logic", do you mean that the scroll step should be the equivalent of one row? That is, one click on the scroll bar button should move the scrollable content by one row? If so, this can be achieved even with the pixel units by setting the scroll step (the return value of the overriden method getStep()) to the pixel height of one row.

Let me know if this update has objections, I'll think for how to rework if this approach is not acceptable.

In ScrollableSelectionListGuiImpl line 179, why not to set the clipping rectangle height to the actual visible height?

Hynek Cihlar wrote:

Let me know if this update has objections, I'll think for how to rework if this approach is not acceptable.

In ScrollableSelectionListGuiImpl line 179, why not to set the clipping rectangle height to the actual visible height?

Because this is not common case. The ScrollableSelectionListGuiImpl is used to draw DROP-DOWN-* combo-box modes when this will not work.

Task branch 2832a for review updated to revision 10965.

This is the fix for format ignoring issue for combo-box widget. Now the format is auto detected from provided item list initials.

Task branch 2832a for review updated to revision 10966.

The fix for recent note. Also I have found the format ignorance is limited to GUI mode for SIMPLE and DROP-DOWN combo boxes. Respective changes was added to use this option only when it is really required. Also I did some investigation and conclude the Windows 4gl also uses line based scrolling, not pixel one.

So I consider this update as release candidate for current task if there are no objections.

Eugenie Lyzenko wrote:

Task branch 2832a for review updated to revision 10966.

The fix for recent note. Also I have found the format ignorance is limited to GUI mode for SIMPLE and DROP-DOWN combo boxes. Respective changes was added to use this option only when it is really required. Also I did some investigation and conclude the Windows 4gl also uses line based scrolling, not pixel one.

So I consider this update as release candidate for current task if there are no objections.

No objections here.

Code Review Task Branch 2832a Revision 10966

I'm good with the changes.

Constantin: do you have any concerns with the FILL-IN related changes?

If Constantin is OK, then please put this into runtime regression testing.

I agree that it would be optimal to move to an AbstractContainer approach at some point but considering our time constraints I think the current approach is OK.

Rebased task branch 2832a from P2J trunk revision 10958, new branch revision is 10967.

Constantin,

If you have no objections I will start runtime testing for 10967?

Eugenie Lyzenko wrote:

Rebased task branch 2832a from P2J trunk revision 10958, new branch revision is 10967.

Constantin,

If you have no objections I will start runtime testing for 10967?

I'm OK with the fillin related changes, but I have a note about ComboBoxGuiImpl.setEnabled - I think you should repaint only and only if the current widget state is not enabled... otherwise the widget will be repainted even if it is already enabled.

Eugenie: please go ahead and make this fix and then go into testing.

Task branch 2832a for review updated to revision 10968.

Added conditional repaint when changing widget enable state to eliminate extra paintings. Is it OK now?

Eugenie Lyzenko wrote:

Task branch 2832a for review updated to revision 10968.

Added conditional repaint when changing widget enable state to eliminate extra paintings. Is it OK now?

Yes, you can start testing.

The testing is in progress, main part has been started.

Testing completed the results: 2832a_10968_de19d84_20151213_evl.zip. No regressions, the TC-CODES-EMPLOYEES-021 and CTRL-C 3-way tests were executed in standalone mode to confirm test passing. The server stopped, starting merge 2832a into trunk.

Branch 2832a was merged to trunk as revno 10959 then it was archived.