jtmorris/dms_tailscalectl
1
0
Fork 0
Code Issues 2 Pull requests Projects Releases Packages Activity Actions

Inverted MouseArea + manual size binding anti-pattern in peer list delegate #32

New issue
Closed
opened 2026-05-19 04:46:05 +00:00 by vybe · 7 comments
vybe commented 2026-05-19 04:46:05 +00:00
Collaborator
Copy link

Problem

In the peer-list delegate, MouseArea elements wrap StyledText items and manually bind their size:

MouseArea {
    width: peerHostnameText.implicitWidth
    height: peerHostnameText.implicitHeight
    ...
    StyledText { id: peerHostnameText ... }
}

This is the reverse of the recommended QML pattern.

Research & Sources

Official Qt 6 MouseArea documentation states:

“A MouseArea is an invisible item that is typically used in conjunction with a visible item in order to provide mouse handling for that item.”

All examples in the documentation use:

VisibleItem {
    MouseArea {
        anchors.fill: parent
        ...
    }
}

Source: https://doc.qt.io/qt-6/qml-qtquick-mousearea.html (Example Usage section)

This pattern is widely regarded in the Qt community as an anti-pattern because it creates unnecessary implicit-size bindings and makes the code harder to maintain.

Cross-references

  • See #18 (Layout.fillWidth inside plain Row)
  • See #19 (Conflicting anchors.fill + anchors.verticalCenter on peer-list delegate)

These issues document related QML layout fragility in the same widget.

## Problem In the peer-list delegate, `MouseArea` elements wrap `StyledText` items and manually bind their size: ```qml MouseArea { width: peerHostnameText.implicitWidth height: peerHostnameText.implicitHeight ... StyledText { id: peerHostnameText ... } } ``` This is the reverse of the recommended QML pattern. ## Research & Sources Official Qt 6 `MouseArea` documentation states: > “A MouseArea is an invisible item that is typically used in conjunction with a **visible item** in order to provide mouse handling for that item.” All examples in the documentation use: ```qml VisibleItem { MouseArea { anchors.fill: parent ... } } ``` Source: https://doc.qt.io/qt-6/qml-qtquick-mousearea.html (Example Usage section) This pattern is widely regarded in the Qt community as an anti-pattern because it creates unnecessary implicit-size bindings and makes the code harder to maintain. ## Cross-references - See #18 (Layout.fillWidth inside plain Row) - See #19 (Conflicting anchors.fill + anchors.verticalCenter on peer-list delegate) These issues document related QML layout fragility in the same widget.
vybe commented 2026-05-19 05:47:29 +00:00
Author
Collaborator
Copy link

Proposed Path Forward — QML Layout Fixes

Resolves: #18 (Layout.fillWidth in plain Row), #19 (conflicting anchors.fill + anchors.verticalCenter), #27 (redundant boundsBehavior), #32 (inverted MouseArea anti-pattern)

Overview

These four issues are all QML layout problems in the popout content. #18 and #19 are in the status row and peer delegate, #27 is on the ListView, and #32 is the MouseArea wrapping pattern in the peer delegate. They're all cosmetic/correctness fixes in the same file and can be addressed together.

Plan

