FWD » Core Development » User Interface

This testcase:

DEF VAR fill1 AS CHARACTER NO-UNDO.
DEF BUTTON hidden_btn SIZE 4 BY .81.

DEFINE FRAME fmain
    fill1 AT ROW 2.71 COL 11.8 COLON-ALIGNED
          VIEW-AS FILL-IN
          SIZE 13.2 BY 1
    hidden_btn AT ROW 2.86 COL 22.6  NO-TAB-STOP

    WITH 1 DOWN KEEP-TAB-ORDER
    AT COL 1 ROW 1 SCROLLABLE SIZE 50 BY 10.

hidden_btn:HIDDEN           = TRUE.
fill1:HIDDEN                = TRUE.
fill1:VISIBLE               = TRUE.

FRAME fmain:HIDDEN          = FALSE.
ENABLE ALL WITH FRAME fmain.

WAIT-FOR CLOSE OF CURRENT-WINDOW.

In OE: --- In FWD:

• move the hidden widgets (not frames) on the top of the parent frame when the frame is set to visible (even if it is still visible)

Committed the changes to 11294a branch rev. 16469

Hynek, please review. Thanks!

With the changes makeFrameVisible duplicates the logic already in setChildrenVisible. Can these be deduplicated?

Also makeFrameVisible uses widgets for the z-order logic, but should fieldGroup.getWidgets() be used instead? See setChildrenVisible.

Hynek Cihlar wrote:

With the changes makeFrameVisible duplicates the logic already in setChildrenVisible. Can these be deduplicated?

I've added a method findHiddenWidgets that returns the list of the hidden widget of the frame to use it in setChildrenVisible and makeFrameVisible in moveToBatchWorker call to "deduplicate" that part of the code, but I think we should let the moveToBatchWorker call at the end of the methods to ensure the moving is made correctly.

Also makeFrameVisible uses widgets for the z-order logic, but should fieldGroup.getWidgets() be used instead? See setChildrenVisible.

Done. findHiddenWidgets method uses fieldGroup.getWidgets().

Please review rev. 16470.
Thank you!

• [MAJOR] functional GenericFrame.setChildrenVisible: findHiddenWidgets is called unconditionally regardless of the value parameter. The old code only populated moveToTopWidgets inside the if (isHidden[i++] && value) block, so hidden widgets were collected only when making children visible. Now when hiding children (value false), moveToBatchWorker will still unnecessarily move hidden widgets to the top of z-order. Guard the call with value true or pass an empty list when hiding.
• [MAJOR] style GenericFrame.makeFrameVisible: The findHiddenWidgets call at line 11700 exceeds the 110-character line length limit (116 characters). It should be wrapped, e.g.:
  

        List<GenericWidget<?>> moveToTopWidgets =
           findHiddenWidgets((List<GenericWidget<?>>) fieldGroup.getWidgets());
  

• [MINOR] performance GenericFrame.setChildrenVisible: Redundant iteration over the widget list. The isHidden[] array already caches each widget's _isHidden() state from a prior loop, but findHiddenWidgets re-queries _isHidden() on every widget. This adds a third pass over the widget list where the old code only had two. Consider reusing the isHidden[] array to build the move-to-top list.
• [MINOR] performance GenericFrame.makeFrameVisible: findHiddenWidgets calls _isHidden() on every widget, and the subsequent visibility loop also calls _isHidden() on each widget again. Since _isHidden() with flush=true triggers flushEnqueuedWidgetAttrs() on every invocation, this doubles the flush cost. Consider caching the hidden state.

Hynek Cihlar wrote:

• [MAJOR] functional GenericFrame.setChildrenVisible: findHiddenWidgets is called unconditionally regardless of the value parameter. The old code only populated moveToTopWidgets inside the if (isHidden[i++] && value) block, so hidden widgets were collected only when making children visible. Now when hiding children (value false), moveToBatchWorker will still unnecessarily move hidden widgets to the top of z-order. Guard the call with value true or pass an empty list when hiding.
• [MAJOR] style GenericFrame.makeFrameVisible: The findHiddenWidgets call at line 11700 exceeds the 110-character line length limit (116 characters). It should be wrapped, e.g.:
  [...]
