fix(popup): prevent width jumping when content changes in Window popupType - #670
Open
Ivy233 wants to merge 1 commit into
Open
fix(popup): prevent width jumping when content changes in Window popupType#670Ivy233 wants to merge 1 commit into
Ivy233 wants to merge 1 commit into
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ivy233 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuidePrevents Popup.Window from shrinking and visibly jumping when content changes after opening by explicitly retaining the maximum implicit width and height for each open session, without reintroducing the binding loop. Sequence diagram for popup implicit-size ratchetsequenceDiagram
participant Popup
participant Content
participant Window
Popup->>Popup: onAboutToShow
Popup->>Popup: DS.Style.control.implicitWidth(control)
Popup->>Popup: DS.Style.control.implicitHeight(control)
Content-->>Popup: onImplicitContentWidthChanged
Popup->>Popup: Math.max(_maxImplicitWidth, implicitWidth)
Content-->>Popup: onImplicitContentHeightChanged
Popup->>Popup: Math.max(_maxImplicitHeight, implicitHeight)
Popup->>Window: implicitWidthChanged / implicitHeightChanged
Window->>Window: Retain maximum size
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="qt6/src/qml/Popup.qml" line_range="29-39" />
<code_context>
+ property real _maxImplicitWidth: 0
+ property real _maxImplicitHeight: 0
+
+ implicitWidth: Math.max(DS.Style.control.implicitWidth(control), control._maxImplicitWidth)
+ implicitHeight: Math.max(DS.Style.control.implicitHeight(control), control._maxImplicitHeight)
+
+ onAboutToShow: {
+ control._maxImplicitWidth = DS.Style.control.implicitWidth(control)
+ control._maxImplicitHeight = DS.Style.control.implicitHeight(control)
+ }
+ onImplicitContentWidthChanged: control._maxImplicitWidth =
+ Math.max(control._maxImplicitWidth, DS.Style.control.implicitWidth(control))
+ onImplicitContentHeightChanged: control._maxImplicitHeight =
+ Math.max(control._maxImplicitHeight, DS.Style.control.implicitHeight(control))
padding: DS.Style.popup.padding
</code_context>
<issue_to_address>
**issue (broader_impact):** The ratchet is applied unconditionally to every Popup, including Popup.Item, so a non-window popup remains at its largest width and height after its content shrinks during the same open session instead of resizing with the content. The change therefore introduces stale oversized item popups even though the reported regression is specific to Popup.Window.
**Triggers:** When a Popup.Item popup displays content whose implicit size shrinks while it remains open.
**Suggested fix:** Apply the ratchet only when `popupType === Popup.Window`, or keep the existing dynamic sizing for item popups.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Ivy233
force-pushed
the
fix/popup-width-jumping-373843
branch
from
August 25, 2026 09:21
04344d2 to
f90c919
Compare
18202781743
reviewed
Aug 25, 2026
| property real _maxImplicitWidth: 0 | ||
| property real _maxImplicitHeight: 0 | ||
|
|
||
| implicitWidth: control.popupType === Popup.Window |
Contributor
There was a problem hiding this comment.
implicitWidth赋值,不应该有这样复杂的判断,它默认就是那background和contentItem的implicitWidth,在background里赋值正确应该就可以了,
…pType After commit cba9773 removed the background Item's implicitWidth binding to break a binding loop, Popup's implicitWidth started tracking contentImplicitWidth directly. When popupType is Popup.Window, QQuickPopupWindow::implicitWidthChanged() unconditionally adopts popup->implicitWidth(), so search filtering that shrinks the content causes the popup window to jump to a smaller width. Add an explicit ratchet: once the popup is open, implicitWidth/Height only grow, never shrink. The old binding loop accidentally provided this behavior; this is a loop-free replacement. Instead of overriding implicitWidth/Height directly, keep the sticky-max via the background's (windowBlurComponent) implicit size, the natural source of the popup's default implicit size, and only for Popup.Window so Popup.Item keeps resizing with its content. 1. Add _maxImplicitWidth/_maxImplicitHeight ratchet state properties 2. Reset ratchet on aboutToShow for each open session 3. Update ratchet on implicitContentWidth/HeightChanged (grow only) 4. Apply the ratchet to the Popup.Window background's implicit size 修复 Popup 在 Window popupType 下宽度跳变问题 commit cba9773 移除 background Item 的 implicitWidth 绑定以消除绑定环后, Popup 的 implicitWidth 直接随 contentImplicitWidth 变化。当 popupType 为 Popup.Window 时,QQuickPopupWindow::implicitWidthChanged() 无条件采用 popup->implicitWidth(),搜索过滤导致内容减少时窗口宽度跳变。 添加显式棘轮机制:Popup 打开后 implicitWidth/Height 只增不减。旧的绑定环 意外提供了此行为,本修复是无绑定环的等价替代。棘轮不再直接覆盖 implicitWidth/Height,而是通过 background(windowBlurComponent) 的隐式尺寸 (Popup 默认隐式尺寸的自然来源)保持最大值,且仅作用于 Popup.Window, Popup.Item 仍随内容动态缩放。 PMS: BUG-373823、BUG-373843
Ivy233
force-pushed
the
fix/popup-width-jumping-373843
branch
from
August 26, 2026 05:04
f90c919 to
99ceb8d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After commit cba9773 removed the background Item's implicitWidth binding to break a binding loop, Popup's implicitWidth started tracking contentImplicitWidth directly. When popupType is Popup.Window, QQuickPopupWindow::implicitWidthChanged() unconditionally adopts popup->implicitWidth(), so search filtering that shrinks the content causes the popup window to jump to a smaller width.
Add an explicit ratchet: once the popup is open, implicitWidth/Height only grow, never shrink. The old binding loop accidentally provided this behavior; this is a loop-free replacement.
修复 Popup 在 Window popupType 下宽度跳变问题
commit cba9773 移除 background Item 的 implicitWidth 绑定以消除绑定环后, Popup 的 implicitWidth 直接随 contentImplicitWidth 变化。当 popupType 为 Popup.Window 时,QQuickPopupWindow::implicitWidthChanged() 无条件采用 popup->implicitWidth(),搜索过滤导致内容减少时窗口宽度跳变。
添加显式棘轮机制:Popup 打开后 implicitWidth/Height 只增不减。旧的绑定环 意外提供了此行为,本修复是无绑定环的等价替代。
PMS: BUG-373823、BUG-373843
Summary by Sourcery
Keep Window popup dimensions stable while content changes by retaining the largest implicit size throughout each open session.
Bug Fixes:
Enhancements: