FWD » Core Development » User Interface

1. Check if key navigation works even mouse pointer is not over the opened pop-up menu: somewhere else even outside of the window.
2. After pop-up menu is opened and I press down-arrow: the first item should be selected, not the second one.
3. When nested sub-menu is opened: all nested sub-menus become opened too.
4. Sometimes 2 items in one sub-menu becomes displayed highlighted.
5. disabled items are navigable by mouse and keyboard.

Described behaviour makes sense for GUI.

Disabled menu items are navigable.. It contravenes with the our approach, that only enabled isEnabled()==true widgets can process events.

Would it be correct to change our approach (replace isEnabled() with something other in ThinClient) or I should find some other approach..

Can you point out the menu/sub-menu code which you are proposing to change?

Constantin: please review this task.

Greg Shah wrote:

Can you point out the menu/sub-menu code which you are proposing to change?

At this moment I don't have working change. I'm only debugging. It's not so simple, so I proposed that this change could be not acceptable and I can spend some time in vain working on this.

It's not so simple, so I proposed that this change could be not acceptable and I can spend some time in vain working on this.

I understand. However, it is difficult to give you feedback without seeing the code that has to be changed. Once you find that code, mention it here. It will also give you a better idea of how reasonable it is to change.

It is hard to tell if such a change would be OK in those locations. It is possible. However, since the default version (in AbstractWidget.focusTraversable()) has this:

   public boolean focusTraversable()
   {
      return (enabled && visible && !hidden);
   }

This is more restrictive than just checking isEnabled(). It MAY be OK, if in all these locations the focusTraversable() always returns the same thing as isEnabled(). I just don't know if that is the case.

Even if this was OK, you would have to change focusTraversable() for menu/sub-menu because they are currently like this (from SubMenu.java):

   public boolean focusTraversable()
   {
      // The same as AbstractWidget.focusTraversable().
      // children focusability doesn't affect sub-menu focusability.
      return isVisible() && isEnabled() && !hidden();
   }

Changing that code could have unintended consequences elsewhere.

1. disabled menus can be selected via mouse or navigated via keys; in this case, focusTraversable for MENU should not check for ENABLED - only for VISIBLE and NOT HIDDEN. So you need to override focusTraversable() in MENU to fix this case.
2. please check all overrides of focusTraversable() for cases which do not return enabled && visible && !hidden. I ask this because replacing code like !isEnabled() with !focusTraversable() is OK, if the widget's focusTraversable() already checks for ENABLE. But code like isEnabled() replaced with focusTraversable() might be problematic: can FOCUS be set to a not-visible widget? Please try to find if this is possible or not.
3. same for code in FocusManager which replaces isEnabled() with isFocusable() - I don't think the replacement code is compatible with the existing logic. In P2J, AbstractWidget.isFocusable() impl just returns true, and other widgets override it to just return false. So is not OK, as isEnabled() is a widget state which is not static.

Vadim Gindin wrote:

1. please check all overrides of focusTraversable() for cases which do not return enabled && visible && !hidden. I ask this because replacing code like !isEnabled() with !focusTraversable() is OK, if the widget's focusTraversable() already checks for ENABLE. But code like isEnabled() replaced with focusTraversable() might be problematic: can FOCUS be set to a not-visible widget? Please try to find if this is possible or not.

I've tried to check that using FOCUS handle and I didn't find such possibility. Whenever a widget becomes hidden it looses a focus immediately: next enabled widget gains a focus. I've tried to hide focused widget with a trigger or right before WAIT-FOR statement. Is that check sufficient or there is some other way to check that?

What concerns me is the FocusManager.FOCUS_HIDDEN state. I don't recall the exact case, but from how this state is used, I think is possible to end up with a not-visible focused widget (or initial focus attempt, at least). More, this code in TC:15468:

      if (cur != null && !cur.isEnabled())

• focusTraversable() && isVisible() expands to enabled && visible && !hidden && visible, which is equivalent with isEnabled() && isVisible()
• !focusTraversable() || !isVisible() expands to !(enabled && visible && !hidden) || !visible, which is equivalent with !enabled || !visible || hidden || !visible

But replacing a singular isEnabled() with focusTraversable() adds visibility check in the game, and the result is no longer equivalent.

Have a look at the last diff? Can I commit the changes as usual?

Yes, do add them to 2909a (also). I would suggest running a MAJIC runtime testing with this rev, to see if anything fails related to the focusTraversable() changes.

I've committed current changes and merged branch 2909a with trunk. Current rev #10963.

So what about runtime testing? Can I run it?

Vadim: as Constantin wrote in note 13, you can try running MAJIC regression testing on this branch. Please report the results here.

Runtime regression has failed. No results folder were created. Here is the log:

