FWD » Core Development » User Interface

The focus logic doesn't always focus the expected widget when window is activated. To reproduce on create multiple windows with focusable widgets and switch between the windows with the keyboard, try this with a the default window visible or hidden. This issue has been noted in Swing GUI client.

The problem is that part of the focus logic is executed synchronously right after an input event leading to window switch is processed and the other part of focus logic is processed asynchronously after the application logic receives window activated event from the gui driver. And so in some cases focus is transferred back and forth between top-level windows multiple times.

A pitty is that after the window activation fixes in #2875/2875a the problem gets even more apparent - as the windows now properly reflect the activation state their title bars flash during the focus exchange.

#2958 is the same problem (there is a recreate there that can be useful).

Planning to rebase 1811t from latest trunk.

Misspelled history number was found in the current trunc in this module src/com/goldencode/p2j/ui/client/gui/driver/web/res/p2j.screen.js

** 009 HC  20151213 Improved DIALOG-BOX window parenting logic and related changes.
** 009 IAS 20151214 New cursor types added.
**                  LOAD-MOUSE-POINTER ans SET-WAIT-STATE support

** 014 CA  20151126 When waiting for a message result to be received from the JS side, each message
**                  must be uniquely identified; otherwise, if the same message type is sent in
**                  the same batch more than once, it will collide and it will not preserve the
**                  message result.
** 014 IAS 20160217 LOAD-MOUSE-POINTER ans SET-WAIT-STATE support

According to this diff from the trunk bzr diff -c10976 > 10976.txt

=== modified file 'src/com/goldencode/p2j/ui/client/gui/driver/web/res/p2j.screen.js'
--- src/com/goldencode/p2j/ui/client/gui/driver/web/res/p2j.screen.js    2016-01-04 22:15:15 +0000
+++ src/com/goldencode/p2j/ui/client/gui/driver/web/res/p2j.screen.js    2016-03-01 15:32:06 +0000
@@ -31,6 +31,8 @@
 ** 008 SBI 20151124 Fixed cleanup() to remove the corresponding window canvas from DOM.
 **     SBI 20151201 Fixed two times clicks for menus.
 ** 009 HC  20151213 Improved DIALOG-BOX window parenting logic and related changes.
+** 009 IAS 20151214 New cursor types added.
+**                  LOAD-MOUSE-POINTER ans SET-WAIT-STATE support
 */

 ** 009 HC  20151213 Improved DIALOG-BOX window parenting logic and related changes.
 ** 010 IAS 20151214 New cursor types added.
 **                  LOAD-MOUSE-POINTER ans SET-WAIT-STATE support

Yes, it should be fixed.

Igor, could you help I found this code in the trunc src/com/goldencode/p2j/ui/client/driver/web/res/p2j.socket.js (please look at string "theWindow.deregisterMouseWidget(id);")

                  case 0xA0:
//                     console.log("deregisterWidget");
                     var offset = 1;
                     var wid = me.readInt32BinaryMessage(message, offset);
                     offset += 4;
                     var id = me.readInt32BinaryMessage(message, offset);
                     var theWindow = p2j.screen.getWindow(wid);
                     if (theWindow)
                     {
                        theWindow.deregisterMouseWidget(id);
                     }
                     else 
                     {
                        console.log("unknown window: %s", wid);
                     }
                     break;

Window.prototype.deregisterMouseWidget = function(wid, retainEditable)

Sergey Ivanovskiy wrote:

Igor, could you help I found this code in the trunc src/com/goldencode/p2j/ui/client/driver/web/res/p2j.socket.js (please look at string "theWindow.deregisterMouseWidget(id);")
[...]
according to definition
[...]
this code will be equivalent to theWindow.deregisterMouseWidget(id, false);
Is it okay?

Yes, it is OK.

Now 1811t grows from the current trunc. Encountered trivial history conflicts and the WebClientMessageTypes changes required to assign a new messages id for MSG_IS_FONT_INSTALLED and MSG_REMOVE_EXPIRED_HASH respectively.

Please, don't update from 1811t. After rebase the web client doesn't work. Investigating the reasons. The swing client with hello.p works properly.

Sorry, it is due to messages constants.

Committed revision 11050. Now 1811t is safe to use. I think that such kind of errors can be eliminated if names are used instead of numeric constants in p2j.socket.js.

I think that such kind of errors can be eliminated if names are used instead of numeric constants in p2j.socket.js.

I completely agree, please make this change. It also will read better.

Planning to do it. Found a regression in ./demo/demo_widgets.p (com.goldencode.testcases.demo.DemoWidgets.execute) the editor doesn't scroll its content horizontally by the mouse, but it can scroll vertically.

Sergey Ivanovskiy wrote:

Found a regression in ./demo/demo_widgets.p (com.goldencode.testcases.demo.DemoWidgets.execute) the editor doesn't scroll its content horizontally by the mouse, but it can scroll vertically.

I you sure that it is a regression? I had the issue with editor and the problem which I've found and fixed (too early MOVE event consumption) was not depended on the direction.

Yes, it is a regression in 1811t. The complexity here appears due to the GUI classes hierarchy of ScrollPane and ScrollBar and Editor. Mouse widget regions on the JS client side have some z-order and there are vertical and horizontal scroll bars, editor and scroll pane. May be I will fix it and will think about how to improve it or to do clear.

Sergey Ivanovskiy wrote:

Yes, it is a regression in 1811t. The complexity here appears due to the GUI classes hierarchy of ScrollPane and ScrollBar and Editor. Mouse widget regions on the JS client side have some z-order and there are vertical and horizontal scroll bars, editor and scroll pane. May be I will fix and will think about how to improve it or to do clear.

Oh, I see. The most important changes I've made are in p2.mouse.processMouseMove() method. They can be incompatible with 1811t.

Ok, the previous 1811t code have able to detect if the mouse is over the scroll bars but now all events are delivered to EditorGuiImpl. That is the difference. Planning to investigate the reasons why scroll bars don't receive mouse events. (may be z-order is changed or scroll bars are not registered).

Sergey Ivanovskiy wrote:

Ok, the previous 1811t code have able to detect if the mouse is over the scroll bars but now all events are delivered to EditorGuiImpl. That is the difference. Planning to investigate the reasons why scroll bars don't receive mouse events. (may be z-order is changed or scroll bars are not registered).

If I roll back some changes, then the scroll bars are Ok. Another issue is the focus does't move to the target widget if the mouse is over it. It is also a regression
I think the code should be that

=== modified file 'src/com/goldencode/p2j/ui/client/gui/driver/web/WebMouseHandler.java'
--- src/com/goldencode/p2j/ui/client/gui/driver/web/WebMouseHandler.java    2016-01-04 08:14:16 +0000
+++ src/com/goldencode/p2j/ui/client/gui/driver/web/WebMouseHandler.java    2016-03-03 04:54:26 +0000
@@ -63,7 +63,7 @@
             }
             else
             {
-               Widget editor = WidgetRegistry.findAncestor(w, EditorGuiImpl.class);
+               Widget editor = WidgetRegistry.findAncestor(target, EditorGuiImpl.class);
                if (editor != null && editor.getId() != null)
                {
                   widgetId = editor.getId().asInt();

=== modified file 'src/com/goldencode/p2j/ui/client/widget/AbstractContainer.java'
--- src/com/goldencode/p2j/ui/client/widget/AbstractContainer.java    2016-02-10 19:24:23 +0000
+++ src/com/goldencode/p2j/ui/client/widget/AbstractContainer.java    2016-03-03 04:53:51 +0000
@@ -361,8 +361,7 @@
             continue;
          }

-         return (found != null && found.getId() != null) ? found : 
-               (w.getId() == null ? null : w); 
+         return (found == null) ? w : found;
       }

       return null;

To be more concrete please look at these changes and the function definition

   /**
    * Find the widget positioned just bellow the specified mouse position (in physical units).
    * Each container should search for own widgets. Because widgets are relative positioned 
    * inside containers the container physical position is adjusted before search.  
    * 
    * @param    p
    *           The mouse physical location.
    * 
    * @return   A widget over which the mouse is positioned or null if none found.
    */
   @Override
   public Widget<O> findMouseSource(NativePoint p)

=== modified file 'src/com/goldencode/p2j/ui/client/widget/AbstractContainer.java'
--- src/com/goldencode/p2j/ui/client/widget/AbstractContainer.java    2016-02-10 19:24:23 +0000
+++ src/com/goldencode/p2j/ui/client/widget/AbstractContainer.java    2016-03-03 04:53:51 +0000
@@ -361,8 +361,7 @@
             continue;
          }

-         return (found != null && found.getId() != null) ? found : 
-               (w.getId() == null ? null : w); 
+         return (found == null) ? w : found;
       }

       return null;

Sergey Ivanovskiy wrote:

To be more concrete please look at these changes and the function definition
[...]
[...]
After the original code has been changed, this function becomes to return only widgets having id. Is it correct?

