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.