..
     [exec] Result = STATUS_STOPPED
     [exec]  remove_jar
     [exec] ------------
     [exec]
     [exec] (1 row)
     [exec]
     [exec]  install_jar
     [exec] -------------
     [exec]
     [exec] (1 row)
     [exec]
     [exec]  set_classpath
     [exec] ---------------
     [exec]
     [exec] (1 row)
     [exec]
     [exec] ANALYZE
     [exec] ** <confidential information redacted>
     [exec] ** Starting GSO server...
     [exec] ** Starting RFQ server...
     [exec] ** Waiting for servers to start (60 seconds max)...
     [exec] ...checking 1 out of 12
     [exec] ...checking 2 out of 12
     [exec] ...checking 3 out of 12
     [exec] ...checking 4 out of 12
     [exec] ...checking 5 out of 12
     [exec] ...checking 6 out of 12
     [exec] ...checking 7 out of 12
     [exec] ...checking 8 out of 12
     [exec] ...checking 9 out of 12
     [exec] ...checking 10 out of 12
     [exec] ...checking 11 out of 12
     [exec] ...checking 12 out of 12
     [exec] GSO server could not be started!

BUILD FAILED
/opt/secure/clients/timco/testing/build_rt.xml:473: exec returned: 1

..
dencode.p2j.persist.id.SequenceIdentityManager]
[01/10/2016 10:55:34 EST] (com.goldencode.p2j.persist.TempTableConnectionProvider:INFO) Temp table connection provider configured;  backed by com.goldencode.p2j.persist.UnpooledConnectionProvider
[01/10/2016 10:55:37 EST] (TemporaryAccountPool:WARNING) {00000000:00000001:rfq} Admin privileges are mandatory to create temporary account pool.
[01/10/2016 10:55:44 EST] (SessionManager.listen():WARNING) {00000000:00000001:rfq} INSECURE sockets in use!
[01/10/2016 10:55:44 EST] (SessionManager.listen():INFO) {00000000:00000001:rfq} Server ready
[01/10/2016 10:55:52 EST] (LogicalTerminal:WARNING) No client parameters are set: calling the client-side to determine the driver type is expensive!
[01/10/2016 10:55:52 EST] (com.goldencode.p2j.net.Protocol:WARNING) stopQueue
com.goldencode.p2j.net.SilentUnwindException: Connection ended abnormally
        at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1420)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:97)
        at com.sun.proxy.$Proxy13.isChui(Unknown Source)
        at com.goldencode.p2j.ui.LogicalTerminal.isChui(LogicalTerminal.java:8107)
        at com.goldencode.p2j.ui.LogicalTerminal.init(LogicalTerminal.java:962)
        at com.goldencode.p2j.security.ContextLocal.initializeValue(ContextLocal.java:578)
        at com.goldencode.p2j.security.ContextLocal.get(ContextLocal.java:418)
        at com.goldencode.p2j.security.ContextLocal.get(ContextLocal.java:373)
        at com.goldencode.p2j.ui.LogicalTerminal.locate(LogicalTerminal.java:10073)
        at com.goldencode.p2j.ui.LogicalTerminal.access$400(LogicalTerminal.java:727)
        at com.goldencode.p2j.ui.LogicalTerminal$3.createScopeable(LogicalTerminal.java:11034)
        at com.goldencode.p2j.util.TransactionManager$ContextContainer.obtain(TransactionManager.java:5794)
        at com.goldencode.p2j.util.TransactionManager.abortProcessing(TransactionManager.java:3754)
        at com.goldencode.p2j.net.SessionManager.endSession(SessionManager.java:1120)
        at com.goldencode.p2j.net.SessionManager.queueStopped(SessionManager.java:1587)
        at com.goldencode.p2j.net.Queue.stop(Queue.java:410)
        at com.goldencode.p2j.net.Protocol.stopQueue(Protocol.java:334)
        at com.goldencode.p2j.net.Protocol.access$600(Protocol.java:106)
        at com.goldencode.p2j.net.Protocol$Reader.run(Protocol.java:439)
        at java.lang.Thread.run(Thread.java:745)

LE: GES has removed from confidential information from this entry.

Sometimes the GSO server startup can time out. Did you try starting it again?

Greg Shah wrote:

Sometimes the GSO server startup can time out. Did you try starting it again?

I've tried to start it about a half hour ago - and I've got the same error.

Try it again. If it still fails, dig into the logs and determine the problem.

The third run was successful. Servers are started. I'll post results later.

Runtime regression testing is still running. Probably it hang up..

Wait for it to complete and then look at the results. Most likely things are badly broken because of the changes in your branch. As noted above, Constantin and I both thought your changes were dangerous. When things are badly broken, the testing takes a very long time because every test that fails must wait 10 minutes to timeout.