• [MINOR] performance GenericFrame.setChildrenVisible: Redundant iteration over the widget list. The isHidden[] array already caches each widget's _isHidden() state from a prior loop, but findHiddenWidgets re-queries _isHidden() on every widget. This adds a third pass over the widget list where the old code only had two. Consider reusing the isHidden[] array to build the move-to-top list.
• [MINOR] performance GenericFrame.makeFrameVisible: findHiddenWidgets calls _isHidden() on every widget, and the subsequent visibility loop also calls _isHidden() on each widget again. Since _isHidden() with flush=true triggers flushEnqueuedWidgetAttrs() on every invocation, this doubles the flush cost. Consider caching the hidden state.

Fixed all of these and committed the changes to 11294a branch rev. 16471 .

Hynek, please review. Thanks!

Code review 11294a revisions 16468..16471

• [MAJOR] functional GenericFrame.makeFrameVisible: The hiddenIds cache is populated by iterating this.widgets[], but findHiddenWidgets is later called with fieldGroup.getWidgets(). The two collections are not guaranteed to hold the same widget set - GenericFrame.addDynamicWidget can skip the widgets[] array on a name hit-test (early return), while FieldGroup.addWidget unconditionally receives the widget via GenericWidget.setFrame. Widgets present only in fieldGroup.getWidgets() return null from the map and are silently treated as "not hidden" by the != null ? ... : false guard, so such hidden widgets will never be moved to top - reintroducing the exact z-order bug the patch is meant to fix. Please check whether this can be an issue when the two lists are not identical.
• [MAJOR] style GenericFrame.findHiddenWidgets: The method signature exceeds the project's 110-character line-length limit; wrap the parameter list onto additional lines.
• [MINOR] performance GenericFrame.findHiddenWidgets: hiddenIds.get(widget.getId()) is called twice per widget (once for null check, once for the value). Store the result once in a local Boolean or use hiddenIds.getOrDefault(widget.getId(), false).
• [MINOR] performance GenericFrame.setChildrenVisible: The refactor replaced a primitive boolean[] (O(1) indexed access, no boxing) with HashMap<Integer, Boolean>, incurring Integer / Boolean autoboxing, hash computation, and Node allocation per entry. Since both loops iterate the same widgets list in the same order, the original parallel boolean[] suffices and should be restored; only makeFrameVisible genuinely needs the id-keyed map because its two passes iterate different lists.
• [MINOR] style GenericFrame.setChildrenVisible: When value == false the expression value ? findHiddenWidgets(...) : new ArrayList<>() still allocates a throwaway empty list that is immediately discarded by moveToBatchWorker (which short-circuits on empty input). Guard just the findHiddenWidgets / moveToBatchWorker pair with if (value), or use Collections.emptyList(), to avoid the allocation and make the intent explicit.

Committed the new changes to 11294a rev. 16531.

I've tried to de-duplicate the logic of moving the widget to the top as you said in #11294-4, to use the fieldGroup widget list to be consistent with the client side (as in setChildrenVisible method), to introduce a cache for the hidden state in GenericFrame.makeFrameVisible method and keep the one from GenericFrame.setChildrenVisible as it is.

Hynek, please review.

Code review 11294a revisions 16527..16531

• [MAJOR] performance GenericFrame.makeFrameVisible: hiddenIds.getOrDefault(widget.getId(), widget._isHidden()) evaluates the default argument unconditionally (Java is strict), so _isHidden() runs for every fgWidgets entry even on cache hits. Since _isHidden() routes through getAttr(..., true) which performs flushEnqueuedWidgetAttrs(), this defeats the very cache the diff introduces. Use containsKey()/get() (or computeIfAbsent) so _isHidden() only runs on genuine cache misses.

• [MAJOR] style GenericFrame.moveHiddenWidgets: Javadoc declares @param hiddenIds but the actual parameter is named hidden. Update the Javadoc tag to @param hidden (or rename the parameter) so they match.

• [MAJOR] style GenericFrame.setChildrenVisible: The new call moveHiddenWidgets(widgets, isHidden); is indented with 8 spaces. The enclosing if (value) block opens at 6 spaces, so its body should be at 9 spaces per the project's 3-space indent convention. Re-indent the block body and closing brace accordingly.

