FWD » Core Development » User Interface

For keyboard events there is a specific order on which 4GL is looking for the right trigger to fire:
- key label
- key function
- any-printable (if appropriate)
- any-key

It is using that order first for exact widget, then for the widget with anywhere option and then it goes up the widget's parents tree and as last resort it looks for anywhere without any widget (default handler).

Current implementation is using recursion to lookup the trigger, this might be improved to call for all key events in a single pass (with or without anywhere) and there is still one case we've found where FWD is picking a different trigger compared to the 4GL.

Code review 6764.01.patch.

It is very nice that the result is a lot cleaner. I didn't find any particular issue from looking at the code.

I'm only worried about the removed browser-specific behavior and "frame/window propagation" in EventDefintion.lookup. Will these be missing for their specific use cases?

Also UiContainer.locateContainer javadoc deserves clarification. Sometimes it mentions "container", sometimes "frame", but it never mentions browse which can be returned, too. Also the statement "the given widget is itself a container" is also not accurate.

Stanislav: Please review.

Marian Edu wrote:

Hynek Cihlar wrote:

Code review 6764.01.patch.

I'm only worried about the removed browser-specific behavior and "frame/window propagation" in EventDefintion.lookup. Will these be missing for their specific use cases?

No, the browse use-case is handled by using the locateContainer in ThinClient when looking-up the event.

OK. What about the "frame/window propagation"?

Also UiContainer.locateContainer javadoc deserves clarification. Sometimes it mentions "container", sometimes "frame", but it never mentions browse which can be returned, too. Also the statement "the given widget is itself a container" is also not accurate.

Agreed, copy/paste is evil... tried to fix the comment, attached the new patch.

It looks good now.

One more thing. In UiUitls.getTopLevelWindow, top-level window for dialogs depends on the visibility of the immediate top-level window. It seems confusing, the window is still there, it's just not visible, why shouldn't it be returned? If you do need to consider the visibility state I would do it in the caller of the method.

Hynek Cihlar wrote:

OK. What about the "frame/window propagation"?

That is handled in lookupWorker, first walking through parent frames (UiUtils.getParentFrames) and then the 'top level window' (see below).

One more thing. In UiUitls.getTopLevelWindow, top-level window for dialogs depends on the visibility of the immediate top-level window. It seems confusing, the window is still there, it's just not visible, why shouldn't it be returned? If you do need to consider the visibility state I would do it in the caller of the method.

The code was 'extracted' as-is from the lookupWorker method, I can't really comment on why the window visibility for the dialog is being considered... it just looked like something that should be extracted out and UiUtils seemed like a good place to put it :(

Marian Edu wrote:

Hynek Cihlar wrote:

OK. What about the "frame/window propagation"?

That is handled in lookupWorker, first walking through parent frames (UiUtils.getParentFrames) and then the 'top level window' (see below).

OK.

One more thing. In UiUitls.getTopLevelWindow, top-level window for dialogs depends on the visibility of the immediate top-level window. It seems confusing, the window is still there, it's just not visible, why shouldn't it be returned? If you do need to consider the visibility state I would do it in the caller of the method.

The code was 'extracted' as-is from the lookupWorker method, I can't really comment on why the window visibility for the dialog is being considered... it just looked like something that should be extracted out and UiUtils seemed like a good place to put it :(

With this kind of specialized behavior it probably doesn't make sense to be placed in utils, what do you think?

Hynek Cihlar wrote:

With this kind of specialized behavior it probably doesn't make sense to be placed in utils, what do you think?

You might be right, maybe I can just move it back to EventList as aa private method... looking at it it does look like it will return null for a dialog if it's top level window isn't visible but as said I really don't understand why it is like that :(

Marian Edu wrote:

Attached the new patch.