W/o this change there where problems with mouse cursor support for the COMBO-BOX. In the initial version in the code there was check for non-null id for the w but not for found (#2565 note 159) which looked a little bit strange. As I have mentioned in #2565 note 310 the whole approach for finding mouse source based on the positive/negative/absent id looks fragile. It is also looks suspicious that the mouse events dispatching is performed in 3 different places based on two approaches which are not interchangeable (#2565 notes234, 236, 237)

Igor Skornyakov wrote:

Sergey Ivanovskiy wrote:

To be more concrete please look at these changes and the function definition
[...]
[...]
After the original code has been changed, this function becomes to return only widgets having id. Is it correct?

W/o this change there where problems with mouse cursor support for the COMBO-BOX. In the initial version in the code there was check for non-null id for the w but not for found (#2565 note 159) which looked a little bit strange. As I have mentioned in #2565 note 310 the whole approach for finding mouse source based on the positive/negative/absent id looks fragile. It is also looks suspicious that the mouse events dispatching is performed in 3 different places based on two approaches which are not interchangeable (#2565 notes234, 236, 237)

I don't understand what is AbstractContainer.findMouseSource. I need the answer to fix its regression. It depends on the definition AbstractContainer.findMouseSource.
May be it needs to extend this function to return the target mouse source and to reject targets that have no ids.

Sergey Ivanovskiy wrote:

I don't understand what is AbstractContainer.findMouseSource. I need the answer to fix its regression. It depends on the definition AbstractContainer.findMouseSource.
May be it needs to extend this function to return the target mouse source and to reject targets that have no ids.

Sorry, I do not understand the logic good enough to be able to provide a formal semantics definition. May be Constantin can do this. Please note also that some widgets (including the EditorGuiImpl) override this method which makes the situation even more complicated.

Igor Skornyakov wrote:

Sergey Ivanovskiy wrote:

I don't understand what is AbstractContainer.findMouseSource. I need the answer to fix its regression. It depends on the definition AbstractContainer.findMouseSource.
May be it needs to extend this function to return the target mouse source and to reject targets that have no ids.

Sorry, I do not understand the logic good enough to be able to provide a formal semantics definition. May be Constantin can do this. Please note also that some widgets (including the EditorGuiImpl) override this method which makes the situation even more complicated.

Understand. Thank you. Constantin, could you help to explain the issue in note 23? It is important to fix its regression.

I need to extend its definition as

   /**
    * Find the widget positioned just bellow the specified mouse position (in physical units).
    * Each container should search for own widgets. Because widgets are relative positioned 
    * inside containers the container physical position is adjusted before search.  
    * 
    * @param    p
    *           The mouse physical location.
    * @param    registeredWidgetsOnly
    *           The search filter indicates the mouse source mouse must have an id.
    * 
    * @return   A widget over which the mouse is positioned or null if none found.
    */
   @Override
   public Widget<O> findMouseSource(NativePoint p, boolean registeredWidgetsOnly)

Sergey Ivanovskiy wrote:

Constantin, could you help to explain the issue in note 23? It is important to fix its regression.

As Igor mentioned previously, the changes were needed to be able to identify widgets with a valid ID, so the mouse event (incoming from the driver) will use the widget which was registered some other custom actions, at the driver level (these needs a widget ID to be registered).

Previously, before we enhanced the driver mouse actions, there were no dependencies if the resolved widget has an ID or not. Adding that registeredWidgetsOnly (and calling it only from places which require a widget ID) looks OK, but I can't tell if it doesn't break the load-mouse-pointer support added by Igor.

Code Review Task Branch 1811t Revision 11052

I don't see the need to expose all of these constants as public data. Can't we just use the same approach as in p2j.screen.js for the private ops variable? It keeps it simple, hidden but still provides symbols instead of hexidecimal values.

To be clear, I'm suggesting that all the constants should be in a private var inside p2j.sockets.js.

Greg Shah wrote:

To be clear, I'm suggesting that all the constants should be in a private var inside p2j.sockets.js.

If it would be a private variable, then the other modules couldn't use it. Is it correct?

Yes, correct. I don't think the other modules need to use it. The message types should be hidden inside p2j.sockets.js, otherwise we are hard coding message type dependencies in other modules which is not a good practice.

Tests to check the focus management are #1798-32.

Found that labels in frames are not registered as widgets but have unique ids. In this program "A" if the mouse pointer is over i1 or str1 side labels, then this kind of messages appears in the Swing client logs

Received mouse event for zombie source -91: java.awt.event.MouseEvent[MOUSE_EXITED,(83,55),absolute(920,158),button=0,clickCount=0] on com.goldencode.p2j.ui.client.gui.driver.swing.SwingEmulatedWindow$1[,0,0,408x541,alignmentX=0.0,alignmentY=0.0,border=,flags=0,maximumSize=java.awt.Dimension[width=408,height=541],minimumSize=java.awt.Dimension[width=408,height=541],preferredSize=java.awt.Dimension[width=408,height=541]]

def var h1 as handle.

create window h1 assign title = "h1".

def var i1   as int label "Number".
def var str1 as char label "Text".

def button ok1 label "ok".

form i1 skip str1 skip ok1 with frame f1 title "fh1".

enable i1 str1 ok1 with frame f1 in window h1.

current-window = h1.

view h1.

wait-for close of current-window.

The common issue (Swing and Web clients) with the focus_order.p is the top "h2" window doesn't highlight its title bar.

The note 37 has been updated.

Sergey Ivanovskiy wrote:

The attached program doesn't work correctly with "shift + tab" in the web and swing clients. Planning to investigate the issue.
For the Swing client if we move the current focus back using "shift + tab", then the current window is collapsed and then is restored again.

The root cause is that the current focus is moved out from the Caption button and this code FocusManager.focusChange(Widget widget, boolean afterTab, boolean direction) applied to the Caption Button widget

      while (true)
      {
         Frame frame;
         if (test.config().frameId == -1)
         {
            if (test.parent() instanceof Browse ||
                test.parent() instanceof Menu || 
                test.parent() instanceof SubMenu)
            {
               // widget attached to inner browse
               possibleFocus = test.parent().hasFocusable(test);
               break;
            }
            else if (test instanceof Frame)
            {
               frame = (Frame) test;
            }
            else
            {
               throw new IllegalStateException("Could not resolve frame from widget " + widget);
            }
         }
         else
         {
            frame = tc.getFrame(test.config().frameId);
         }

Mar 09, 2016 9:43:37 PM Dispatcher.processInbound() 
SEVERE: {main} Unexpected throwable.
java.lang.reflect.InvocationTargetException
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
    at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
    at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
    at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:257)
    at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1132)
    at com.goldencode.p2j.net.Queue.transact(Queue.java:589)
    at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:223)
    at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:163)
    at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1425)
    at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:97)
    at com.sun.proxy.$Proxy4.standardEntry(Unknown Source)
    at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:281)
    at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:102)
    at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:202)
    at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:396)
    at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:1)
    at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:265)
Caused by: java.lang.IllegalStateException: Could not resolve frame from widget CaptionButton [id=-77]
    at com.goldencode.p2j.ui.client.FocusManager.focusChange(FocusManager.java:1225)
    at com.goldencode.p2j.ui.chui.ThinClient.previousTabStop(ThinClient.java:18079)
    at com.goldencode.p2j.ui.client.AbstractButton.prevTabStop(AbstractButton.java:466)
    at com.goldencode.p2j.ui.client.AbstractButton.handleCursorKeys(AbstractButton.java:353)
    at com.goldencode.p2j.ui.client.AbstractButton.processKeyEventCommon(AbstractButton.java:309)
    at com.goldencode.p2j.ui.client.Button.processKeyEvent(Button.java:467)
    at com.goldencode.p2j.ui.client.widget.AbstractWidget$1.run(AbstractWidget.java:2027)
    at com.goldencode.p2j.ui.chui.ThinClient.eventBracket(ThinClient.java:14109)
    at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:14050)
    at com.goldencode.p2j.ui.chui.ThinClient.independentEventDrawingBracket(ThinClient.java:13923)
    at com.goldencode.p2j.ui.client.widget.AbstractWidget.processSystemKey(AbstractWidget.java:2004)
    at com.goldencode.p2j.ui.chui.ThinClient.checkForSystemEvent(ThinClient.java:13289)
    at com.goldencode.p2j.ui.chui.ThinClient.waitForEvent(ThinClient.java:13178)
    at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:11049)
    at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:10623)
    at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:10577)
    ... 21 more

Hynek, do you know if caption buttons should be traversed using TAB or SHIFT + TAB?

Sergey Ivanovskiy wrote:

Hynek, do you know if caption buttons should be traversed using TAB or SHIFT + TAB?

No, they should not.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Hynek, do you know if caption buttons should be traversed using TAB or SHIFT + TAB?

No, they should not.

Committed revision 11057.

Have you looked at the recreate in #2958?

What are the remaining known issues?

What testing is still pending?

2958 issue is reproduced for the Swing client, trying to identify the root causes.

Sergey Ivanovskiy wrote:

2958 issue is reproduced for the Swing client, trying to identify the root causes.

After the "Message Update" dialog is first opened and then closed, the focus window has become the default window and continues to be the focused window. Thus the current focused widget is null and the focus input is owned by the default window. It explains why the pressing ESC helps to unblock the target application. From the moment of the first dialog appearance to its closing the focus window is null.

Sergey Ivanovskiy wrote:

After the "Message Update" dialog is first opened and then closed, the focus window has become the default window and continues to be the focused window. Thus the current focused widget is null and the focus input is owned by the default window. It explains why the pressing ESC helps to unblock the target application. From the moment of the first dialog appearance to its closing the focus window is null.

I got some logs of this incorrect focus behavior in case the "Message Update" dialog was opened first time. It follows that ThinClient.waitForWorker does this incorrectly, but the considered function is very sophisticated.

WindowManager.getCurrentWindow = WindowGuiImpl [id=1, title=]
WindowManager.topLevelWindow()=DialogBoxWindow [id=-37, title=Message Update]
java.lang.Exception
    at com.goldencode.p2j.ui.client.WindowManager.setFocusWindow(WindowManager.java:703)
    at com.goldencode.p2j.ui.client.widget.AbstractContainer.setFocus(AbstractContainer.java:1052)
    at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:11394)
    at com.goldencode.p2j.ui.client.Window.tinyInput(Window.java:1682)
    at com.goldencode.p2j.ui.client.Window.message(Window.java:2046)
    at com.goldencode.p2j.ui.chui.ThinClient.message(ThinClient.java:6915)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
    at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:705)
    at com.goldencode.p2j.net.Conversation.block(Conversation.java:319)
    at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:257)
    at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1132)
    at com.goldencode.p2j.net.Queue.transact(Queue.java:589)
    at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:223)
    at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:163)
    at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1425)
    at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:97)
    at com.sun.proxy.$Proxy4.standardEntry(Unknown Source)
    at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:281)
    at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:102)
    at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:202)
    at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:396)
    at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:96)
    at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:265)