• [MINOR] performance GenericFrame.makeFrameVisible: After building hidden[] via one pass over fgWidgets, moveHiddenWidgets iterates fgWidgets a second time to filter into hiddenWidgets. The two passes can be fused into one at this call site (build hiddenWidgets directly during the cache-resolution loop), saving a full traversal and the boolean[] bridge allocation. The moveHiddenWidgets helper can still be kept for setChildrenVisible, which genuinely needs the cache captured before its mutating loop.

• [MINOR] performance GenericFrame.setChildrenVisible: The move-to-top collection is now computed in a second pass inside moveHiddenWidgets. Previously it was produced as a side effect of the single pass that flipped visibility. Restoring the single-pass accumulation in setChildrenVisible (while still reusing the helper from makeFrameVisible) avoids N extra iterations and N redundant instanceof FrameWidget checks on every frame un-hide.

The changes from 11294a were overcomplicating things. I rebased the branch and switched to a cleaner approach.

Hynek: Please do a review for 11294a.

Code review 11294a revisions 16653..16658

• [MAJOR] functional GenericFrame.makeFrameVisible: The new move-to-top logic builds moveToTopWidgets by iterating the widgets[] array, but the sibling setChildrenVisible deliberately iterates fieldGroup.getWidgets() for this same purpose (see its comment that widgets order can be "inconsistent with the client containers ... when using MOVE-TO-TOP", introduced by history rev 391, with the move-to-top added in rev 401). A MOVE-TO-TOP — or the batch move itself, or a prior HIDDEN=FALSE/VIEW cycle — reorders only the field group (GenericFrame.toTop → FieldGroup.toTop) and never widgets[], so the two orderings diverge. Because moveToBatch applies moveToTop per z-order class with the last id ending topmost, iterating widgets[] can restack two or more simultaneously-hidden, overlapping, same-class widgets in the wrong relative z-order; once they are shown (e.g. ENABLE ALL) the overlap this change is meant to fix renders incorrectly, and the wrong order is also written back to the field group via syncMoveToState. Trigger: a frame reaching makeFrameVisible (e.g. via VIEW) that holds 2+ overlapping hidden same-class widgets after an earlier MOVE-TO-TOP or hide/show cycle diverged the two orderings. This reintroduces the exact widgets[]-vs-field-group inconsistency that rev 391 removed. Fix: iterate fieldGroup.getWidgets() in makeFrameVisible, matching setChildrenVisible.

I think this went over my head while refactoring Delia's approach. Please check 11294a/rev. 16659.

Code review 11294a revisions 16653..16659

There is one group of issues remaining, which share one root cause: that call moveToBatchWorker is unguarded and placed after _setVisible(true), so it fires on every makeFrameVisible() invocation rather than only on the hidden-visible transition that history entry 405 describes.

• [MAJOR] functional GenericFrame.makeFrameVisible: The new moveToBatchWorker(true, moveToTopWidgets) is unconditional, so it runs even when the frame is already visible, silently overriding an application-requested z-order for hidden widgets. prepareView's gate !frame._isHidden() || currentStatement != UIStatement.DISPLAY (5855) is satisfied by the first disjunct alone for a visible frame, so displayWorker (12055) -> view(data) -> prepareView -> makeFrameVisible() re-runs the batch on every DISPLAY ... WITH FRAME. Trigger (GUI; CHUI rejects MOVE-TO-BOTTOM on non-frame widgets with 4088): hidden_btn:HIDDEN = TRUE. / hidden_btn:MOVE-TO-BOTTOM(). / DISPLAY fill1 WITH FRAME fmain. — the MOVE-TO-BOTTOM is honored (AbstractWidget._setHidden does not detach the widget from its container, so Utils.moveToBottomInClass returns TRUE and the server state syncs), and the following DISPLAY silently undoes it. The batch also rewrites the 4GL handle chain via syncMoveToState -> moveInChain(true) (re-insert at the firstResource head) plus FieldGroup.toTop, so FIRST-CHILD/NEXT-SIBLING traversal flips as a side effect of a DISPLAY statement. Gate the batch on the visible->transition; wasVisible is already computed at 11839 but currently only pushed, never used.