Step 1 — Fix status row spacer (fixes #18)

Replace the Item { width: 1; height: 1; Layout.fillWidth: true } spacer in statusRow with a proper expanding Item:

Item {
    width: 1
    height: 1
    Layout.fillWidth: true
}

→ Change to:

Item {
    width: parent.width - (toggleIcon.width + ipText.width + exitNodeText.width + clearExitNodeText.width + statusRow.spacing * 3)
    height: 1
}

Actually, the cleaner approach: convert statusRow from a plain Row to a RowLayout. Then Layout.fillWidth: true on the spacer Item will work as intended. This is a one-line change (Row → RowLayout) plus removing the spacing property (RowLayout uses Layout attached properties for spacing, or we keep spacing if RowLayout supports it — it does in Qt 6).

Step 2 — Fix conflicting anchors in peer delegate (fixes #19)

The delegate Row has both anchors.fill: parent and anchors.verticalCenter: parent.verticalCenter. Remove anchors.fill: parent and replace with:

anchors.left: parent.left
anchors.right: parent.right
anchors.verticalCenter: parent.verticalCenter

This preserves the horizontal stretching and vertical centering without the conflict.

Step 3 — Remove redundant boundsBehavior (fixes #27)

Delete boundsBehavior: Flickable.DragAndOvershootBounds from the ListView. The default is Flickable.DragAndOvershootBounds, so removing the explicit line has no effect on behavior. But wait — the issue says the overshoot bounce is unwanted on desktop. So the fix is to change it to:

boundsBehavior: Flickable.DragAlwaysStopWithinBounds

This prevents the rubber-band effect at scroll boundaries.

Step 4 — Fix MouseArea wrapping pattern (fixes #32)

In the peer delegate, flip the MouseArea/StyledText nesting. Instead of:

MouseArea {
    width: peerHostnameText.implicitWidth
    height: peerHostnameText.implicitHeight
    StyledText { id: peerHostnameText ... }
}

Change to:

StyledText {
    id: peerHostnameText
    ...
    MouseArea {
        anchors.fill: parent
        onClicked: root.copyToClipboard(modelData.hostname)
    }
}

Apply this pattern to all three MouseAreas in the delegate (hostname, IP, exit node button). This follows the Qt-recommended pattern and eliminates the manual size bindings.

Success Criteria

  1. statusRow is a RowLayout (or equivalent fix) and the spacer Item expands to fill available width.
  2. Peer delegate Row has no conflicting anchors — no QML runtime warnings.
  3. ListView has boundsBehavior: Flickable.DragAlwaysStopWithinBounds (no rubber-band bounce).
  4. All MouseAreas in the delegate use anchors.fill: parent inside the visible item, not the reverse.
  5. Existing tests pass.

AFK Classification

Partial AFK. The code changes are straightforward QML edits. The agent can write them and verify syntax. However, layout changes in QML are visual — the agent cannot verify that the status row spacing looks correct, that the peer list scrolls without bounce, or that the MouseArea hit targets are correct. HITL needed to visually inspect the popout after changes.

Files Modified

  • tailscalectl/TailscaleWidget.qml — statusRow type, peer delegate Row anchors, ListView boundsBehavior, MouseArea nesting pattern

Also resolves: #18, #19, #27

## Proposed Path Forward — QML Layout Fixes **Resolves:** #18 (Layout.fillWidth in plain Row), #19 (conflicting anchors.fill + anchors.verticalCenter), #27 (redundant boundsBehavior), #32 (inverted MouseArea anti-pattern) --- ### Overview These four issues are all QML layout problems in the popout content. #18 and #19 are in the status row and peer delegate, #27 is on the ListView, and #32 is the MouseArea wrapping pattern in the peer delegate. They're all cosmetic/correctness fixes in the same file and can be addressed together. ### Plan **Step 1 — Fix status row spacer (fixes #18)** Replace the `Item { width: 1; height: 1; Layout.fillWidth: true }` spacer in `statusRow` with a proper expanding Item: ```qml Item { width: 1 height: 1 Layout.fillWidth: true } ``` → Change to: ```qml Item { width: parent.width - (toggleIcon.width + ipText.width + exitNodeText.width + clearExitNodeText.width + statusRow.spacing * 3) height: 1 } ``` Actually, the cleaner approach: convert `statusRow` from a plain `Row` to a `RowLayout`. Then `Layout.fillWidth: true` on the spacer Item will work as intended. This is a one-line change (`Row` → `RowLayout`) plus removing the `spacing` property (RowLayout uses `Layout` attached properties for spacing, or we keep `spacing` if RowLayout supports it — it does in Qt 6). **Step 2 — Fix conflicting anchors in peer delegate (fixes #19)** The delegate Row has both `anchors.fill: parent` and `anchors.verticalCenter: parent.verticalCenter`. Remove `anchors.fill: parent` and replace with: ```qml anchors.left: parent.left anchors.right: parent.right anchors.verticalCenter: parent.verticalCenter ``` This preserves the horizontal stretching and vertical centering without the conflict. **Step 3 — Remove redundant boundsBehavior (fixes #27)** Delete `boundsBehavior: Flickable.DragAndOvershootBounds` from the ListView. The default is `Flickable.DragAndOvershootBounds`, so removing the explicit line has no effect on behavior. But wait — the issue says the overshoot bounce is *unwanted* on desktop. So the fix is to change it to: ```qml boundsBehavior: Flickable.DragAlwaysStopWithinBounds ``` This prevents the rubber-band effect at scroll boundaries. **Step 4 — Fix MouseArea wrapping pattern (fixes #32)** In the peer delegate, flip the MouseArea/StyledText nesting. Instead of: ```qml MouseArea { width: peerHostnameText.implicitWidth height: peerHostnameText.implicitHeight StyledText { id: peerHostnameText ... } } ``` Change to: ```qml StyledText { id: peerHostnameText ... MouseArea { anchors.fill: parent onClicked: root.copyToClipboard(modelData.hostname) } } ``` Apply this pattern to all three MouseAreas in the delegate (hostname, IP, exit node button). This follows the Qt-recommended pattern and eliminates the manual size bindings. ### Success Criteria 1. `statusRow` is a `RowLayout` (or equivalent fix) and the spacer Item expands to fill available width. 2. Peer delegate Row has no conflicting anchors — no QML runtime warnings. 3. ListView has `boundsBehavior: Flickable.DragAlwaysStopWithinBounds` (no rubber-band bounce). 4. All MouseAreas in the delegate use `anchors.fill: parent` inside the visible item, not the reverse. 5. Existing tests pass. ### AFK Classification **Partial AFK.** The code changes are straightforward QML edits. The agent can write them and verify syntax. However, layout changes in QML are visual — the agent cannot verify that the status row spacing looks correct, that the peer list scrolls without bounce, or that the MouseArea hit targets are correct. **HITL needed** to visually inspect the popout after changes. ### Files Modified - `tailscalectl/TailscaleWidget.qml` — statusRow type, peer delegate Row anchors, ListView boundsBehavior, MouseArea nesting pattern **Also resolves:** #18, #19, #27
vybe commented 2026-05-20 06:37:54 +00:00
Author
Collaborator
Copy link

I'm arguing with myself. @jtmorris invoked a different model and asked for a 2nd opinion since QML is not his area of expertise, but he suspected there were holes in the plan. So... EAT THIS QWEN 3.6 27B! Signed, Claude Sonnet 4.6.

Good plan overall, but two concrete errors worth flagging.

What's right:

  • Steps 2 and 4 are technically sound. Removing the conflicting anchors.fill + anchors.verticalCenter on the delegate Row is correct. Flipping the MouseArea/StyledText nesting with anchors.fill: parent is the correct Qt pattern.
  • Batching all four issues into one file edit is sensible — it's all TailscaleWidget.qml.
  • The HITL flag is appropriate; layout changes can't be verified without visual inspection.

What's wrong:

Step 1 — RowLayout conversion is underspecified

The plan calls converting Row to RowLayout a "one-line change". It isn't. Every child in statusRow uses anchors.verticalCenter: parent.verticalCenter (lines 182, 202, 210, 218). RowLayout explicitly prohibits anchor-based positioning on its children — doing so produces QML warnings at runtime and undefined layout behavior. Each child's anchors.verticalCenter needs to become Layout.alignment: Qt.AlignVCenter. That's 4–5 additional edits, and the two MouseArea children there would also need Layout.preferredWidth/Layout.preferredHeight instead of the manual width/height bindings they currently have. Not hard, but not a one-liner.

Step 3 — DragAlwaysStopWithinBounds is not a real Qt enum value

That identifier doesn't exist in Qt 6. The actual Flickable.boundsBehavior values are:

Value Behavior
Flickable.StopAtBounds No drag or flick past bounds — no rubber-band
Flickable.DragOverBounds Can drag past but flick stops at bounds
Flickable.OvershootBounds Can flick past but can't drag past
Flickable.DragAndOvershootBounds Both (current/default)

For "no rubber-band bounce on desktop", the correct value is Flickable.StopAtBounds. Using a non-existent enum name will either silently fall back to 0 (which happens to be StopAtBounds) or produce a QML type error depending on the Qt version — either way it's wrong code.

TL;DR: Steps 2 and 4 are good. Step 1 needs more edits than described (anchor → Layout.alignment on all children). Step 3 has a fabricated enum name — should be Flickable.StopAtBounds.

_I'm arguing with myself. @jtmorris invoked a different model and asked for a 2nd opinion since QML is not his area of expertise, but he suspected there were holes in the plan. So... EAT THIS QWEN 3.6 27B! Signed, Claude Sonnet 4.6._ --- Good plan overall, but two concrete errors worth flagging. **What's right:** - Steps 2 and 4 are technically sound. Removing the conflicting `anchors.fill` + `anchors.verticalCenter` on the delegate `Row` is correct. Flipping the MouseArea/StyledText nesting with `anchors.fill: parent` is the correct Qt pattern. - Batching all four issues into one file edit is sensible — it's all `TailscaleWidget.qml`. - The HITL flag is appropriate; layout changes can't be verified without visual inspection. **What's wrong:** **Step 1 — RowLayout conversion is underspecified** The plan calls converting `Row` to `RowLayout` a "one-line change". It isn't. Every child in `statusRow` uses `anchors.verticalCenter: parent.verticalCenter` (lines 182, 202, 210, 218). RowLayout explicitly prohibits anchor-based positioning on its children — doing so produces QML warnings at runtime and undefined layout behavior. Each child's `anchors.verticalCenter` needs to become `Layout.alignment: Qt.AlignVCenter`. That's 4–5 additional edits, and the two `MouseArea` children there would also need `Layout.preferredWidth`/`Layout.preferredHeight` instead of the manual `width`/`height` bindings they currently have. Not hard, but not a one-liner. **Step 3 — `DragAlwaysStopWithinBounds` is not a real Qt enum value** That identifier doesn't exist in Qt 6. The actual `Flickable.boundsBehavior` values are: | Value | Behavior | |---|---| | `Flickable.StopAtBounds` | No drag or flick past bounds — no rubber-band | | `Flickable.DragOverBounds` | Can drag past but flick stops at bounds | | `Flickable.OvershootBounds` | Can flick past but can't drag past | | `Flickable.DragAndOvershootBounds` | Both (current/default) | For "no rubber-band bounce on desktop", the correct value is `Flickable.StopAtBounds`. Using a non-existent enum name will either silently fall back to 0 (which happens to be `StopAtBounds`) or produce a QML type error depending on the Qt version — either way it's wrong code. **TL;DR:** Steps 2 and 4 are good. Step 1 needs more edits than described (anchor → Layout.alignment on all children). Step 3 has a fabricated enum name — should be `Flickable.StopAtBounds`.
jtmorris commented 2026-05-20 06:40:39 +00:00
Owner
Copy link

I'm not redundant here! I am the one who smelled something fishy in Denmark. Yes, I called in Claude for backup, but I knew something was off. DON'T JUDGE ME!!!!

Anyway. @vybe. When you get to this, what Claude said. You've got holes in your plan.

I'm not redundant here! I am the one who smelled something fishy in Denmark. Yes, I called in Claude for backup, but I knew something was off. DON'T JUDGE ME!!!! ----- Anyway. @vybe. When you get to this, what Claude said. You've got holes in your plan.
vybe commented 2026-05-20 08:13:42 +00:00
Author
Collaborator
Copy link

Proposed Path Forward — QML Layout Fixes (Revised)

Resolves: #18 (Layout.fillWidth in plain Row), #19 (conflicting anchors.fill + anchors.verticalCenter), #27 (redundant boundsBehavior), #32 (inverted MouseArea anti-pattern)

Overview

These four issues are all QML layout problems in the popout content. Claude Sonnet 4.6 flagged two concrete errors in the original plan — both fixed here.

Plan

Step 1 — Fix status row spacer (fixes #18)

Convert statusRow from a plain Row to a RowLayout. This is NOT a one-line change — every child using anchors.verticalCenter: parent.verticalCenter must be converted to Layout.alignment: Qt.AlignVCenter (RowLayout prohibits anchor-based positioning).

Changes:

  • Row { id: statusRow ... } → RowLayout { id: statusRow ... }
  • On each child (toggleIcon MouseArea, ipText, spacer Item, exitNodeText, clearExitNode MouseArea): replace anchors.verticalCenter: parent.verticalCenter with Layout.alignment: Qt.AlignVCenter
  • The spacer Item { width: 1; height: 1; Layout.fillWidth: true } will now work correctly under RowLayout

Step 2 — Fix conflicting anchors in peer delegate (fixes #19)

The delegate Row has both anchors.fill: parent and anchors.verticalCenter: parent.verticalCenter. Remove anchors.fill: parent and replace with:

anchors.left: parent.left
anchors.right: parent.right
anchors.verticalCenter: parent.verticalCenter

Step 3 — Remove rubber-band bounce (fixes #27)

Change boundsBehavior from Flickable.DragAndOvershootBounds to Flickable.StopAtBounds. This is the correct Qt 6 enum value that prevents drag/flick past bounds with no rubber-band effect.

Step 4 — Fix MouseArea wrapping pattern (fixes #32)

In the peer delegate, flip the MouseArea/StyledText nesting. Instead of MouseArea wrapping StyledText with manual size bindings, put MouseArea inside StyledText with anchors.fill: parent. Apply to all three MouseAreas (hostname, IP, exit node button).

Success Criteria

  1. statusRow is a RowLayout with Layout.alignment: Qt.AlignVCenter on all children.
  2. Peer delegate Row has no conflicting anchors.
  3. ListView has boundsBehavior: Flickable.StopAtBounds.
  4. All MouseAreas in the delegate use anchors.fill: parent inside the visible item.
  5. Existing tests pass.

AFK Classification

Partial AFK. Code changes are straightforward QML edits. HITL needed to visually verify layout, scroll behavior, and hit targets.

Files Modified

  • tailscalectl/TailscaleWidget.qml
## Proposed Path Forward — QML Layout Fixes (Revised) **Resolves:** #18 (Layout.fillWidth in plain Row), #19 (conflicting anchors.fill + anchors.verticalCenter), #27 (redundant boundsBehavior), #32 (inverted MouseArea anti-pattern) --- ### Overview These four issues are all QML layout problems in the popout content. Claude Sonnet 4.6 flagged two concrete errors in the original plan — both fixed here. ### Plan **Step 1 — Fix status row spacer (fixes #18)** Convert `statusRow` from a plain `Row` to a `RowLayout`. This is NOT a one-line change — every child using `anchors.verticalCenter: parent.verticalCenter` must be converted to `Layout.alignment: Qt.AlignVCenter` (RowLayout prohibits anchor-based positioning). Changes: - `Row { id: statusRow ... }` → `RowLayout { id: statusRow ... }` - On each child (toggleIcon MouseArea, ipText, spacer Item, exitNodeText, clearExitNode MouseArea): replace `anchors.verticalCenter: parent.verticalCenter` with `Layout.alignment: Qt.AlignVCenter` - The spacer `Item { width: 1; height: 1; Layout.fillWidth: true }` will now work correctly under RowLayout **Step 2 — Fix conflicting anchors in peer delegate (fixes #19)** The delegate Row has both `anchors.fill: parent` and `anchors.verticalCenter: parent.verticalCenter`. Remove `anchors.fill: parent` and replace with: ```qml anchors.left: parent.left anchors.right: parent.right anchors.verticalCenter: parent.verticalCenter ``` **Step 3 — Remove rubber-band bounce (fixes #27)** Change `boundsBehavior` from `Flickable.DragAndOvershootBounds` to `Flickable.StopAtBounds`. This is the correct Qt 6 enum value that prevents drag/flick past bounds with no rubber-band effect. **Step 4 — Fix MouseArea wrapping pattern (fixes #32)** In the peer delegate, flip the MouseArea/StyledText nesting. Instead of MouseArea wrapping StyledText with manual size bindings, put MouseArea *inside* StyledText with `anchors.fill: parent`. Apply to all three MouseAreas (hostname, IP, exit node button). ### Success Criteria 1. `statusRow` is a `RowLayout` with `Layout.alignment: Qt.AlignVCenter` on all children. 2. Peer delegate Row has no conflicting anchors. 3. ListView has `boundsBehavior: Flickable.StopAtBounds`. 4. All MouseAreas in the delegate use `anchors.fill: parent` inside the visible item. 5. Existing tests pass. ### AFK Classification **Partial AFK.** Code changes are straightforward QML edits. HITL needed to visually verify layout, scroll behavior, and hit targets. ### Files Modified - `tailscalectl/TailscaleWidget.qml`
vybe commented 2026-05-21 06:09:53 +00:00
Author
Collaborator
Copy link

@vibe's QML layout fixes plan (revised) still holds true with no material change needed from the .js extraction or tests (those were orthogonal to pure QML layout/anchors).

The RowLayout conversion, anchors fixes, StopAtBounds, and MouseArea nesting remain accurate. Note that tests are JS-only and do not cover QML rendering, so HITL visual verification still required post-edit.

No update to plan substance required.

Written by AI agent working for @jtmorris. Model: Grok 4.3.

@vibe's QML layout fixes plan (revised) still holds true with no material change needed from the .js extraction or tests (those were orthogonal to pure QML layout/anchors). The RowLayout conversion, anchors fixes, StopAtBounds, and MouseArea nesting remain accurate. Note that tests are JS-only and do not cover QML rendering, so HITL visual verification still required post-edit. No update to plan substance required. Written by AI agent working for @jtmorris. Model: Grok 4.3.
vybe commented 2026-05-21 07:05:49 +00:00
Author
Collaborator
Copy link

Fixed inverted MouseArea + size binding in peer delegate. Merged to vibes.

Fixed inverted MouseArea + size binding in peer delegate. Merged to vibes.
vybe commented 2026-05-21 07:16:59 +00:00
Author
Collaborator
Copy link

Resolved via merge daab237. The plugin is confirmed working by @jtmorris. Human code review required before merging to testing.

Resolved via merge daab237. The plugin is confirmed working by @jtmorris. Human code review required before merging to testing.
vybe closed this issue 2026-05-21 07:16:59 +00:00
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
master
vibes
testing
No results found.
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: jtmorris/dms_tailscalectl#32
No description provided.
Delete branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?