Greg Shah wrote:

Wait for it to complete and then look at the results. Most likely things are badly broken because of the changes in your branch. As noted above, Constantin and I both thought your changes were dangerous. When things are badly broken, the testing takes a very long time because every test that fails must wait 10 minutes to timeout.

It is still working.. How long it could be in a worst case?

Vadim Gindin wrote:

Greg Shah wrote:

Wait for it to complete and then look at the results. Most likely things are badly broken because of the changes in your branch. As noted above, Constantin and I both thought your changes were dangerous. When things are badly broken, the testing takes a very long time because every test that fails must wait 10 minutes to timeout.

It is still working.. How long it could be in a worst case?

Did CTRL-C finish? I think you can assume the servers are either deadlocked or frozen. Please check a jstack log to see which tests are executing (if any) and for any deadlocks.

Also, try connecting manually with a client to your devsrv01 server - does it work? If so, try executing a few random scenarios (look in the generated logs from a previous good run).

Vadim Gindin wrote:

CTRL-C tests are not finished. Here is jstack log for GSO server.

OK, I think this means the harness has reached a deadlock. Please do this: start a client manually and execute the first steps in a CTRL-C test (i.e. from main-menu, V, C select RFQ, ENTER) see if screens are the same.

There should be a failure in the steps somewhere.

Constantin Asofiei wrote:

Vadim Gindin wrote:

CTRL-C tests are not finished. Here is jstack log for GSO server.

OK, I think this means the harness has reached a deadlock. Please do this: start a client manually and execute the first steps in a CTRL-C test (i.e. from main-menu, V, C select RFQ, ENTER) see if screens are the same.

There should be a failure in the steps somewhere.

I've executed proposed steps without errors.

Vadim Gindin wrote:

I've executed proposed steps without errors.

Please re-start CTRL-C testing, maybe it was some load-issue on devsrv01...

Vadim Gindin wrote:

Constantin Asofiei wrote:

Please re-start CTRL-C testing, maybe it was some load-issue on devsrv01...

Item seems, that was a real reason. Thank you! Btw, how did you see in the jstack log, that exactly harness is locked? I'm just interesting.

It's a matter of exclusion, if P2J server is OK, then something must have happened with the harness.

I restarted regression testing yesterday. It finished after 3 hours with the following failing tests (see also attached log):

I think these are real issues. Please manually check the failed tests to find why they fail.

I'm looking at gso_233. The fail happen when I press "Page Down" in "System Code Browser". There is a paged files list and paging does not work: no effect of the "PgDwn" press. I've searched the string "System Code Browser" in the MAJIC sources and found src/syman/util/b-syst.p, that includes tools/stebrows.i containing description of DOWN-frame. Therefore I suppose that my changes could fail on all down-frames and I'm going to check that. Have I correctly reasoned?

Vadim Gindin wrote:

I'm looking at gso_233. The fail happen when I press "Page Down" in "System Code Browser". There is a paged files list and paging does not work: no effect of the "PgDwn" press. I've searched the string "System Code Browser" in the MAJIC sources and found src/syman/util/b-syst.p, that includes tools/stebrows.i containing description of DOWN-frame. Therefore I suppose that my changes could fail on all down-frames and I'm going to check that. Have I correctly reasoned?

Yes, your reasoning is OK. You can look into the b-syst.p.cache file to see how the preprocessed 4GL program looks like. Also, beside writing a standalone test which duplicates the behaviour, you can debug the P2J client connected to MAJIC.

Vadim: as mentioned in our prior discussions, your changes were expected (by Constantin and myself) to be very dangerous. I really doubt there is simple fix that will resolve all the regressions.

This work has already gone on a very long time. My idea with having you run testing was to see how close you are to a solution that we can accept. I think the result of the testing shows that the current proposed solution is not acceptable.

Instead of spending a lot of time on fixing regressions, please look to take a different (and much safer) approach to the original problem of this task.

I've made new solution. See branch 2909b. I've added new method navigable() to Widget that saves old implementation, and I override it for menu widgets to replace enabled() with focusTraversable(). I've ran regression testing. Here is the list of failed tests:

gso_tests: gso_375, gso_430, tc_codes_employees_024, tc_job_002, tc_po_item_003

I'm working on fixing regressions.

tc_job_002, tc_po_item_003

These are known issues, not your regressions.

gso_375, gso_430, tc_codes_employees_024

This may be real issues, but you should re-run testing (or manually check these tests).

I've manually checked these tests using run_single.sh script and manually running the client. Here are the results:
gso_375 and tc_codes_employees_024 are passed, but gso_430 failed to run with the following log:

