FWD » Core Development » User Interface

Committed the complete solution to 11343a rev. 16504

• prevent the alert-box to be suddenly closed
• avoid the frozen app after the alert-box is closed
• prevent the generation of a second VALUE-CHANGED event for the COMBO-BOX

Hynek, please review.
Thanks!

Code review 11343a.

• [MAJOR] design ComboBox/ThinClient alert-box handling: The patch treats the symptom, not the cause. The root bug is that the keyboard selection path fires VALUE-CHANGED while the drop-down OverlayWindow is still on screen, whereas the mouse-click path closes the overlay first and then fires VALUE-CHANGED. The proper fix is to align the keyboard commit path with the mouse path (close the drop-down before firing the trigger). Doing so eliminates every secondary problem the current patch has to work around: no overlay-over-alert-box, no re-entrancy into the exit path from inside a running trigger (so no need for a visual-only half-hide), no duplicate VALUE-CHANGED on the post-modal close (so no need for deferredProgressTrigger = false), and no stale activeComboBox. The two new ComboBox methods (hideDropDownOverlay, hideDropDown) and the ThinClient plumbing around them can be removed entirely once the keyboard path is fixed at its source.
• [MAJOR] functional ThinClient alert-box path: The pre-modal and post-modal calls read the global activeComboBox field twice instead of saving a local. This is safe for an AlertBox (no widgets inside) but breaks the moment the modal can host another combo-box - e.g. if this code path is ever reached for a DialogBox containing its own COMBO-BOX. The inner combo-box's showDropDown() calls setActiveComboBox(this), clobbering the outer reference. On return from modalEventLoop, activeComboBox is either null (post-modal hideDropDown() skipped, leaving the outer drop-down wedged in the half-state created by hideDropDownOverlay(): overlay invisible but dropDown != null and deferredProgressTrigger still armed) or pointing at the inner combo-box (we then call hideDropDown() on the wrong widget, and the outer one is still wedged). Additionally, while a dialog-box modal runs, focus/event/wait-state code that consults dropDown != null on the outer combo-box gets a lying answer. Save a local ComboBox saved = activeComboBox; at entry and operate on saved both before and after - though see the [MAJOR] design point above: the better resolution is to remove this scaffolding altogether.
• [MAJOR] functional ComboBox.hideDropDownOverlay: Unsafe cast of dropDown to (DropDownGuiImpl) without an instanceof check. The field dropDown is typed as DropDown<O> (abstract) and there are two concrete subclasses: DropDownGuiImpl (GUI) and DropDownImpl (CHUI). The setActiveComboBox call in ComboBox.showDropDown() executes for both CHUI and GUI contexts, so activeComboBox can reference a CHUI combo-box whose dropDown is a DropDownImpl. This will throw ClassCastException if a message box appears while a CHUI combo-box drop-down is active. Use an instanceof guard or add hideOverlay() as a no-op on the abstract DropDown base class.
• [MAJOR] style ComboBox.hideDropDown: Javadoc summary line exceeds 110 characters (117 chars). The line "Hides the active drop-down of the combo-box and prevents the VALUE-CHANGED event after the drop-down is closed." should be wrapped.
• [MINOR] style ComboBox.hideDropDown: Typo in comment: "Do not firevaluechanged after the drop-down eas hidden like that" - missing space in "firevaluechanged" (should be "fire value-changed") and "eas" should be "was".
• [MINOR] style ComboBox: The explicit import com.goldencode.p2j.ui.client.gui.DropDownGuiImpl should use a wildcard (com.goldencode.p2j.ui.client.gui.*) per coding standards. Note: the pre-existing import for ComboBoxGuiImpl from the same package has the same issue, so the new import follows the existing pattern.

Replicating the mouse-click behavior for the keyboard selection case is not possible.

• It is guaranteed that a mouse-click on an item will close the drop-down. So, we can close the drop-down and defer the VALUE-CHANGED processing for later.

• The drop-down should not close. Instead, the VALUE-CHANGED event should propagate and execute its corresponding trigger, while in the drop-down event loop. As such, we can not just defer this event for later because the drop-down should not close in the first place. We must react only if a modal appears while executing this trigger.

What is actually happening¶

1. You select an item with the UP/DOWN keyboard keys. This will post a VALUE-CHANGED event.
2. The trigger executes and a modal window is invoked. There can really be any modal window, such as alert boxes, printer setup (through SYSTEM-DIALOG PRINTER-SETUP), modal frame (view-as dialog-box), file-chooser, etc...
3. The modal event loop starts with processing the OS events first. There is a WindowActivated (activated = false) on the overlay window which has the drop-down. This will eventually destroy the window and all of its widgets. When the dropdown is destroyed, TC.requestEndCurrentWaitForEventLoop() is invoked because the drop-down thinks it will interrupt its own event loop, but instead it ends the event loop of the modal window.
4. Now that the modal event loop is terminated, the modal disappears. FWD seems frozen because we are still stuck in the drop-down event loop, even if it was already destroyed.

For testing I'm using the following:

def var cbModalFrame as char view-as combo-box list-items "a", "b", "c", "d" inner-lines 5 label "Modal frame".
def var cbPrinter as char view-as combo-box list-items "a", "b", "c", "d" inner-lines 5 label "Printer".
def var cbAlertBox as char view-as combo-box list-items "a", "b", "c", "d" inner-lines 5 label "Alert-box".
def var cbFileChooser as char view-as combo-box list-items "a", "b", "c", "d" inner-lines 5 label "File chooser".

def frame f cbModalFrame skip(2) cbPrinter skip(2) cbAlertBox skip(2) cbFileChooser with side-labels.

on value-changed of cbModalFrame do:
    if cbModalFrame:screen-value = "c" then do:
        def var dummycb as char view-as combo-box list-items "1", "2", "3" label "dummy".
        def frame modal dummycb with view-as dialog-box.
        enable all with frame modal.
        wait-for close of frame modal.
    end.
end.

on value-changed of cbPrinter do:
    if cbPrinter:screen-value = "c" then do:
        SYSTEM-DIALOG PRINTER-SETUP.
    end.
end.

on value-changed of cbAlertbox do:
    if cbAlertbox:screen-value = "c" then do:
        message "alert-box" view-as alert-box.
    end.
end.

on value-changed of cbFileChooser do:
    if cbFileChooser:screen-value = "c" then do:
        def var fileName as char.
        SYSTEM-DIALOG GET-FILE fileName.
    end.
end.

enable all with frame f.
message "Click on any combo-box to activate the drop-down and scroll with the DOWN keyboard key until 'c' is selected. Drop-down should close because of the modal.".
wait-for window-close of current-window.

Hynek: Please review 11343a/rev. 16576.

I'm testing the changes and found an issue when opening a dialog-box from the value-changed trigger. See the attached 4gl example. When the dialog-box (or alert-box) is displayed, the event loop of the drop-down must not be already on the call stack.

I'll get this back to the review phase to get a response from Hynek for #11343-17.