Looks like attachments are not accepted, it gives an 'internal server error' instead :(

Hynek/Stanislav: Please review.

6764.03.patch

I've tested misc related cases for the browse and found no regressions. I'm OK with the changes for the browse part.

Hynek: Are you OK with the proposed patch?

Code review 6764.03.patch.

The changes look good, I didn't do any regression testing though. Just a few minor comments below.

In EventDefinition.lookup the condition for "global anywhere" excludes resource check, while resource check is in place in isAnywhereGlobally: widgetId == -1 && anywhere && !hasWidget(). Is this expected?

In EventList.lookupWorker:

         // lookup event for the widget and honor any-printable/any-key if needed (specific or anywhere)
         if (lookupEventHelper(wid, evCode, functionCode, trig, EventDefinition.SCOPE_SELF | EventDefinition.SCOPE_ANYWHERE, isKey, isPrintable,
                  result) != -1 || specificWidget)
         {
            return;
         }

Change ThinClient.getInstance().getWidget(parentId); back to tc.getWidget(parentId); in EventList.lookupWorker. tc variable is available in the scope.

Marian Edu wrote:

Hynek Cihlar wrote:

move specificWidget before lookupEventHelper.

Actually that is like that on purpose, the idea is to return if we've found something or we don't need to check the parent's chain for anywhere triggers.

I see, makes sense.

The changes look good, I have no more concerns.

Hynek: Please merge the changes into 3821a.

Greg Shah wrote:

Hynek: Please merge the changes into 3821a.

Will do, I assume you mean 3821c.

Yes, sorry.

6764.04.patch checked in 3821c revision 14285.

Is there anything left open in this task?

Greg Shah wrote:

Is there anything left open in this task?

I don't have anything left, all our tests in 4GL are passing so far although the only thing we've tested were UI triggers.

Marian Edu wrote:

Greg Shah wrote:

Is there anything left open in this task?

I don't have anything left, all our tests in 4GL are passing so far although the only thing we've tested were UI triggers.

I have a potential regression, I will confirm shortly.

Hynek Cihlar wrote:

Marian Edu wrote:

Greg Shah wrote:

Is there anything left open in this task?

I don't have anything left, all our tests in 4GL are passing so far although the only thing we've tested were UI triggers.

I have a potential regression, I will confirm shortly.

A false alarm. So nothing left on my side.

Hynek Cihlar wrote:

There is a regression after all. The test case is as follows:

[...]

This causes issue #6840.

I'm attaching a fix candidate. Marian, please review.

This is interesting, running the test I was puzzled as for why the ANYWHERE is not true for that EventDefinition - it was defined as ANYWHERE and then did look at your patch :)

I don't see any reason to reset ANYWHERE there since I've dropped the 'GLOBAL_ANYWHERE@ - this is an ANYWHERE with no widget/resource set.

The patch is good, thank you.

Marian Edu wrote:

The patch is good, thank you.

I will run some more regression tests before I check it in.

Hynek Cihlar wrote:

Marian Edu wrote:

The patch is good, thank you.

I will run some more regression tests before I check it in.

The method addEvent(String[] event, boolean anywhere) seems to be broken as it disregard the value for anywhere and send false instead but it looks like is not used anywhere in code (at least not in <large_gui_application>). The constructor with same signature is also disregarding the value of input parameter and always use false - at least one use of this one in <large_gui_application> (some_program.w - on alt-page-up, alt-page-down anywhere).

I think I need to add more combinations to the testcase I've used :(

Marian Edu wrote:

Hynek Cihlar wrote:

Marian Edu wrote:

The patch is good, thank you.

I will run some more regression tests before I check it in.

The method addEvent(String[] event, boolean anywhere) seems to be broken as it disregard the value for anywhere and send false instead but it looks like is not used anywhere in code (at least not in <large_gui_application>). The constructor with same signature is also disregarding the value of input parameter and always use false - at least one use of this one in <large_gui_application> (some_program.w - on alt-page-up, alt-page-down anywhere).

This looks suspicious and deserves attention. OTOH if it doesn't cause issues we can defer it. Looking in the history, it seems it's been in there since revision 11115.1.2.

This looks suspicious and deserves attention. OTOH if it doesn't cause issues we can defer it. Looking in the history, it seems it's been in there since revision 11115.1.2.

At a minimum, please put a big TODO and BUG comment in that code so that we can't miss it later. If you can fix it now in a reasonable amount of work, go ahead.

Hynek Cihlar wrote:

There is a regression in a customer ChUI application. The cause was the following removed from one of the EventDefinition constructors:

[...]

Marian, please review the attached patch.