• [MAJOR] functional GenericFrame.makeFrameVisible: On the DISPLAY path the batch runs before DISPLAY clears the HIDDEN flag, so it raises exactly the widgets DISPLAY is about to un-hide. prepareView calls makeFrameVisible() at 5857 and only afterwards does gw.setAttr("hidden", wcfg.hidden, false, ...) for the displayed widgets (5876-5880). The client genuinely performs the move — ThinClient.moveToTop (26461) -> AbstractWidget.moveToTop -> Utils.moveToTopInClass (5993) has no visible/hidden/realized filter — and AbstractContainer.draw() (603) paints the list forward, so last == topmost. Trigger: two overlapping fill-ins a and b in a frame, DISPLAY a b (b over a), then a:HIDDEN IN FRAME f = TRUE. / DISPLAY a WITH FRAME f. — a is raised, then un-hidden, and now paints over b. This contradicts both pre-existing, deliberately gated raise mechanisms: ThinClient 14283-14294 requires w.canMovetoTop() and WINDOW:KEEP-FRAME-Z-ORDER (both default false, so BUTTON/RECTANGLE/IMAGE were never raised), and AbstractWidget.afterConfigUpdateBase 3411-3414 raises only un-realized widgets. The new batch honors neither gate, so trunk and branch disagree for any realized hidden widget in a default window. Note also that setHidden implicitly hides the side label (1596-1607), so labels are collected and raised too.

• [MAJOR] functional GenericFrame.makeFrameVisible: The batch is issued before the client realizes and lays out the frame, so auto-placed widgets in a frame containing a HIDDEN field are physically mis-positioned on first display. flushDeferredDefBeforeVisible() force-pushes the ScreenDefinition synchronously (9084-9086), so the client Frame and its children exist and are parented to a TopLevelWindow — but config().realized is set only in Frame.realizeFrame (2928), reached from Frame.setVisible(true), which the subsequent view RPC triggers. ThinClient.moveToTop has no realized guard and all three of its checks pass, so Utils.moveToTopInClass physically reorders the live AbstractContainer.widgets list; realizeFrame then clears savedTabOrder, doLayoutWorker re-snapshots it from the reordered getContentPane().widgets(), and ZeroColumnLayout.calcLayout (~700) walks it sequentially, re-applying setLocation for every widget whose bc.row/bc.column is INV_COORD. Hidden widgets are not skipped in the placement loop, so the hidden widget still consumes its slot. Trigger: DEFINE FRAME f a b c WITH SIDE-LABELS. / b:HIDDEN IN FRAME f = TRUE. / VIEW FRAME f. — pane order becomes [a, c, b] and the realization layout places c in b's slot. Contrast the pre-existing call site setChildrenVisible (5722), where the frame is already realized so the reorder is pure z-order. Fix: defer the move until after the view RPC, or apply it in realizeFrame after doLayout() — where normalizeZOrder() is already called for exactly this reason ("z-order is normalized after layout is done; layout needs to use the original widget order", 2956-2962).

• [MAJOR] performance GenericFrame.makeFrameVisible: Adds a synchronous server->client round trip to every VIEW/DISPLAY of any frame holding at least one hidden widget. LogicalTerminal.moveToBatch (19231) returns Boolean[] through clientCall() -> session.transact, so it blocks; it is not enqueued/deferred like pushWidgetAttr. Since makeFrameVisible runs once per DISPLAY (see the first finding), a frame with a conditionally hidden field pays this on every iteration: form v1 v2 with frame f. / v2:hidden in frame f = yes. / for each customer: display customer.name @ v1 with frame f. end. Precisely, this *doubles* the round trips on the affected path (the following @LogicalTerminal.view is also blocking), i.e. 1 -> 2 per DISPLAY, and it is a new RTT that did not exist on this path before. Per-widget client amplification: AbstractContainer.moveToTopInClass (1017) calls widgetStateChanged(), which recurses over every child widget, and Utils.moveToTopInClass runs normalizeZOrder over the whole container list even when np == pos; in web mode those mark ids in GuiWebDriver.stateChanged, forcing a full registerInteractiveWidgets() walk at the next input point. moveToBatchWorker does early-return on an empty list and WidgetConfig.hidden defaults to false, so frames with no explicitly hidden widgets are unaffected. Caveat on the fix: a bare !wasVisible gate would skip widgets whose HIDDEN is set after the frame became visible; a dirty-flag or piggy-backing the ids onto the following view call is safer.