WindowManager.setFocusWindow WindowGuiImpl [id=1, title=]

Logging ./ask-gui.p with ./hello.p as an input program helped to determine a thread raise condition occurred when a user confirms a selection pressing ENTER.
Swing UI thread (SwingEmulatedWindow/WindowFocusListener) is posted InvocationEvent to activate DialogBoxWindow [id=-37, title=Message Update], while the P2J client main thread invokes WindowManager.demodalizeWindow() and then from Window.tinyInput(...) invokes this code screen.getRegistry().removeFrame(tmpFrame); that removes the dialog from the window registry.
The current code is unreliable and theoretically this condition can be reproduced without logging. The logging changes the time between commands in the sequential program flow.

Hynec, could you help to define and to write the condition when the Progress active window becomes the Progress focused window. Logging ./ask-gui.p with ./hello.p as an input program proves that the modal dialog becomes the Progress active window but it is not the focused window.

The unique place where the focused window is set in the program above is that after this modal dialog has been closed, the focused window is set indirectly by

   public ScreenBuffer waitForWorker(EventList          list,
                                      int                focusWidgetId,
                                      int                seconds,
                                      final ScreenBuffer inbuf,
                                      boolean            force,
                                      boolean            status,
                                      boolean            entryWindow,
                                      BlockingOperation  operation)
{

      if (triggerNesting == 0 && !inEditingBlock)
      {
         // always set focus to undefined, if on root nesting level
         WindowManager.getDefaultWindow().setFocus(null);
      }

Sergey Ivanovskiy wrote:

Logging ./ask-gui.p with ./hello.p as an input program helped to determine a thread raise condition occurred when a user confirms a selection pressing ENTER.
Swing UI thread (SwingEmulatedWindow/WindowFocusListener) is posted InvocationEvent to activate DialogBoxWindow [id=-37, title=Message Update], while the P2J client main thread invokes WindowManager.demodalizeWindow() and then from Window.tinyInput(...) invokes this code screen.getRegistry().removeFrame(tmpFrame); that removes the dialog from the window registry.

I don't see a race condition there. The activation of DialogBoxWindow is executed on main thread with the help of InvocationEvent. So there should not be a race between the activation and demodalization.

I think the problem lies in the split focus processing. One part is executed when WindowActivated is handled in TopLevelWindow and the other part is processed in TC.waitForWorker() (and related methods). WindowActivated is posted to the event queue only after the window is activated by the OS which is (at least in Swing) undeterministic. So WindowActivated can be "merged" to the active event queue pretty much at any moment and so may interfere with event processing of the TC part of focus processing.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Logging ./ask-gui.p with ./hello.p as an input program helped to determine a thread raise condition occurred when a user confirms a selection pressing ENTER.
Swing UI thread (SwingEmulatedWindow/WindowFocusListener) is posted InvocationEvent to activate DialogBoxWindow [id=-37, title=Message Update], while the P2J client main thread invokes WindowManager.demodalizeWindow() and then from Window.tinyInput(...) invokes this code screen.getRegistry().removeFrame(tmpFrame); that removes the dialog from the window registry.

I don't see a race condition there. The activation of DialogBoxWindow is executed on main thread with the help of InvocationEvent. So there should not be a race between the activation and demodalization.

I think the problem lies in the split focus processing. One part is executed when WindowActivated is handled in TopLevelWindow and the other part is processed in TC.waitForWorker() (and related methods). WindowActivated is posted to the event queue only after the window is activated by the OS which is (at least in Swing) undeterministic. So WindowActivated can be "merged" to the active event queue pretty much at any moment and so may interfere with event processing of the TC part of focus processing.

Thank you, I will provide the detailed explanation of this possible situation later.

Sergey Ivanovskiy wrote:

Hynec, please review the proposed diff that fixes the 2958 issue and the new issue occurred in the case of old invocation event is executed after the modal window is removed.

Committed revision 11063 in 1811t branch. Planning to test it with the 454 testcase.

Sergey Ivanovskiy wrote:

Hynec, please review the proposed diff that fixes the 2958 issue and the new issue occurred in the case of old invocation event is executed after the modal window is removed.

The change in WM.activateWindow() seems to be ok.

Although the change in AC.setFocus() may fix the particular case, I don't think it resolves the cause. Top modal window by definition must always be active. So fixing the deactivated modal window here (which should have been active) is too late.

Sergey Ivanovskiy wrote:

The server log contains HQL parser error like that

This is a known issue.

Sergey Ivanovskiy wrote:

With current 1811t GenericFrame.initLiteralFG() is the same as in the current P2j. 454 testcase is changed after my last usage. If click on the OK button, then the testcase window is reopened and the current opened window is closed.

I don't understsand where the problem is. I've tested with 1811t rev 11063 and it works as expected - test 454 can be reached.

Constantin Asofiei wrote:

I don't understsand where the problem is. I've tested with 1811t rev 11063 and it works as expected - test 454 can be reached.

I have tested the 454 test case again after all jars are rebuild (ant jar) and it is ok now. The main screen appears and it looks that 3 windows are opened "Szzzzz Cyyyyy Auuuuuu", "<customer> GUI Test Harness" and one empty window with its closed button and without a title. Also Tab and Shift Tab focus traversal doesn't work properly. The focus from one panel tab is moved to another none neighbor tab panel and it doesn't cover all panels and some widget attracts the focus constantly, in this case only the mouse pointer can help to fix the current focus.

Hynek Cihlar wrote:

Although the change in AC.setFocus() may fix the particular case, I don't think it resolves the cause. Top modal window by definition must always be active. So fixing the deactivated modal window here (which should have been active) is too late.

For 454 test case the focus management doesn't work properly, but in order to proceed we need simple target tests. Hynek, do you know some test cases (Progress 4GL programs) for which the focus management doesn't work properly and which are more simple than the customer's GUI Test Harness 454 test case?

For an example, in note 44 if the current window handle is assigned to new dynamic window but the default window is visible, then it can mess the focus management.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Although the change in AC.setFocus() may fix the particular case, I don't think it resolves the cause. Top modal window by definition must always be active. So fixing the deactivated modal window here (which should have been active) is too late.

For 454 test case the focus management doesn't work properly, but in order to proceed we need simple target tests. Hynek, do you know some test cases (Progress 4GL programs) for which the focus management doesn't work properly and which are more simple than the customer's GUI Test Harness 454 test case?

Try two frames each with one FILL-IN widget. Alternatively one frame displayed as DIALOG-BOX.

EDIT: Each frame in its own window.

I don't understand the proposed test. Please correct me but focus_order.p from note 44 has 3 frames in 3 different windows and it looks working properly.

Sergey Ivanovskiy wrote:

I don't understand the proposed test. Please correct me but focus_order.p from note 44 has 3 frames in 3 different windows and it looks working properly.

I just tried focus_order.p. For example when I switch from one window to another (with Alt-Tab) the focus should be transferred to one of the fill-ins of the active window, but sometime it is not (text input is not reflected in the widget).

Hynek Cihlar wrote:

I just tried focus_order.p. For example when I switch from one window to another (with Alt-Tab) the focus should be transferred to one of the fill-ins of the active window, but sometime it is not (text input is not reflected in the widget).

Ok, thank you, I didn't test ALT + TAB. It can move the focus out of the P2j windows to another OS windows and it can't be performed on the Web client. Do you know another tests? Usually in simple tests we work with the client processes but if the server rules or some triggers are invoked, then another code can be involved. I have never considered the server.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

I just tried focus_order.p. For example when I switch from one window to another (with Alt-Tab) the focus should be transferred to one of the fill-ins of the active window, but sometime it is not (text input is not reflected in the widget).

Ok, thank you, I didn't test ALT + TAB. It can move the focus out of the P2j windows to another OS windows

I meant switching between the P2J windows, not to/from other OS windows. Also, you can achieve the same by using the OS task bar.

Do you know another tests?

I think focus_order.p is good test to start with. You can see focusing issues there.

Usually in simple tests we work with the client processes but if the server rules or some triggers are invoked, then another code can be involved. I have never considered the server.

Let's do the client first where we know for sure focusing issues do occur.

Ok, thank you, planning to consider ALT+TAB (1811t) for the Swing client.

Although the change in AC.setFocus() may fix the particular case, I don't think it resolves the cause. Top modal window by definition must always be active. So fixing the deactivated modal window here (which should have been active) is too late.

Sergey: What about this feedback from Hynek?

Greg Shah wrote:

Although the change in AC.setFocus() may fix the particular case, I don't think it resolves the cause. Top modal window by definition must always be active. So fixing the deactivated modal window here (which should have been active) is too late.

Sergey: What about this feedback from Hynek?

The target of this fix is to do the modal active window to be the focused window but not to set it active, because it is active at the moment of this invocation. The existing code (note 51) sets indirectly the focused window (!?)

   @Override
   public void setFocus(Widget<O> focus)
   {
      TopLevelWindow<O> window = topLevelWindow();

      if (WindowManager.topLevelWindow() != window)
      {
         // activate the related window
         WindowManager.activateWindow(window);

         // set the new focus window.  a window can be set in ACTIVATED state even without being
         // the focus owner - so we can't set it as FOCUS owner when the window gets activated
         WindowManager.setFocusWindow(window);
      }

      setFocusInt(focus);
      if (parent() != null)
         parent().setFocus(this);
   }

   @Override
   public void setFocus(Widget<O> focus)
   {
      TopLevelWindow<O> window = topLevelWindow();

      // if window is modal, then WindowManager.setFocusWindow(window)

      if (WindowManager.topLevelWindow() != window ||
               WindowManager.getActiveModalWindow() == window) 
      {
         // activate the related window
         WindowManager.activateWindow(window);

         // set the new focus window.  a window can be set in ACTIVATED state even without being
         // the focus owner - so we can't set it as FOCUS owner when the window gets activated
         WindowManager.setFocusWindow(window);
      }

      setFocusInt(focus);
      if (parent() != null)
         parent().setFocus(this);
   }

Sergey Ivanovskiy wrote:

Greg Shah wrote:

Although the change in AC.setFocus() may fix the particular case, I don't think it resolves the cause. Top modal window by definition must always be active. So fixing the deactivated modal window here (which should have been active) is too late.

Sergey: What about this feedback from Hynek?

The target of this fix is to do the modal active window to be the focused window but not to set it active, because it is active at the moment of this invocation. The existing code (note 51) sets indirectly the focused window (!?)
[...]
and this modification is to fix the focused window correctly
[...]

Sergey, if the window is already active (and the top modal window must be), then you should not call WM.activateWindow().

Also I am not sure if focus can be moved out of a widget in an active DIALOG-BOX to other widget of a deactivated window. Certainly not with a mouse input. I also doubt this can be done from a trigger of the active DIALOG-BOX, or can it? If you can move focus to a widget of a deactivated window while there is an active DIALOG-BOX then

WindowManager.setFocusWindow(widget)

in setFocus(Widget) would make sense if WindowManager.topLevelWindow() != window || WindowManager.getActiveModalWindow() == window.

Otherwise I don't see a valid case for setting focus to an active modal window in setFocus(Widget) because the modal window should not lose the focus in the first place.

EDIT: loose -> lose

Excuse me, but I will try but now don't understand. Do you describe how the focus must work?

Sergey Ivanovskiy wrote:

Do you describe how the focus must work?

I hope so.

Ok, thank you. Now I understand that you mean that this code

 @Override
   public void setFocus(Widget<O> focus)
   {
      TopLevelWindow<O> window = topLevelWindow();

      if (WindowManager.topLevelWindow() != window)
      {
         // activate the related window
         WindowManager.activateWindow(window);

         // set the new focus window.  a window can be set in ACTIVATED state even without being
         // the focus owner - so we can't set it as FOCUS owner when the window gets activated
         WindowManager.setFocusWindow(window);
      }

      setFocusInt(focus);
      if (parent() != null)
         parent().setFocus(this);
   }

Another question: why public static void modalizeWindow(TopLevelWindow window) doesn't set the global focused window? Where can be the logic that must set the focused window?

Sergey Ivanovskiy wrote:

Another question: why public static void modalizeWindow(TopLevelWindow window) doesn't set the global focused window? Where can be the logic that must set the focused window?

I would like to note (51) that the unique method that sets the focused window indirectly is that

class AbstractContainer {
 public void setFocus(Widget<O> focus);
}

Sergey Ivanovskiy wrote:

Ok, thank you. Now I understand that you mean that this code
[...]
must only work for the cases where the focus has been moved to new widget in an active unfocused window and since the modal window doesn't lose the focus until it is opened, then the proposed fix is not correct. Correct?

Yes. Just to clarify, the code should also work for cases where the focus has been moved to non-active unfocused window.

Sergey Ivanovskiy wrote:

Another question: why public static void modalizeWindow(TopLevelWindow window) doesn't set the global focused window? Where can be the logic that must set the focused window?

Good question. The reason is that this method would be too early to update focus, it is called before the actual window is requested to be displayed by the driver.

Planning to study why the logic with FocusListener doesn't work now.

Sergey Ivanovskiy wrote:

Planning to study why the logic with FocusListener doesn't work now.

Please look at the TopLevelWindow that processes WindowActivated by this code

         WindowActivated activated = (WindowActivated) event;

         final TitledWindow<?> oldActive = WindowManager.topLevelWindow();
         // current active top-level window but not overlay
         TopLevelWindow tlw = null;
         if (deactivated event)
         {
...............................................
         }
         else if ((oldActive != this && !(oldActive != null && oldActive.isOverlay())) ||
                  activated.withNativeWindow())
         {
            // the ACTIVE-WINDOW handle is updated before the LEAVE/ENTRY events are sent
            // if LEAVE event returns NO-APPLY, ENTRY is no longer sent

            TopLevelWindow<?> owner = getOwner();
            if (owner != null)
            {
               // this is a window connected to an owner, move the owner to top 
               WindowManager.moveToTop(owner);

               // and also all the other dialogs owned by the owner, make sure 
               // the z-order is preserved
               int oId = owner.getId().asInt();
               WindowManager.getModalsOwnedBy(oId, true).stream()
                  .forEach(w -> WindowManager.moveToTop(w));

               // and finally move us to top
               WindowManager.moveToTop(this);
            }
            else
            {
               // this is a main window, move us to top
               WindowManager.moveToTop(this);

               // move to top the modal windows owned by this window, preserve z-order 
               WindowManager.getModalsOwnedBy(getId().asInt(), true).stream()
                  .forEach(w -> WindowManager.moveToTop(w));
            }

            // the active-window handle is set BEFORE executing the LEAVE/ENTRY triggers
            WindowManager.setActiveWindow(owner != null ? owner : this);

            // do not use the top-level window here as it may not be the active one
            // (consider the TOP-ONLY attribute)
            TopLevelWindow<?> actualActive = WindowManager.getActiveWindow();

            if (oldActive != actualActive)
            {
               final boolean[] entry = new boolean[1];
               // send leave, only if prev focus was within P2J
               ThinClient.getInstance().independentEventDrawingBracket(oldActive, new Runnable()
               {
                  @Override
                  public void run()
                  {
                     // old top receives LEAVE event
                     entry[0] = activated.withNativeWindow() || 
                                ThinClient.getInstance().sendLeave(oldActive);
                     EventManager.postEvent(new FocusEvent(oldActive, EventType.FOCUS_LOST));
                     System.err.println("focus lost entry[0]=" + entry[0] + " oldActive=" + oldActive);
                     oldActive.repaint();
                     if (oldActive.currentFocus() != null)
                     {
                        oldActive.currentFocus().repaint();
                     }
                  }
               });

               if (entry[0])
               {
                  // new top receives ENTRY event only if LEAVE didn't return NO-APPLY
                  ThinClient.getInstance().sendEntry(actualActive);
                  EventManager.postEvent(new FocusEvent(actualActive, EventType.FOCUS_GAINED));
                  System.err.println("focus gained entry[0]=" + entry[0] + " actualActive=" + actualActive);
                  if (currentFocus() != null)
                  {
                     currentFocus().repaint();
                  }
               }
            }
         }
         else if (!isOverlay())
         {
            // the current top level window is a target window of this WindowActivated event
            // the logic with FocusLost FocusGained and Entry events are absent
            WindowManager.setActiveWindow(this);
         }

I suspect that the logic with the current window is broken by this two methods

   public static <O extends OutputManager<?>> Window<O> getActiveWindow()
   {
      TopLevelWindow<O> active = (TopLevelWindow<O>) activeWnd.get();

      // if the top-level active window is not the main window, return its
      // main window owner
      return active == null ? null : active.window();
   }

   public static void setActiveWindow(TopLevelWindow<?> window)
   {
      // overlay window can not be considered as active one
      if (window != null && window.isOverlay())
      {
         return;
      }

      // ACTIVE-WINDOW handle can not be null
      if (window != null)
      {
         activeWnd.set(window);
      }

      // update variable keeping track on the real active window
      currentActiveWnd.set(window);
   }

Hynec, do you know it is done intentionally? Because it changes the logic dramatically. For my aesthetic sense this statement WindowManager.setActiveWindow(window) && window == WindowManager.getActiveWindow() looks truly. If it is not, then WindowManager.getActiveWindow() should be renamed to WindowManager.getActiveWindowOwner(). What do you think?

I think if this issue will be solved, then the focus management can be improved to work properly.

Sergey Ivanovskiy wrote:

Sergey Ivanovskiy wrote:

Planning to study why the logic with FocusListener doesn't work now.

Please look at the TopLevelWindow that processes WindowActivated by this code
[...]
Is it correct that in the case of the current top level window is a target window of this WindowActivated event, then FocusGained and Entry aren't thrown? Modal windows are created on the top of their owners.

If the window is already top-level window and active, then FocusGained should not be sent.

Sergey Ivanovskiy wrote:

I suspect that the logic with the current window is broken by this two methods
[...]
[...]
Because in the code someone may suppose that WindowManager.setActiveWindow(window) && window == WindowManager.getActiveWindow(), but it is not true!

This is a bit unfortunate indeed. When you set an overlay window as active with WM.setActiveWindow() you will get its owner from WM.getActiveWindow().

Sergey Ivanovskiy wrote:

Hynec, do you know it is done intentionally? Because it changes the logic dramatically. For my aesthetic sense this statement WindowManager.setActiveWindow(window) && window == WindowManager.getActiveWindow() looks truly. If it is not, then WindowManager.getActiveWindow() should be renamed to WindowManager.getActiveWindowOwner(). What do you think?

The think is that WM.getActiveWindow() and partly also WM.setActiveWindow() are used for runtime support of ACTIVE-WINDOW. I agree this is a bit confusing especially as WM.setActiveWindow() mixes two concerns now - WINDOW-ACTIVE and the actual active window (as visible by the OS).

I personally would rather see the ACTIVE-WINDOW concerns out of WM and implemented on top of WM. WINDOW-ACTIVE support belongs more to the business logic than the core windowing subsystem IMHO.

Greg, what do you think if we will create a separate branch for this task and do all fixes in new branch but not in 1811t? It seems that 2954 is a blocking task.

Greg, what do you think if we will create a separate branch for this task and do all fixes in it but not in 1811t? It seems that it is a blocking task.

I think that is a good idea.

But at this point, I think you should switch to #3017. Hynek: please finish this task.

Sergey, if you have any pending changes, please create new task branch and commit them there.

Hynek Cihlar wrote:

Sergey, if you have any pending changes, please create new task branch and commit them there.

Planning to apply to 2954a branch the proposed changes.

Rebased task branch 2954a from trunk revision 10985, task branch is now at revision 10986.

Greg, do you remember why TitledWindow does requestFocus() on every draw()? This seems to be too aggressive and to interfere with other focusing logic.

Greg, do you remember why TitledWindow does requestFocus() on every draw()? This seems to be too aggressive and to interfere with other focusing logic.

No, I don't recall this. I wonder if this was a workaround put in before window activation was working.

Constantin: do you recall this?

Greg Shah wrote:

Greg, do you remember why TitledWindow does requestFocus() on every draw()? This seems to be too aggressive and to interfere with other focusing logic.

No, I don't recall this. I wonder if this was a workaround put in before window activation was working.

Constantin: do you recall this?

The code was added by SIY, in the first history of this file (when it was created). I wonder if it's not related to combo-box drop-down, as the DropDown inherits it.

Constantin Asofiei wrote:

Greg Shah wrote:

Greg, do you remember why TitledWindow does requestFocus() on every draw()? This seems to be too aggressive and to interfere with other focusing logic.

No, I don't recall this. I wonder if this was a workaround put in before window activation was working.

Constantin: do you recall this?

The code was added by SIY, in the first history of this file (when it was created). I wonder if it's not related to combo-box drop-down, as the DropDown inherits it.

Thanks for the efforts. I will get rid of the call, it causes more bad than good.

Hynek Cihlar wrote:

Thanks for the efforts. I will get rid of the call, it causes more bad than good.

I agree, but keep an eye for drop-down related issues (especially in ChUI).

For the record. There are multiple issues in the windowing subsystem causing the wrong focusing and window activation behavior.

(1) Managing the active window in WindowManager is too complicated and buggy, it mixes the ACTIVE-WINDOW system handle concern with the OS-active window concern. Also, it tracks separately active modal and non-modal windows.

(2) Handling of window activation event has some serious issues, for example it incorrectly interprets top-level window as the active window, this causes the focus events not being sent in some cases (this is also the root cause of the problems with the frozen MESSAGE UPDATE, the update window is not set as the focused window and the key input is missed).

(3) Focusing logic assumes the UI driver requests are synchronous - i.e. the requested changes are "committed" as soon as the request method of the driver interface exits. This is however not always the case, especially for Swing driver.

Points (1) and (2) must be resolved before (3).

Nice work! Thank you for providing these details. I think your fixes will make the resulting environment significantly more stable and functional.

Igor, I am trying to decode the role of the active window (as received by WindowManager.getActiveWindow()) in ThinClient.refreshHover(). I am doing some changes to the way active window is being managed and I am reviewing all the places where active window is being referenced. Ideally if you could give me an example use case where the active window occurs.

Igor Skornyakov wrote:

Hynek Cihlar wrote:

Igor, I am trying to decode the role of the active window (as received by WindowManager.getActiveWindow()) in ThinClient.refreshHover(). I am doing some changes to the way active window is being managed and I am reviewing all the places where active window is being referenced. Ideally if you could give me an example use case where the active window occurs.

Hynek,
Please find the (simplified) version of my test attached.

So if I understand it correctly, the active window in ThinClient.refreshHover() is used in case a modal window is closed while the mouse is not moved and as it is closed the window under the cursor becomes the next active window which may have a different cursor defined. Correct?

But what in the case the window under the cursor does not happen to be the active window? For example when multiple windows are visible? Wouldn't it be better to find the window at the cursor position with highest z-order instead of the active window?

Hynek Cihlar wrote:

So if I understand it correctly, the active window in ThinClient.refreshHover() is used in case a modal window is closed while the mouse is not moved and as it is closed the window under the cursor becomes the next active window which may have a different cursor defined. Correct?

But what in the case the window under the cursor does not happen to be the active window? For example when multiple windows are visible? Wouldn't it be better to find the window at the cursor position with highest z-order instead of the active window?

I haven't performed a detailed investigation. What I see is: after a model window is closed and I click on the main window and press ESC nothing happens. After I click outside and inside the window again the functionality is restored. There are no enabled widgets inside the main window, moreover the frame is rendered incorrectly (this is #3033 bug)

Igor Skornyakov wrote:

Hynek Cihlar wrote:

So if I understand it correctly, the active window in ThinClient.refreshHover() is used in case a modal window is closed while the mouse is not moved and as it is closed the window under the cursor becomes the next active window which may have a different cursor defined. Correct?

But what in the case the window under the cursor does not happen to be the active window? For example when multiple windows are visible? Wouldn't it be better to find the window at the cursor position with highest z-order instead of the active window?

I haven't performed a detailed investigation. What I see is: after a model window is closed and I click on the main window and press ESC nothing happens. After I click outside and inside the window again the functionality is restored. There are no enabled widgets inside the main window, moreover the frame is rendered incorrectly (this is #3033 bug)

Yes, what you describe are the focusing issues. I am trying to understand the custom cursors functionality. Can you please confirm the active window retrieved by WM.getActiveWindow() in TC.refreshHover() is used to refresh the cursor after window is closed while no mouse move input is performed?

Hynek Cihlar wrote:

For the record. There are multiple issues in the windowing subsystem causing the wrong focusing and window activation behavior.

(1) Managing the active window in WindowManager is too complicated and buggy, it mixes the ACTIVE-WINDOW system handle concern with the OS-active window concern. Also, it tracks separately active modal and non-modal windows.

(2) Handling of window activation event has some serious issues, for example it incorrectly interprets top-level window as the active window, this causes the focus events not being sent in some cases (this is also the root cause of the problems with the frozen MESSAGE UPDATE, the update window is not set as the focused window and the key input is missed).

(3) Focusing logic assumes the UI driver requests are synchronous - i.e. the requested changes are "committed" as soon as the request method of the driver interface exits. This is however not always the case, especially for Swing driver.

Points (1) and (2) must be resolved before (3).

As I am already also touching the overlay windows logic I will remove the limitation of one active window at a time. Multiple visible overlays will be needed to support sub-menus.

Hynek Cihlar wrote:

Yes, what you describe are the focusing issues. I am trying to understand the custom cursors functionality. Can you please confirm the active window retrieved by WM.getActiveWindow() in TC.refreshHover() is used to refresh the cursor after window is closed while no mouse move input is performed?

• on exit from the wait state
• on the close of the drop-down list.
• on the change of the mouse cursor via LOAD-MOUSE-POINTER.

In theory it is possible that TC.refreshHover() is called after window is closed only in the last scenario but I'm not sure.

Igor Skornyakov wrote:

Hynek Cihlar wrote:

Yes, what you describe are the focusing issues. I am trying to understand the custom cursors functionality. Can you please confirm the active window retrieved by WM.getActiveWindow() in TC.refreshHover() is used to refresh the cursor after window is closed while no mouse move input is performed?

The TC.refreshHover is invoked in 3 cases:

• on exit from the wait state
• on the close of the drop-down list.
• on the change of the mouse cursor via LOAD-MOUSE-POINTER.

In theory it is possible that TC.refreshHover() is called after window is closed only in the last scenario but I'm not sure.

Ok, thanks for the list of cases, I will test it.

Sergey Ivanovskiy wrote:

Found that labels in frames are not registered as widgets but have unique ids. In this program "A" if the mouse pointer is over i1 or str1 side labels, then this kind of messages appears in the Swing client logs
[...]
The tested program "A" is listed below
[...]
Constantin, what do you think it is correct?

I have a fix for this in 2226a (#1782)

Constantin, do you remember what test cases you used to come up with the algorithm to calculate the next active window when a window is made hidden? The current algorithm is in GuiWindowCommons.hide(), in particular the window instanceof WindowGuiImpl branch.

Here is a test case that works differently in 4GL:

DEF VAR h1 AS HANDLE.
CREATE WINDOW h1 ASSIGN TITLE = "Window 1".
DEF VAR h2 AS HANDLE.
CREATE WINDOW h2 ASSIGN TITLE = "Window 2".

DEFAULT-WINDOW:VISIBLE = true.
h1:VISIBLE = true.
h2:VISIBLE = true.

/* make the window 2 active */
PAUSE.

h2:VISIBLE = FALSE.

/* 
   at this point the active window in 4GL becomes DEFAULT-WINDOW, in P2J however
   this would be Window 1 
*/

It seems like 4GL always chooses the first visible window in creation order as the next active window.

Hynek Cihlar wrote:

Constantin, do you remember what test cases you used to come up with the algorithm to calculate the next active window when a window is made hidden? The current algorithm is in GuiWindowCommons.hide(), in particular the window instanceof WindowGuiImpl branch.

I think I used uast/window_events/multiple_window.p, but in the implementation I might have gotten confused by the window parenting behaviour (where creation order does matter...).

It seems like 4GL always chooses the first visible window in creation order as the next active window.

Actually, I don't think creation order has anything to do determining the next active window. I think the z-order is used.

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Constantin, do you remember what test cases you used to come up with the algorithm to calculate the next active window when a window is made hidden? The current algorithm is in GuiWindowCommons.hide(), in particular the window instanceof WindowGuiImpl branch.

I think I used uast/window_events/multiple_window.p, but in the implementation I might have gotten confused by the window parenting behaviour (where creation order does matter...).

It seems like 4GL always chooses the first visible window in creation order as the next active window.

Actually, I don't think creation order has anything to do determining the next active window. I think the z-order is used.

Yes, z-order would make sense to me, too. But try the following program.

/* 
Run this test from command line, the Procedure Editor window
is a regular 4GL window and would interfere with the test.

Instructions:
Run the test, press 0, 1, 2, 3, 3, 2, 1, 0 and observe the window
activation order.
*/ 

DEF VAR h1 AS HANDLE.
DEF VAR h2 AS HANDLE.
DEF VAR h3 AS HANDLE.

ON '0':U ANYWHERE 
DO:
   DEFAULT-WINDOW:VISIBLE = NOT DEFAULT-WINDOW:VISIBLE.
   RETURN.
END.

ON '1':U ANYWHERE 
DO:
   IF h1 EQ ? THEN DO:
      CREATE WINDOW h1 ASSIGN TITLE = "Window 1".
   END.
   h1:VISIBLE = NOT h1:VISIBLE.
   RETURN.
END.

ON '2':U ANYWHERE
DO:
   IF h2 EQ ? THEN DO:
      CREATE WINDOW h2 ASSIGN TITLE = "Window 2".
   END.
   h2:VISIBLE = NOT h2:VISIBLE.
   RETURN.
END.

ON '3':U ANYWHERE
DO:
   IF h3 EQ ? THEN DO:
      CREATE WINDOW h3 ASSIGN TITLE = "Window 3".
   END.
   h3:VISIBLE = NOT h3:VISIBLE.
   RETURN.
END.

WAIT-FOR GO OF THIS-PROCEDURE.

• Run it from cli.
• Press 3, 2, 1.
• Activate Window 3, Window 2, Window 1.
• Press 1 to hide Window 1.
• 4GL activates Window 3.

Something is missing in your test, because running it from CLI it terminates the program immediately.

Hynek Cihlar wrote:

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Constantin, do you remember what test cases you used to come up with the algorithm to calculate the next active window when a window is made hidden? The current algorithm is in GuiWindowCommons.hide(), in particular the window instanceof WindowGuiImpl branch.

I think I used uast/window_events/multiple_window.p, but in the implementation I might have gotten confused by the window parenting behaviour (where creation order does matter...).

It seems like 4GL always chooses the first visible window in creation order as the next active window.

Actually, I don't think creation order has anything to do determining the next active window. I think the z-order is used.

Yes, z-order would make sense to me, too. But try the following program.

Another note here: running the multiple_window.p program from CLI instead of editor behaves differently: hiding window h2 does indeed move focus to window h1 (i.e. the window created before h2, as P2J implements it now), regardless which one is next in z-order, bellow h2.

So we might have some "mixed" behavior.

Constantin Asofiei wrote:

Something is missing in your test, because running it from CLI it terminates the program immediately.

Actually it doesn't terminate, it just doesn't open any window. Still it can receive input, so just press one of 1, 2, 3 right after you execute it from cli.

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Constantin, do you remember what test cases you used to come up with the algorithm to calculate the next active window when a window is made hidden? The current algorithm is in GuiWindowCommons.hide(), in particular the window instanceof WindowGuiImpl branch.

I think I used uast/window_events/multiple_window.p, but in the implementation I might have gotten confused by the window parenting behaviour (where creation order does matter...).

It seems like 4GL always chooses the first visible window in creation order as the next active window.

Actually, I don't think creation order has anything to do determining the next active window. I think the z-order is used.

Yes, z-order would make sense to me, too. But try the following program.

Another note here: running the multiple_window.p program from CLI instead of editor behaves differently: hiding window h2 does indeed move focus to window h1 (i.e. the window created before h2, as P2J implements it now),

When you modify the test and hide h3 instead, you still get h1 as the active window. Hence the correct implementation is to activate the first visible window in creation order as suggested by other tests.

regardless which one is next in z-order, bellow h2.

So we might have some "mixed" behavior.

You are right, the behavior depends on the way the program is executed, CLI vs Procedure Editor. I assume we don't want to replicate the behavior of Procedure Editor, correct?

Hynek Cihlar wrote:

When you modify the test and hide h3 instead, you still get h1 as the active window. Hence the correct implementation is to activate the first visible window in creation order as suggested by other tests.

OK, your findings make sense.

I assume we don't want to replicate the behavior of Procedure Editor, correct?

Correct.

Rebased task branch 2954a from trunk revision 10990, task branch is now at revision 10999.

Eugenie, I am fixing OverlayWindow not cleaning up after itself when it is dismissed and have a related question. What is the purpose of emulating the ESC key in DropDownGuiImpl.dismiss()?

Hynek Cihlar wrote:

Eugenie, I am fixing OverlayWindow not cleaning up after itself when it is dismissed and have a related question. What is the purpose of emulating the ESC key in DropDownGuiImpl.dismiss()?

As explained in DropDownGuiImpl.dismiss() javadoc header:

...
    * Executing event to hide the drop-down from the screen.  This can be done either from outside
    * or inside the working drop-down code, so the message processing loop is used to avoid dead
    * locking.  This simulates ESC key pressing when drop-down is opened.
...

Eugenie Lyzenko wrote:

Hynek Cihlar wrote:

Eugenie, I am fixing OverlayWindow not cleaning up after itself when it is dismissed and have a related question. What is the purpose of emulating the ESC key in DropDownGuiImpl.dismiss()?

As explained in DropDownGuiImpl.dismiss() javadoc header:
[...]

I wasn't sure whether there is more to the ESC event than just the drop-down dismiss unification, even after reading the doc. Also I'm not sure how a dead-lock could occur while dismissing the drop-down.

Generally this is unification of the drop-down dismiss to protect from the event de-sync. No matter how dismiss is initiated(mouse press outside for example), the real work finally is happening in key event handler.

What do you mean by event de-sync?

Hynek Cihlar wrote:

I wasn't sure whether there is more to the ESC event than just the drop-down dismiss unification, even after reading the doc. Also I'm not sure how a dead-lock could occur while dismissing the drop-down.

For example when drop-down code attempts to use the objects already terminated during termination sequence in other parts of the overlay/drop-down code.

Generally this is unification of the drop-down dismiss to protect from the event de-sync. No matter how dismiss is initiated(mouse press outside for example), the real work finally is happening in key event handler.

What do you mean by event de-sync?

This means the event handling sequence is not the same as the event generation sequence.

Rebased task branch 2954a from trunk revision 10994, task branch is now at revision 11007.

Task branch 2954a revision 11008 can be reviewed. There are some todos yet (the focus changes not yet fully complete, regression testing not finished), but most of the changes is in place.

Task branch 2954a revision 11009 resolves the last known focusing issues. The left task is regression testing.

Please review (ignore the debug logging changes, I haven't removed it yet in case it is still needed).

Code Review Task Branch 2954a Revision 11009

Overall, this is a really great update. The logic is much cleaner, removing many special cases and making the dependencies clearer and more abstract.

1. In TopLevelWindow.processEvent(), there is this code:

if (!activated.isActivated())
{
}

if (activated.isActivated())
{
}

The state in activated cannot change between the two blocks. I think it is more clear in this case to turn the second block into an else. Otherwise the reader must do the same analysis that I did to determine that the two blocks are mutually exclusive.

2. In TopLevelWindow.processEvent() there is a check for TopLevelWindow.this != null. Considering this is an instance method, in what scenario can this be false?

3. build.xml needs a history entry.

4. WindowManager.getActiveWindows() needs javadoc.

5. Please enhance the WindowManager.processWindowActivated() javadoc for the activated parameter to explain the behavior when passed as null.

6. The parameter javadoc for WindowManager.forEachActiveWindow() is incomplete.

7. In ComboBoxGuiImpl line 432 there is a TOOD that probably should be TODO. Fixing the typo may make it easier to find later. :)

Constantin and Sergey: Please do a code review. Consider that 1811t will be merged to trunk before this task branch. Are there any considerations Hynek should have in regard to rebasing? Also, do you have any concerns with the web client changes?

Greg Shah wrote:

Code Review Task Branch 2954a Revision 11009

Overall, this is a really great update. The logic is much cleaner, removing many special cases and making the dependencies clearer and more abstract.

1. In TopLevelWindow.processEvent(), there is this code:

[...]

The state in activated cannot change between the two blocks. I think it is more clear in this case to turn the second block into an else. Otherwise the reader must do the same analysis that I did to determine that the two blocks are mutually exclusive.

You are right, will fix.

2. In TopLevelWindow.processEvent() there is a check for TopLevelWindow.this != null. Considering this is an instance method, in what scenario can this be false?

This looks like a braino. The code should be this:

                  lastLeaveResult = TopLevelWindow.this.getOwner() == null &&
                                    !activated.withNativeWindow() && 
                                    ThinClient.getInstance().sendLeave(TopLevelWindow.this);

Will fix.

3. build.xml needs a history entry.

4. WindowManager.getActiveWindows() needs javadoc.

5. Please enhance the WindowManager.processWindowActivated() javadoc for the activated parameter to explain the behavior when passed as null.

6. The parameter javadoc for WindowManager.forEachActiveWindow() is incomplete.

7. In ComboBoxGuiImpl line 432 there is a TOOD that probably should be TODO. Fixing the typo may make it easier to find later. :)

All will fix.

Code Review Task Branch 2954a Revision 11010

I'm fine with the changes.

Greg Shah wrote:

Are there any considerations Hynek should have in regard to rebasing?

My changes from 1811t should not affect/interfere with 2954a.

Also, do you have any concerns with the web client changes?

The changes look OK to me. There is an improvement we can do, in p2j.screen.js:2471, the sendWindowStateActive function: we should not use two trips for the iconification/activate messages, this should be done in only one trip. But this can be improved in 1811u.

Found that rev 10999 changed the logic related to child windows, where resizable attribute substituted overlay. The most web client changes are in this revision, aren't?

Sergey Ivanovskiy wrote:

Found that rev 10999 changed the logic related to child windows, where resizable attribute substituted overlay. The most web client changes are in this revision, aren't?

Yes 10999 contains most of the JS changes, but be safe and compare the latest task branch against trunk.

Issues from the review in note 127 addressed in revision 11011 (decimal).

Code review for 11011:

1. com/goldencode/p2j/ui/client/event/FocusEvent.java does not have the history entry for this branch
2. com/goldencode/p2j/ui/client/event/KeyInput.java does not have the history entry for this branch
3. com/goldencode/p2j/ui/client/event/WindowActivated.java does not have the history entry for this branch

I have found the method to check difference between overlay window and other was removed:
com/goldencode/p2j/ui/client/widget/TitledWindow.java - does not have isOverlay() method for example

I need to test all my drop-down related tests to see if there are the regression or not.

Hynek,

The branch can not be built:

------------
compile:
    [javac] Compiling 1637 source files to /home/evl/p2j_hc/build/classes
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/event/KeyInput.java:28: error: package org.apache.logging.log4j does not exist
    [javac] import org.apache.logging.log4j.*;
    [javac] ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/WindowTitleBar.java:67: error: package org.apache.logging.log4j does not exist
    [javac] import org.apache.logging.log4j.*;
    [javac] ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/OverlayWindow.java:28: error: package org.apache.logging.log4j does not exist
    [javac] import org.apache.logging.log4j.*;
    [javac] ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/WindowManager.java:72: error: package org.apache.logging.log4j does not exist
    [javac] import org.apache.logging.log4j.*;
    [javac] ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/event/WindowActivated.java:20: error: package org.apache.logging.log4j does not exist
    [javac] import org.apache.logging.log4j.*;
    [javac] ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/AbstractGuiDriver.java:67: error: package org.apache.logging.log4j does not exist
    [javac] import org.apache.logging.log4j.LogManager;
    [javac]                                ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/GuiPrimitivesImpl.java:45: error: package org.apache.logging.log4j does not exist
    [javac] import org.apache.logging.log4j.*;
    [javac] ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/swing/SwingGuiDriver.java:91: error: package org.apache.logging.log4j does not exist
    [javac] import org.apache.logging.log4j.*;
    [javac] ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/widget/AbstractContainer.java:1369: error: package org.apache.logging.log4j does not exist
    [javac]       org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(this);
    [javac]                               ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/widget/AbstractContainer.java:1369: error: package org.apache.logging.log4j does not exist
    [javac]       org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(this);
    [javac]                                                                     ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/TopLevelWindow.java:702: error: package org.apache.logging.log4j does not exist
    [javac]          org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(this);
    [javac]                                  ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/TopLevelWindow.java:702: error: package org.apache.logging.log4j does not exist
    [javac]          org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(this);
    [javac]                                                                        ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/event/KeyInput.java:163: error: cannot find symbol
    [javac]          LogManager.getLogger(this).debug("KeyInput.ctor ESC, source = {}", src);
    [javac]          ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class KeyInput
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/event/FocusEvent.java:39: error: package org.apache.logging.log4j does not exist
    [javac]       org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(this);
    [javac]                               ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/event/FocusEvent.java:39: error: package org.apache.logging.log4j does not exist
    [javac]       org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(this);
    [javac]                                                                     ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/ModalWindow.java:333: error: package org.apache.logging.log4j does not exist
    [javac]       org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(this);
    [javac]                               ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/ModalWindow.java:333: error: package org.apache.logging.log4j does not exist
    [javac]       org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(this);
    [javac]                                                                     ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/WindowTitleBar.java:421: error: cannot find symbol
    [javac]       LogManager.getLogger(this).debug("WindowTitleBar isActiveWindow = {}", result);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class WindowTitleBar
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/OverlayWindow.java:581: error: cannot find symbol
    [javac]       LogManager.getLogger(this).debug("destroy() {}", this);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class OverlayWindow
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/OverlayWindow.java:582: error: cannot find symbol
    [javac]       LogManager.getLogger(this).debug(LogHelper.getStackTrace());
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class OverlayWindow
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/persist/TempTableHelper.java:1051: warning: [deprecation] getIdentifierGeneratorFactory() in Mapping has been deprecated
    [javac]       public IdentifierGeneratorFactory getIdentifierGeneratorFactory()
    [javac]                                         ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/persist/dialect/P2JH2Dialect.java:893: warning: [deprecation] getIdentifierGeneratorFactory() in Mapping has been deprecated
    [javac]                public IdentifierGeneratorFactory getIdentifierGeneratorFactory()
    [javac]                                                  ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/persist/dialect/P2JPostgreSQLDialect.java:1016: warning: [deprecation] getLimitString(String,int,int) in Dialect has been deprecated
    [javac]    public String getLimitString(String sql, int offset, int limit)
    [javac]                  ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/persist/dialect/P2JPostgreSQLDialect.java:1032: warning: [deprecation] getLimitString(String,int,int) in Dialect has been deprecated
    [javac]          stmt = super.getLimitString(sql, offset, limit);
    [javac]                      ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/persist/dialect/P2JPostgreSQLDialect.java:1050: warning: [deprecation] supportsVariableLimit() in Dialect has been deprecated
    [javac]    public boolean supportsVariableLimit()
    [javac]                   ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/persist/dialect/P2JPostgreSQLDialect.java:1052: warning: [deprecation] supportsVariableLimit() in Dialect has been deprecated
    [javac]       return inlineLimit.get() ? false : super.supportsVariableLimit();
    [javac]                                               ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/WindowManager.java:337: error: cannot find symbol
    [javac]       LogManager.getLogger(WindowManager.class).debug("window activation request {}", window.getId().asInt());
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class WindowManager
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/WindowManager.java:338: error: cannot find symbol
    [javac]       LogManager.getLogger(WindowManager.class).debug(LogHelper.getStackTrace());
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class WindowManager
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/WindowManager.java:722: error: package org.apache.logging.log4j does not exist
    [javac]       org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(WindowManager.class);
    [javac]                               ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/WindowManager.java:722: error: package org.apache.logging.log4j does not exist
    [javac]       org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(WindowManager.class);
    [javac]                                                                     ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/WindowManager.java:1287: error: cannot find symbol
    [javac]       LogManager.getLogger(WindowManager.class).debug("windowActivated windowId = {}, fromNativeWindow = {}", windowId, fromNativeWindow);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class WindowManager
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/WindowManager.java:1324: error: cannot find symbol
    [javac]       LogManager.getLogger(WindowManager.class).debug("windowDeactivated windowId = {}, toNativeWindow = {}", windowId, toNativeWindow);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class WindowManager
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/WindowManager.java:1438: error: cannot find symbol
    [javac]       LogManager.getLogger(WindowManager.class).debug("current activeDriverWindow is {}", actDriverWindow);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class WindowManager
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/WindowManager.java:1439: error: cannot find symbol
    [javac]       LogManager.getLogger(WindowManager.class).debug("assigning activeDriverWindow to {}", activated);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class WindowManager
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/driver/swing/SwingKeyboardReader.java:140: warning: [deprecation] setModifiers(int) in KeyEvent has been deprecated
    [javac]          last.setModifiers(last.getModifiers() | InputEvent.SHIFT_MASK);
    [javac]              ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/driver/swing/SwingKeyboardReader.java:144: warning: [deprecation] setModifiers(int) in KeyEvent has been deprecated
    [javac]          last.setModifiers(last.getModifiers() | InputEvent.ALT_MASK);
    [javac]              ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/driver/swing/SwingKeyboardReader.java:148: warning: [deprecation] setModifiers(int) in KeyEvent has been deprecated
    [javac]          last.setModifiers(last.getModifiers() | InputEvent.CTRL_MASK);
    [javac]              ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/event/WindowActivated.java:52: error: cannot find symbol
    [javac]       LogManager.getLogger(this).debug("WindowActivated ctor  {}, {} {}", source, activated, withNativeWindow);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class WindowActivated
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/event/WindowActivated.java:53: error: cannot find symbol
    [javac]       LogManager.getLogger(this).debug(com.goldencode.util.LogHelper.getStackTrace());
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class WindowActivated
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/AbstractGuiDriver.java:1599: error: incompatible types: AbstractGuiDriver<F> cannot be converted to String
    [javac]       LogManager.getLogger(this).debug("deregisterWindow() {}", windowId);      
    [javac]                            ^
    [javac]   where F is a type-variable:
    [javac]     F extends Object declared in class AbstractGuiDriver
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/GuiPrimitivesImpl.java:247: error: cannot find symbol
    [javac]       LogManager.getLogger(this).debug("registerWindow put emulator for window {}", windowId);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class GuiPrimitivesImpl
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/GuiPrimitivesImpl.java:263: error: cannot find symbol
    [javac]       LogManager.getLogger(this).debug("removing emulator for window {}", windowId);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class GuiPrimitivesImpl
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/swing/SwingEmulatedWindow.java:824: warning: XORComposite is internal proprietary API and may be removed in a future release
    [javac]             g2.setComposite(new XORComposite(new Color(0, 0, 0), NullSurfaceData.theInstance));
    [javac]                                 ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/swing/SwingEmulatedWindow.java:824: warning: NullSurfaceData is internal proprietary API and may be removed in a future release
    [javac]             g2.setComposite(new XORComposite(new Color(0, 0, 0), NullSurfaceData.theInstance));
    [javac]                                                                  ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/swing/SwingGuiDriver.java:498: error: cannot find symbol
    [javac]       LogManager.getLogger(this).debug("moving window to top {} {}", windowId, focus);
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class SwingGuiDriver
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/ui/client/gui/driver/swing/SwingGuiDriver.java:499: error: cannot find symbol
    [javac]       LogManager.getLogger(this).debug(LogHelper.getStackTrace());
    [javac]       ^
    [javac]   symbol:   variable LogManager
    [javac]   location: class SwingGuiDriver
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/xml/XDocumentImpl.java:1074: warning: Constants is internal proprietary API and may be removed in a future release
    [javac]               (String)xdoc.getDomConfig().getParameter(Constants.DOM_SCHEMA_LOCATION);
    [javac]                                                        ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/xml/XDocumentImpl.java:1173: warning: Constants is internal proprietary API and may be removed in a future release
    [javac]       xdoc.getDomConfig().setParameter(Constants.DOM_SCHEMA_LOCATION, schemaLoc);
    [javac]                                        ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/xml/XDocumentImpl.java:1281: warning: DocumentImpl is internal proprietary API and may be removed in a future release
    [javac]       ((DocumentImpl)xdoc).setXmlEncoding(encodingStr);
    [javac]         ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/xml/XDocumentImpl.java:1301: warning: Constants is internal proprietary API and may be removed in a future release
    [javac]          schemaLocation = (String) domConfig.getParameter(Constants.SCHEMA_NONS_LOCATION);
    [javac]                                                           ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/xml/XDocumentImpl.java:1326: warning: Constants is internal proprietary API and may be removed in a future release
    [javac]       domConfig.setParameter(Constants.SCHEMA_NONS_LOCATION, location.toStringMessage());
    [javac]                              ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/xml/XDocumentImpl.java:1357: warning: Constants is internal proprietary API and may be removed in a future release
    [javac]               (String)xdoc.getDomConfig().getParameter(Constants.DOM_SCHEMA_LOCATION);
    [javac]                                                        ^
    [javac] /home/evl/p2j_hc/src/com/goldencode/p2j/xml/XDocumentImpl.java:1361: warning: Constants is internal proprietary API and may be removed in a future release
    [javac]       domConfig.setParameter(Constants.DOM_SCHEMA_LOCATION, newSchemaLocation + locationEntity);
    [javac]                              ^
    [javac] Note: Some input files use unchecked or unsafe operations.
    [javac] Note: Recompile with -Xlint:unchecked for details.
    [javac] Note: Some messages have been simplified; recompile with -Xdiags:verbose to get full output
    [javac] 35 errors
    [javac] 18 warnings

BUILD FAILED
/home/evl/p2j_hc/build.xml:897: Compile failed; see the compiler error output for details.
------------

The apache logging stuff is just temporary. That is why it is not checked into the branch.

Eugenie Lyzenko wrote:

Code review for 11011:
I have found the method to check difference between overlay window and other was removed:
com/goldencode/p2j/ui/client/widget/TitledWindow.java - does not have isOverlay() method for example

This is expected. isOverlay() was replaced with more concrete method isShareActivationWithOwner().

Hynek Cihlar wrote:

This is expected. isOverlay() was replaced with more concrete method isShareActivationWithOwner().

Yes, I see. That's why I want to see your changes in action. With real drop-down testcases.

Can you send me the jar you use containing the following class:

...
    [javac] import org.apache.logging.log4j.*;
...

You have compiled your branch, right? Then you must have this *.jar file. And it is not log4j-1.2.XX, this does not work.

OK. I have found the file on the Web. The testing results will be soon.

Hynek,

Javadoc errors to fix:

...
  [javadoc] /home/evl/timco_new/p2j/src/com/goldencode/p2j/ui/client/WindowManager.java:1242: error: type arguments not allowed here
  [javadoc]     * @return  A list of {@link TopLevelWindow<?>} instances or empty list if no such
  [javadoc]                                 ^
  [javadoc] /home/evl/timco_new/p2j/src/com/goldencode/p2j/ui/client/WindowManager.java:1277: error: element not closed: code
  [javadoc]     *          If true<code> the window losing activated state is not a P2J window.
  [javadoc]                             ^
  [javadoc] /home/evl/timco_new/p2j/src/com/goldencode/p2j/ui/client/WindowManager.java:1277: error: element not closed: code
  [javadoc]     *          If <code>true<code> the window losing activated state is not a P2J window.
  [javadoc]                   ^
  [javadoc] /home/evl/timco_new/p2j/src/com/goldencode/p2j/ui/client/WindowManager.java:1306: error: element not closed: code
  [javadoc]     *          If <code>true<code> the window gaining activated state is not a P2J window.
  [javadoc]                             ^
  [javadoc] /home/evl/timco_new/p2j/src/com/goldencode/p2j/ui/client/WindowManager.java:1306: error: element not closed: code
  [javadoc]     *          If <code>true<code> the window gaining activated state is not a P2J window.
  [javadoc]                   ^
  [javadoc] /home/evl/timco_new/p2j/src/com/goldencode/p2j/ui/client/WindowManager.java:1427: error: element not closed: code
  [javadoc]     *          If <code>true<code> the window losing activated state is not a P2J window.
  [javadoc]                             ^
  [javadoc] /home/evl/timco_new/p2j/src/com/goldencode/p2j/ui/client/WindowManager.java:1427: error: element not closed: code
  [javadoc]     *          If <code>true<code> the window losing activated state is not a P2J window.
  [javadoc]                   ^
  [javadoc] /home/evl/timco_new/p2j/src/com/goldencode/p2j/ui/client/gui/driver/web/GuiWebDriver.java:471: error: @param name not found
  [javadoc]     * @param    overlay
  [javadoc]                 ^
...

The testing for 2954a

Swing mode and Web mode:
It is not possible to make selection and close drop-down by pressing ENTER key.

Sometimes drop-down is not opening by mouse(dropDownActive flag is inconsistent may be or focus related issue when mouse press is not delivered to the combo? Why volatile?)

There is certainly focus related issue, try combo_box/combo_box21_1.p play with combos and then try to exit by pressing exit button. Can be seen in both swing and web clients.

And for swing client the initial focus issue is still here. When application starts the P2J window is shown as active(window title) but the window/frame/widgets does not have the focus. Click outside P2J and then back on P2J window makes the client really focused.

Eugenie Lyzenko wrote:

The testing for 2954a

Swing mode and Web mode:
It is not possible to make selection and close drop-down by pressing ENTER key.

This exists in trunk, too.

Sometimes drop-down is not opening by mouse(dropDownActive flag is inconsistent may be or focus related issue when mouse press is not delivered to the combo?

A similar problem with keys. For example down key doesn't sometimes open the drop-down. This doesn't seem to be focusing problem, as the combo had focus and was receiving events. I was able to reproduce this in trunk, too.

Why volatile?)

This is probably a left over from the times when mouse events were dispatched on the Swing event thread. I already removed some now-unnecessary synchronization constructs. I will remove the volatile keyword, too.

There is certainly focus related issue, try combo_box/combo_box21_1.p play with combos and then try to exit by pressing exit button. Can be seen in both swing and web clients.

And for swing client the initial focus issue is still here. When application starts the P2J window is shown as active(window title) but the window/frame/widgets does not have the focus. Click outside P2J and then back on P2J window makes the client really focused.

I will check the two above tomorrow when I finish merging 1811t.

Rebased task branch 2954a from trunk revision 10998, task branch is now at revision 11016.

Eugenie Lyzenko wrote:

The testing for 2954a

There is certainly focus related issue, try combo_box/combo_box21_1.p play with combos and then try to exit by pressing exit button. Can be seen in both swing and web clients.

This is a problem in the COMBO-BOX implementation itself. When you comment out the call to requestFocus() in ComboBox.exitDropDown() and the call to fixWaitForFocus() in ThinClient, the problem disappears.

I am putting a TODO in the code and deferring the issue as this not a regression and out of scope.

And for swing client the initial focus issue is still here. When application starts the P2J window is shown as active(window title) but the window/frame/widgets does not have the focus. Click outside P2J and then back on P2J window makes the client really focused.

I was not able to reproduce this. Do you have a test case where this happens?

Hynek Cihlar wrote:

And for swing client the initial focus issue is still here. When application starts the P2J window is shown as active(window title) but the window/frame/widgets does not have the focus. Click outside P2J and then back on P2J window makes the client really focused.

I was not able to reproduce this. Do you have a test case where this happens?

Take every combo-box testcase, for example - combo_box/combo_box9_1.p or another test case starting with the following:

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

I used this clause to be able to attach the debugger to client before any test logic execution. And when application starts and looks activated it does not react to any key pressing.

Rebased task branch 2954a from trunk revision 10999, task branch is now at revision 11022.

Task branch 2954a revision 11022 passed ChUI and GUI regression testing. Please review.

Code Review Task Branch 2954a Revision 11022

I'm fine with the changes.

Please merge to trunk.

I fixed yet two issues I missed previously. Please see and review 2954a revision 11023.

The fixed are, Swing GUI window sometimes not receiving key input when activated and modal windows not focusing properly when activated.

The changes only affect GUI and passed GUI regression tests. If review ok the revision is good to be merged to trunk.

Code Review Task Branch 2954a Revision 11023

My only question:

Does the TopLevelWindow.processEvent() activation change require sending the ENTRY event as is done in the no-owner case? Also, do we need to consider the state of lastLeaveResult?