fix: Popover positioning#111
Conversation
Generated by Claude Sonnet v4.5 https://github.com/minikin/popover/issues/103 (deleted)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe popover positioning now centers the popover on the attach rect by default and clamps horizontal/vertical offsets to fixed 8.0-pixel margins when centering would overflow the physical screen; branching and fallback logic tied to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/src/popover_position_render_object.dart (1)
118-139: Same indentation and shared-constant concerns as_horizontalOffsetabove.The centering logic itself is correct and mirrors the horizontal approach appropriately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/popover_position_render_object.dart` around lines 118 - 139, The _verticalOffset method uses a local const verticalMargin and inconsistent indentation; mirror the fix from _horizontalOffset by replacing the local verticalMargin with the shared margin constant used elsewhere (e.g. the same top-level/shared const name used by _horizontalOffset) and adjust the indentation to match surrounding code style; ensure you reference attachRect, PopoverUtils.physicalSize and size.height in the same ordering/format as _horizontalOffset so both functions share the same margin constant and consistent formatting.
🧹 Nitpick comments (1)
lib/src/popover_position_render_object.dart (1)
95-116: Misleading indentation: if/else bodies are at the same level as the control-flow keywords.Lines 105–106, 108–109, and 111–112 are indented at the same depth as the
if/else if/elsethey belong to, making the control flow hard to follow at a glance. The same issue is present in_verticalOffsetbelow.Also, the
8.0margin is duplicated as a localconstin both_horizontalOffsetand_verticalOffset. Consider extracting it to a single class-level or file-level constant (or making it a constructor parameter for configurability).♻️ Suggested fix (indentation + shared constant)
+ static const _defaultMargin = 8.0; + double _horizontalOffset(Size size) { var offset = 0.0; - const horizontalMargin = 8.0; // Proper horizontal margin instead of arrowHeight // Always try to center the popover first final centeredOffset = attachRect.left + attachRect.width / 2 - size.width / 2; // Check if centered popover would fit on screen with margins - if (centeredOffset >= horizontalMargin && - centeredOffset + size.width <= PopoverUtils.physicalSize.width - horizontalMargin) { - // Perfect centering case - offset = centeredOffset; - } else if (centeredOffset < horizontalMargin) { - // Popover would go off left edge, align to left margin - offset = horizontalMargin; + if (centeredOffset >= _defaultMargin && + centeredOffset + size.width <= PopoverUtils.physicalSize.width - _defaultMargin) { + // Perfect centering case + offset = centeredOffset; + } else if (centeredOffset < _defaultMargin) { + // Popover would go off left edge, align to left margin + offset = _defaultMargin; } else { - // Popover would go off right edge, align to right margin - offset = PopoverUtils.physicalSize.width - horizontalMargin - size.width; + // Popover would go off right edge, align to right margin + offset = PopoverUtils.physicalSize.width - _defaultMargin - size.width; } return offset; }Apply the same indentation fix and constant usage to
_verticalOffset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/popover_position_render_object.dart` around lines 95 - 116, The if/else branches in _horizontalOffset (and likewise in _verticalOffset) are misindented and hard to read; re-indent the bodies so they sit inside their respective if/else blocks and mirror that fix in _verticalOffset, and remove the duplicated local const horizontalMargin = 8.0 by extracting a shared constant (e.g., const double _popoverMargin = 8.0 at file- or class-level or accept it as a constructor parameter) and use that single symbol in both _horizontalOffset and _verticalOffset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/src/popover_position_render_object.dart`:
- Around line 118-139: The _verticalOffset method uses a local const
verticalMargin and inconsistent indentation; mirror the fix from
_horizontalOffset by replacing the local verticalMargin with the shared margin
constant used elsewhere (e.g. the same top-level/shared const name used by
_horizontalOffset) and adjust the indentation to match surrounding code style;
ensure you reference attachRect, PopoverUtils.physicalSize and size.height in
the same ordering/format as _horizontalOffset so both functions share the same
margin constant and consistent formatting.
---
Nitpick comments:
In `@lib/src/popover_position_render_object.dart`:
- Around line 95-116: The if/else branches in _horizontalOffset (and likewise in
_verticalOffset) are misindented and hard to read; re-indent the bodies so they
sit inside their respective if/else blocks and mirror that fix in
_verticalOffset, and remove the duplicated local const horizontalMargin = 8.0 by
extracting a shared constant (e.g., const double _popoverMargin = 8.0 at file-
or class-level or accept it as a constructor parameter) and use that single
symbol in both _horizontalOffset and _verticalOffset.
|
@userFortyTwo thank you for your contribution! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/popover_position_render_object.dart`:
- Around line 95-129: The clamping logic in _horizontalOffset(Size size) and
_verticalOffset(Size size) currently uses PopoverUtils.physicalSize (device
screen) but should use the render object's actual bounds set in performLayout;
replace references to PopoverUtils.physicalSize.width/height with
this.size.width/height (the render object's size) when checking and computing
the max offset and fallback positions so the popover is clamped to the render
object's bounds rather than the full screen.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afd88cd9-b1f5-4e5e-807d-f17c4a94e880
📒 Files selected for processing (1)
lib/src/popover_position_render_object.dart
| double _horizontalOffset(Size size) { | ||
| var offset = 0.0; | ||
| const horizontalMargin = 8.0; | ||
|
|
||
| if (attachRect.left > size.width / 2 && | ||
| PopoverUtils.physicalSize.width - attachRect.right > size.width / 2) { | ||
| offset = attachRect.left + attachRect.width / 2 - size.width / 2; | ||
| } else if (attachRect.left < size.width / 2) { | ||
| offset = arrowHeight; | ||
| final centeredOffset = | ||
| attachRect.left + attachRect.width / 2 - size.width / 2; | ||
|
|
||
| if (centeredOffset >= horizontalMargin && | ||
| centeredOffset + size.width <= | ||
| PopoverUtils.physicalSize.width - horizontalMargin) { | ||
| offset = centeredOffset; | ||
| } else if (centeredOffset < horizontalMargin) { | ||
| offset = horizontalMargin; | ||
| } else { | ||
| offset = PopoverUtils.physicalSize.width - arrowHeight - size.width; | ||
| offset = PopoverUtils.physicalSize.width - horizontalMargin - size.width; | ||
| } | ||
|
|
||
| return offset; | ||
| } | ||
|
|
||
| double _verticalOffset(Size size) { | ||
| var offset = 0.0; | ||
| const verticalMargin = 8.0; | ||
|
|
||
| if (attachRect.top > size.height / 2 && | ||
| PopoverUtils.physicalSize.height - attachRect.bottom > | ||
| size.height / 2) { | ||
| offset = attachRect.top + attachRect.height / 2 - size.height / 2; | ||
| } else if (attachRect.top < size.height / 2) { | ||
| offset = arrowHeight; | ||
| final centeredOffset = | ||
| attachRect.top + attachRect.height / 2 - size.height / 2; | ||
|
|
||
| if (centeredOffset >= verticalMargin && | ||
| centeredOffset + size.height <= | ||
| PopoverUtils.physicalSize.height - verticalMargin) { | ||
| offset = centeredOffset; | ||
| } else if (centeredOffset < verticalMargin) { | ||
| offset = verticalMargin; | ||
| } else { | ||
| offset = PopoverUtils.physicalSize.height - arrowHeight - size.height; | ||
| offset = PopoverUtils.physicalSize.height - verticalMargin - size.height; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file under review
head -150 lib/src/popover_position_render_object.dart | tail -60Repository: minikin/popover
Length of output: 1712
🏁 Script executed:
# Check the PopoverUtils definition
find . -name "popover_utils.dart" -type fRepository: minikin/popover
Length of output: 93
🏁 Script executed:
# Look at the performLayout method to understand the context
sed -n '40,80p' lib/src/popover_position_render_object.dartRepository: minikin/popover
Length of output: 1185
🏁 Script executed:
# Check PopoverUtils.physicalSize definition
cat lib/src/utils/popover_utils.dartRepository: minikin/popover
Length of output: 1436
🏁 Script executed:
# Find which widgets use PopoverPositionRenderObject
rg "PopoverPositionRenderObject" --type dartRepository: minikin/popover
Length of output: 405
🏁 Script executed:
# Look for the parent widget that creates this render object
rg "createRenderObject" lib/src/ --type dart -A 5Repository: minikin/popover
Length of output: 889
🏁 Script executed:
# Check for any CompositedTransformFollower or similar positioning
rg "CompositedTransformFollower\|Positioned\|Overlay" lib/src/ --type dartRepository: minikin/popover
Length of output: 41
🏁 Script executed:
# Look at the full popover_position_render_object.dart to understand the class
wc -l lib/src/popover_position_render_object.dartRepository: minikin/popover
Length of output: 106
🏁 Script executed:
# Read the full file to understand context
cat lib/src/popover_position_render_object.dartRepository: minikin/popover
Length of output: 3736
🏁 Script executed:
# Look at the widget that uses this render object
cat lib/src/popover_position_widget.dartRepository: minikin/popover
Length of output: 1055
🏁 Script executed:
# Check how PopoverPositionWidget is used
rg "PopoverPositionWidget" --type dart -B 2 -A 2Repository: minikin/popover
Length of output: 1008
🏁 Script executed:
# Look at the popover context to understand the full layout structure
head -100 lib/src/popover_context.dartRepository: minikin/popover
Length of output: 1612
🏁 Script executed:
# Look at PopoverItem to see how PopoverPositionWidget is used
cat lib/src/popover_item.dartRepository: minikin/popover
Length of output: 4384
🏁 Script executed:
# Check the Stack and how constraints flow through
rg "Stack\|Dialog\|ConstrainedBox" lib/src/ --type dart -B 2 -A 2Repository: minikin/popover
Length of output: 41
🏁 Script executed:
# Let's understand the coordinate system - check if there are any tests
find . -name "*test*" -type f | grep -i popoverRepository: minikin/popover
Length of output: 83
🏁 Script executed:
# Look at how Popover is shown - the entry point
rg "showPopover" lib/src/ --type dart -A 10Repository: minikin/popover
Length of output: 797
🏁 Script executed:
# Search for any documentation or examples
find . -name "*.md" -o -name "example*" | head -10Repository: minikin/popover
Length of output: 328
🏁 Script executed:
# Let's look at the test file to understand typical usage
cat test/popover_test.dartRepository: minikin/popover
Length of output: 14106
🏁 Script executed:
# Check the example to see how it's typically used
ls -la example/lib/Repository: minikin/popover
Length of output: 231
🏁 Script executed:
# Look for any issues or discussions about positioning
find . -name "*.md" -exec grep -l "position\|overlay\|constrain" {} \;Repository: minikin/popover
Length of output: 73
🏁 Script executed:
# Let's check PopoverRenderShiftedBox to see if it has similar logic
cat lib/src/popover_render_shifted_box.dart | head -100Repository: minikin/popover
Length of output: 2499
Use the render box bounds for clamping instead of the global screen size.
The new center-first positioning logic is better, but _horizontalOffset() and _verticalOffset() still clamp against PopoverUtils.physicalSize, which is the raw device screen size. When PopoverPositionRenderObject is constrained to bounds smaller than the full screen (e.g., within dialogs or other constrained layouts), this can position the popover outside its actual render bounds. Since performLayout() sets this.size before calculateOffset() runs, the clamping should use the render object's actual bounds instead of a global screen size.
🛠️ Proposed fix
- double _horizontalOffset(Size size) {
+ double _horizontalOffset(Size childSize) {
var offset = 0.0;
const horizontalMargin = 8.0;
+ final availableWidth = this.size.width;
final centeredOffset =
- attachRect.left + attachRect.width / 2 - size.width / 2;
+ attachRect.left + attachRect.width / 2 - childSize.width / 2;
if (centeredOffset >= horizontalMargin &&
- centeredOffset + size.width <=
- PopoverUtils.physicalSize.width - horizontalMargin) {
+ centeredOffset + childSize.width <= availableWidth - horizontalMargin) {
offset = centeredOffset;
} else if (centeredOffset < horizontalMargin) {
offset = horizontalMargin;
} else {
- offset = PopoverUtils.physicalSize.width - horizontalMargin - size.width;
+ offset = availableWidth - horizontalMargin - childSize.width;
}
return offset;
}
- double _verticalOffset(Size size) {
+ double _verticalOffset(Size childSize) {
var offset = 0.0;
const verticalMargin = 8.0;
+ final availableHeight = this.size.height;
final centeredOffset =
- attachRect.top + attachRect.height / 2 - size.height / 2;
+ attachRect.top + attachRect.height / 2 - childSize.height / 2;
if (centeredOffset >= verticalMargin &&
- centeredOffset + size.height <=
- PopoverUtils.physicalSize.height - verticalMargin) {
+ centeredOffset + childSize.height <=
+ availableHeight - verticalMargin) {
offset = centeredOffset;
} else if (centeredOffset < verticalMargin) {
offset = verticalMargin;
} else {
- offset = PopoverUtils.physicalSize.height - verticalMargin - size.height;
+ offset = availableHeight - verticalMargin - childSize.height;
}
return offset;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/popover_position_render_object.dart` around lines 95 - 129, The
clamping logic in _horizontalOffset(Size size) and _verticalOffset(Size size)
currently uses PopoverUtils.physicalSize (device screen) but should use the
render object's actual bounds set in performLayout; replace references to
PopoverUtils.physicalSize.width/height with this.size.width/height (the render
object's size) when checking and computing the max offset and fallback positions
so the popover is clamped to the render object's bounds rather than the full
screen.
|
@minikin May I ask where all your issues went? Issue #103 did in fact already contain a minimum working example. Anyway, I have created a new example demonstrating the issue and have updated the description of this pull request. I have also removed all comments generated by the LLM and fixed the indentation issues. And I have now given maintainers of this repo write access to the pull request. coderabbitai seems to be reporting issues with the commits. So you may want to review that the changes do not introduce new problems. Let me know if there's anything else I can do. |
|
Thanks for the update and for taking the time to address the feedback. I appreciate it. Regarding the issues, I intentionally chose to disable them. After many years as an open source maintainer and contributor, I’ve noticed that almost all issues, about 99%, are requests for me to fix things, not actual contributions. That approach just isn’t sustainable for a project I work on in my free time. I prefer to keep things simple and focus on pull requests. PS: I've updated README, and CONTRIBUTING |
minikin
changed the title
|
@userFortyTwo, please, fix CI. |
|
@minikin Might be fixed now (pipeline awaiting approval) |
2 participants
This pull request fixes issue #103 ("[Bug]: Popover is not centered above widget", https://github.com/minikin/popover/issues/103), which, like all other issues, seems to have been deleted for some reason.
Example
The following example can be used to reproduce the issue:
Without this pull request, the result will look like so:
With the pull request in place, this will be the result:
Summary by CodeRabbit