vig@devsrv01:~/testing/majic_baseline$ ./run_single.sh
Cannot load file (bad content).
No such mutex name job_transaction.

------------------Automated TTY Test Harness-------------------

Syntax:

java com.goldencode.harness.Harness [options] <test_plan_filename>

Options:

-?                    = display this help screen
-b <path_list>        = specify a default set of baseline search
                        paths; <path_list> is a list of one or more
                        paths separated by the platform specific

What does it mean?

Manual client testing gave the following result:

Manually and automatically :

01/22/2016              TIME & ATTENDANCE BADGE READER                  17:45:49
                                                                      SCAN BADGE
ClockIn  1745 XXXXXXXXX, XXXX (confidential info redacted)

08/23/2009              TIME & ATTENDANCE BADGE READER                  18:26:55
                                                                      SCAN BADGE

ClockIn  1826 XXXXXXXXX, XXXX (confidential info redacted)

┌──────────────────────────────────────────────────────────────────────────────┐
│INFO: T&A post-time rounded to nearest 1/4 hour. (1)                          │
└──────────────────────────────────────────────────────────────────────────────┘

Code Review Task Branch 2909b Revision 10965

This seems to be a much smarter approach.

Constantin: please review.

1. The references to "mouse" in this javadoc seem to be incorrect. This method is used in both key and mouse navigation.

   /**
    * Returns true only if this widget is navigable using mouse.
    *
    * @return  true if widget is navigable using mouse.
    */
   @Override
   public boolean navigable()

Please change this to be more accurate in all 4 locations where it occurs (AbstractWidget, Widget, MenuItemGuiImpl and SubMenuGuiImpl).

2. I think the debugging changes in AbstractGuiDriver should be reverted.

3. MenuItemChuiImpl needs a history entry.

What does it mean?

That test is dependent upon a mutex semaphore that doesn't exist. It isn't compatible with running in single mode.

Manual client testing gave the following result:

When you ran it, the time was 17:45 which is already at the quarter hour. For that reason the code doesn't have to round the value to the nearest quarter hour (and then print a notice about the rounding). If you test it again, I think it will pass.

I rerun full regression testing yesterday. Here are the failing tests: gso_269, tc_codes_employees_019, tc_job_002, tc_job_clock_002, tc_po_item_003.

Intersections with previous testing results are:tc_po_item_003, tc_job_002. As Greg wrote these regressions are not mine. So can I merge changes to trunk?

I've committed remarks fixes "javadoc fixes, history entry, removing debugging code". Branch revision 10966.

• MenuItem: History entry says "Changed focusTraversable(): removed isEnabled() check." but actually the focusTraversable() is removed from the class
• SubMenuGuiImpl: there is this comment which I think no longer applies to the code?
  

           /* TODO: pop-up menu: when sub-menu become active using keyboard: don't show body.
            * That is why this block is commented.
            */ 
  

• SubMenuGuiImpl: you have a System.out.println on line 1254.

Have you tested with other menu-related tests (beside what you are working now)? The idea is to not regress something else, menu-related.

Constantin Asofiei wrote:

Review for 2909b rev 10966:

• MenuItem: History entry says "Changed focusTraversable(): removed isEnabled() check." but actually the focusTraversable() is removed from the class
• SubMenuGuiImpl: there is this comment which I think no longer applies to the code?
  [...]
• SubMenuGuiImpl: you have a System.out.println on line 1254.

Have you tested with other menu-related tests (beside what you are working now)? The idea is to not regress something else, menu-related.

Yes, I've rechecked basic tests again. They work.

I've fixed header and removed specified comment and debug code.

Code Review Task Branch 2909b Revision 10967

The MenuItem history entry just says "focusTraversable() is.". I think it should say "focusTraversable() is removed." right?

What revision did you use for your regression testing?

Greg Shah wrote:

Code Review Task Branch 2909b Revision 10967

The MenuItem history entry just says "focusTraversable() is.". I think it should say "focusTraversable() is removed." right?

Yes, sorry.

What revision did you use for your regression testing?

I used revision 10965. All subsequent commits are just comments and debug code removing and header and javadoc fixes.

Vadi, you still have TODO's and commented code (like /* !oldFocus.isEnabled()*/) in ThinClient and FocusManager - please clean these too.

Done. By the way EmulatedWindowState is also write a lot of debug info to System.err. Does it really needed or we can remove it too?

By the way EmulatedWindowState is also write a lot of debug info to System.err. Does it really needed or we can remove it too?

It is being used. Please leave it there.

Please merge task branch 2909b revision 10969 to trunk and then archive 2909b (normal archive as completed).

Please note that 2909a should be archived as a DEAD branch.

The branch 2909b is merged into trunk and committed as bzr revision #10965.