I guess this will work, problem is where is this ctor called with anywhere set to false and no widget nor resource? I think it's only from those addEvent method and ctor where anywhere parameter is ignored and false is sent instead. I've fixed those already but the patch is good as a safeguard, we can assume an event with no widget/resource is anywhere (globally) I guess.

Marian Edu wrote:

Hynek Cihlar wrote:

There is a regression in a customer ChUI application. The cause was the following removed from one of the EventDefinition constructors:

[...]

Marian, please review the attached patch.

I guess this will work, problem is where is this ctor called with anywhere set to false and no widget nor resource? I think it's only from those addEvent method and ctor where anywhere parameter is ignored and false is sent instead. I've fixed those already but the patch is good as a safeguard, we can assume an event with no widget/resource is anywhere (globally) I guess.

This is the call stack where the definition gets created. It comes from the authentication plugin of the customer app.

<init>:306, EventDefinition {com.goldencode.p2j.ui}
addEvent:1314, EventList {com.goldencode.p2j.ui}
addEvent:907, EventList {com.goldencode.p2j.ui}
addEvent:1198, EventList {com.goldencode.p2j.ui}
<init>:332, EventList {com.goldencode.p2j.ui}
clientAuthHook:346, LoginClient {package censored}

Hynek Cihlar wrote:

This is the call stack where the definition gets created. It comes from the authentication plugin of the customer app.

[...]

The line numbers match with 3821c revision 14284.

Hynek Cihlar wrote:

The line numbers match with 3821c revision 14284.

Thanks for that Hynek, some more clean-up to do there...

Hynek: Please review.

Greg Shah wrote:

Hynek: Please review.

Greg, this will kinda re-open #6195 - if conversion isn't fixed (#6901) the code in mainwinfwd.w might be updated by adding anywhere on first event (line 312), proven this is allowed by conversion:

on end-error of W-Win anywhere ...

I'm inclined to resolve #6901 sooner rather than later.

Hynek: Please review the latest patch.

Marian: I'd like to get these changes and the fix for #6901 / #6930 into 3821c ASAP.

Marian Edu wrote:

An updated patch to solve some NPE issues (thanks Tomasz for reporting it), it's made against rev #14360.

When does the client send event code of -1 to LT.apply? Should this be prevented on the client so it doesn't happen at all? Should it be logged in LT.apply?

Hynek Cihlar wrote:

When does the client send event code of -1 to LT.apply? Should this be prevented on the client so it doesn't happen at all? Should it be logged in LT.apply?

This was not from the client but coming from an `apply lastkey` right after a message statement (that seems to simply reset lastkey).

Marian Edu wrote:

Hynek Cihlar wrote:

When does the client send event code of -1 to LT.apply? Should this be prevented on the client so it doesn't happen at all? Should it be logged in LT.apply?

This was not from the client but coming from an `apply lastkey` right after a message statement (that seems to simply reset lastkey).

I see. Can you describe this case in the comment at the return statement in LT.apply?

I have no more concerns.

Marian, Greg, I'd like to get 6764.06.patch in 3821c. Any objection?

Hi
What is the status of this issue ?
I'd like to move on with Regression Tests fix and events/triggers are next big thing to work on.
Shall I hold on ?

I'd like to get 6764.06.patch in 3821c. Any objection?

No objection from me.

Greg Shah wrote:

I'd like to get 6764.06.patch in 3821c. Any objection?

No objection from me.

None from me either, there is still widget specific code in thin-client/logical-terminal that might need some consideration but my tests are passing.

Marian Edu wrote:

Greg Shah wrote:

I'd like to get 6764.06.patch in 3821c. Any objection?

No objection from me.

None from me either, there is still widget specific code in thin-client/logical-terminal that might need some consideration but my tests are passing.

Marian, please add a file history entry to ThinClient.java and also fix the one in EventDefinition.java. Each entry must contain the author's initials.

6764.07.patch checked in to 3821c revision 14417.

Hynek Cihlar wrote:

6764.07.patch checked in to 3821c revision 14417.

I reverted the patch in 3821c revision 14420 due to a regression.

Please see #6667-268 for regression tests results for 6764.08.patch

Code review 6764.08.patch. The changes look good. Can I merge it to 3821c?

Yes, please go ahead.

6764.08.patch checked in to 3821c revision 14462.