feat: add presets offsets#9
Conversation
add presets offsets add using of EnumDisplaySettingsW to properly get screen size fix validator of offsets
WalkthroughVersion update to 1.3.2 with refactored UI construction into helper functions and expanded display device settings via DEVMODE and EnumDisplaySettingsW for monitor dimension retrieval. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
assets/fullscreen.svgis excluded by!**/*.svgassets/halfleft.svgis excluded by!**/*.svgassets/halfright.svgis excluded by!**/*.svggo.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
FyneApp.toml(1 hunks)gui_app.go(1 hunks)gui_appsetting.go(6 hunks)winapi.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
gui_appsetting.go (5)
main.go (1)
Window(19-23)settings.go (2)
AppSetting(57-71)Settings(86-94)ui/select.go (2)
Select(14-18)NewSelect(50-69)winapi.go (1)
Monitor(96-103)rx/observable.go (1)
Subscription(12-14)
🔇 Additional comments (10)
FyneApp.toml (1)
7-8: LGTM! Version bump aligns with PR objectives.The version and build number updates correctly reflect the new preset offsets feature.
gui_app.go (1)
45-53: LGTM! Well-designed validator for optional numeric input.The function correctly allows empty strings (defaulting to zero) while validating non-empty input as integers. This is appropriate for offset fields where zero is a valid default.
winapi.go (3)
110-110: LGTM! Enhanced monitor information display.The addition of resolution information to the String() method improves usability by showing monitor dimensions alongside the display number.
114-138: LGTM! Proper Windows API integration.The DEVMODE structure and EnumDisplaySettingsW proc are correctly defined for retrieving monitor display settings.
140-183: LGTM! Robust monitor detection with proper fallback.The refactored
getMonitors()function correctly:
- Uses EnumDisplaySettingsW to get actual display resolution
- Falls back to RECT-based calculation when needed
- Populates monitor position (left, top) for multi-monitor support
This addresses the PR objective of fixing screen size detection.
gui_appsetting.go (5)
79-93: LGTM! Clean extraction of validation logic.The refactoring of window validation into
isValidWindowForSelectionimproves code organization and reusability. The validation logic correctly checks for windows with borders/captions.
95-111: LGTM! Excellent refactoring with modular helper functions.The extraction of UI construction into dedicated helper functions significantly improves:
- Code organization and readability
- Maintainability and testability
- Separation of concerns
The validators are correctly applied:
offsetIntValidatorfor optional offset fields (allows empty = 0)intValidatorfor required size fieldsAlso applies to: 128-155, 172-258
260-284: LGTM! Thread-safe window list subscription.The function correctly:
- Only subscribes for new app settings
- Uses
fyne.Doto ensure UI updates on the main thread- Validates and clears selection when windows are removed
324-379: LGTM! Well-structured dialog creation with proper cleanup.The refactored
makeAppSettingWindowfunction:
- Uses mutex correctly for thread-safe window list access
- Properly unsubscribes from window updates on dialog close
- Delegates UI construction to modular helper functions
- Maintains clean separation of concerns
Note: The critical issues with preset button calculations (flagged in previous comments) should be addressed before merging.
315-322: Embedded asset files verified—no action required.All three SVG files referenced by the
//go:embeddirectives are present in the repository:
- ✓
assets/fullscreen.svg- ✓
assets/halfleft.svg- ✓
assets/halfright.svgNo build failures will occur due to missing embedded assets.
| func getDefaultMonitorIndex(settings *Settings, appSetting AppSetting, isNew bool) int { | ||
| if !isNew { | ||
| return appSetting.Monitor - 1 | ||
| } | ||
|
|
||
| monitorIdx := settings.Defaults.Monitor - 1 | ||
| if monitorIdx < 0 { | ||
| monitorIdx = slices.IndexFunc(monitors, func(m Monitor) bool { | ||
| return m.isPrimary | ||
| }) | ||
| } | ||
|
|
||
| return monitorIdx | ||
| } |
There was a problem hiding this comment.
Critical: Potential index out of bounds panic.
The function doesn't validate the monitor index before returning it:
- If
appSetting.Monitororsettings.Defaults.Monitoris 0 or exceeds the number of monitors, the returned index will be invalid - If no primary monitor is found,
slices.IndexFuncreturns -1 - Line 337 uses this index directly:
selectedMonitor := monitors[monitorIdx]which will panic
Apply this defensive fix:
func getDefaultMonitorIndex(settings *Settings, appSetting AppSetting, isNew bool) int {
+ if len(monitors) == 0 {
+ return 0
+ }
+
if !isNew {
- return appSetting.Monitor - 1
+ idx := appSetting.Monitor - 1
+ if idx >= 0 && idx < len(monitors) {
+ return idx
+ }
}
monitorIdx := settings.Defaults.Monitor - 1
- if monitorIdx < 0 {
+ if monitorIdx < 0 || monitorIdx >= len(monitors) {
monitorIdx = slices.IndexFunc(monitors, func(m Monitor) bool {
return m.isPrimary
})
+ if monitorIdx == -1 {
+ monitorIdx = 0
+ }
}
return monitorIdx
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getDefaultMonitorIndex(settings *Settings, appSetting AppSetting, isNew bool) int { | |
| if !isNew { | |
| return appSetting.Monitor - 1 | |
| } | |
| monitorIdx := settings.Defaults.Monitor - 1 | |
| if monitorIdx < 0 { | |
| monitorIdx = slices.IndexFunc(monitors, func(m Monitor) bool { | |
| return m.isPrimary | |
| }) | |
| } | |
| return monitorIdx | |
| } | |
| func getDefaultMonitorIndex(settings *Settings, appSetting AppSetting, isNew bool) int { | |
| if len(monitors) == 0 { | |
| return 0 | |
| } | |
| if !isNew { | |
| idx := appSetting.Monitor - 1 | |
| if idx >= 0 && idx < len(monitors) { | |
| return idx | |
| } | |
| } | |
| monitorIdx := settings.Defaults.Monitor - 1 | |
| if monitorIdx < 0 || monitorIdx >= len(monitors) { | |
| monitorIdx = slices.IndexFunc(monitors, func(m Monitor) bool { | |
| return m.isPrimary | |
| }) | |
| if monitorIdx == -1 { | |
| monitorIdx = 0 | |
| } | |
| } | |
| return monitorIdx | |
| } |
| func createSizeButton(monitor Monitor, xDivider int32, xOffset int32, label string, icon fyne.Resource) *widget.Button { | ||
| monitorWidth := monitor.width / xDivider | ||
| offsetWidth := int32(0) | ||
| if xOffset == 1 { | ||
| offsetWidth = monitorWidth | ||
| } | ||
| setOnFocusChanged(xOffsetText, func(focused bool) { | ||
| if focused { | ||
| xOffsetText.DoubleTapped(&fyne.PointEvent{}) | ||
| } | ||
|
|
||
| return widget.NewButtonWithIcon(label, icon, func() { | ||
| widthText.SetText(strconv.Itoa(int(monitorWidth))) | ||
| heightText.SetText(strconv.Itoa(int(monitor.height))) | ||
| xOffsetText.SetText(strconv.Itoa(int(offsetWidth))) | ||
| yOffsetText.SetText("0") | ||
| }) | ||
| xOffsetText.SetPlaceHolder("0") | ||
| if isNew { | ||
| xOffsetText.SetText(strconv.Itoa(int(settings.Defaults.OffsetX))) | ||
| } else { | ||
| xOffsetText.SetText(strconv.Itoa(int(appSetting.OffsetX))) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Preset offsets don't account for monitor position in multi-monitor setups.
The preset buttons calculate offsets relative to (0,0) but don't include the monitor's actual position (monitor.left, monitor.top). This will cause incorrect positioning on secondary monitors.
For example, with a secondary monitor at position (1920, 0):
- "Full" preset sets xOffset to 0, but should set it to 1920
- "Half Right" preset sets xOffset to monitor.width/2, but should set it to 1920 + monitor.width/2
Apply this fix:
func createSizeButton(monitor Monitor, xDivider int32, xOffset int32, label string, icon fyne.Resource) *widget.Button {
monitorWidth := monitor.width / xDivider
- offsetWidth := int32(0)
+ offsetWidth := monitor.left
if xOffset == 1 {
- offsetWidth = monitorWidth
+ offsetWidth = monitor.left + monitorWidth
}
return widget.NewButtonWithIcon(label, icon, func() {
widthText.SetText(strconv.Itoa(int(monitorWidth)))
heightText.SetText(strconv.Itoa(int(monitor.height)))
xOffsetText.SetText(strconv.Itoa(int(offsetWidth)))
- yOffsetText.SetText("0")
+ yOffsetText.SetText(strconv.Itoa(int(monitor.top)))
})
}🤖 Prompt for AI Agents
In gui_appsetting.go around lines 157 to 170, the preset button handler sets x/y
offsets relative to (0,0) and ignores monitor.left/monitor.top; update the
offset calculation so xOffsetText gets the monitor-relative value and
yOffsetText gets monitor.top. Concretely, compute offsetWidth including
monitor.left (e.g. if xOffset==1 set offsetWidth = monitor.left + monitorWidth,
otherwise offsetWidth = monitor.left + 0) and use strconv.Itoa(int(offsetWidth))
for xOffsetText, and set yOffsetText to strconv.Itoa(int(monitor.top)) instead
of "0".
| leftIcon := fyne.NewStaticResource("right.svg", halfLeftSVGBytes) | ||
| rightIcon := fyne.NewStaticResource("right.svg", halfRightSVGBytes) | ||
| fullIcon := fyne.NewStaticResource("full.svg", fullscreenSVGBytes) |
There was a problem hiding this comment.
Minor: Copy-paste error in resource names.
Both leftIcon and rightIcon are named "right.svg", which is incorrect. While this may not cause functional issues, it harms code clarity and could cause confusion during debugging.
Apply this fix:
- leftIcon := fyne.NewStaticResource("right.svg", halfLeftSVGBytes)
- rightIcon := fyne.NewStaticResource("right.svg", halfRightSVGBytes)
+ leftIcon := fyne.NewStaticResource("halfleft.svg", halfLeftSVGBytes)
+ rightIcon := fyne.NewStaticResource("halfright.svg", halfRightSVGBytes)
fullIcon := fyne.NewStaticResource("full.svg", fullscreenSVGBytes)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| leftIcon := fyne.NewStaticResource("right.svg", halfLeftSVGBytes) | |
| rightIcon := fyne.NewStaticResource("right.svg", halfRightSVGBytes) | |
| fullIcon := fyne.NewStaticResource("full.svg", fullscreenSVGBytes) | |
| leftIcon := fyne.NewStaticResource("halfleft.svg", halfLeftSVGBytes) | |
| rightIcon := fyne.NewStaticResource("halfright.svg", halfRightSVGBytes) | |
| fullIcon := fyne.NewStaticResource("full.svg", fullscreenSVGBytes) |
🤖 Prompt for AI Agents
In gui_appsetting.go around lines 332 to 334, the StaticResource names are
incorrect: both leftIcon and rightIcon are given "right.svg"; rename leftIcon to
use "left.svg" (and ensure rightIcon remains "right.svg") while keeping fullIcon
as "full.svg" so the resource names accurately match their contents and improve
code clarity.
| halfLeftBtn = createSizeButton(selectedMonitor, 2, 0, "Half Left", leftIcon) | ||
| halfRightBtn = createSizeButton(selectedMonitor, 2, 1, "Half Right", rightIcon) | ||
| fullBtn = createSizeButton(selectedMonitor, 1, 0, "Full", fullIcon) |
There was a problem hiding this comment.
Major: Preset buttons don't update when display selection changes.
The preset buttons (Half Left, Half Right, Full) are created once using the initial selectedMonitor. When the user changes the display selection via displaySelect, the buttons continue to use the original monitor's dimensions rather than updating to the newly selected monitor.
Consider recreating the buttons in the displaySelect onChange handler:
displaySelect = ui.NewSelect(monitors, func(selected Monitor) {
appSetting.Monitor = selected.number
// Recreate preset buttons with new monitor
halfLeftBtn = createSizeButton(selected, 2, 0, "Half Left", leftIcon)
halfRightBtn = createSizeButton(selected, 2, 1, "Half Right", rightIcon)
fullBtn = createSizeButton(selected, 1, 0, "Full", fullIcon)
// Refresh the UI...
setConfirmButtonState(isNew)
})Note: This would require restructuring the code to allow button recreation and UI refresh.
🤖 Prompt for AI Agents
In gui_appsetting.go around lines 339 to 341, the preset buttons are created
once with the initial selectedMonitor so they do not reflect changes when
displaySelect changes; update the displaySelect onChange handler to recreate the
halfLeftBtn, halfRightBtn and fullBtn using the newly selected monitor (call
createSizeButton with the new selected monitor and same params), replace the old
button references, and refresh the UI layout (re-add or re-render the button
container) so the new buttons use the updated monitor dimensions; ensure
setConfirmButtonState(isNew) is still called after recreation.
|
I somehow completely missed this PR and just now noticed it when I was adding a simple thing. I'll take a look now but there are now conflicts because of my changes. |
adamk33n3r
left a comment
adamk33n3r
left a comment
There was a problem hiding this comment.
There seem to be a few issues coderabbit pointed out, specifically the one where changing the display doesn't update what the preset buttons do so this basically makes it non-functional.
There was a problem hiding this comment.
This looks to be a copy of halfright instead of mirrored
There was a problem hiding this comment.
Did you make these images yourself? Just want to make sure of attribution. Also, they aren't vertically centered. Is that intentional?
There was a problem hiding this comment.
Additionally, these should go into res and use the bundler
| ID = "com.adamk33n3r.goborderless" | ||
| Version = "1.3.1" | ||
| Build = 79 | ||
| Version = "1.3.2" |
There was a problem hiding this comment.
I wouldn't call this a patch version.
| github.com/jeandeaual/go-locale v0.0.0-20250421151639-a9d6ed1b3d45 h1:vFdvrlsVU+p/KFBWTq0lTG4fvWvG88sawGlCzM+RUEU= | ||
| github.com/jeandeaual/go-locale v0.0.0-20250421151639-a9d6ed1b3d45/go.mod h1:ZDXo8KHryOWSIqnsb/CiDq7hQUYryCgdVnxbj8tDG7o= | ||
| github.com/joho/godotenv v1.5.1 h1:7eLL/+HRGLY0ldzfGMeQkb7vMd0as4CfYvUVzLqw0N0= | ||
| github.com/joho/godotenv v1.5.1/go.mod h1:f4LDr5Voq0i2e/R5DDNOoa2zzDfwtkZa6DnEwAbqwq4= |
| } | ||
|
|
||
| func offsetIntValidator(s string) error { | ||
| if s == "" { |
There was a problem hiding this comment.
why is empty string valid for offsets?
| if !isNew { | ||
| confirmButton.SetText("Save") | ||
| } | ||
| func isValidWindowForSelection(window Window) bool { |
There was a problem hiding this comment.
what's the point of putting this into a function?
| }) | ||
| } | ||
|
|
||
| func createPresetsContent(settings *Settings, appSetting *AppSetting, isNew bool, cancelButton, confirmBtn *widget.Button) *fyne.Container { |
There was a problem hiding this comment.
weird having cancelButton and confirmBtn
| var devMode DEVMODE | ||
| devMode.DmSize = uint16(unsafe.Sizeof(devMode)) | ||
|
|
||
| ret, _, _ := procEnumDisplaySettingsW.Call( |
There was a problem hiding this comment.
Can you explain to me why this is here? Why isn't the Right - Left sufficient?
| } | ||
|
|
||
| var ( | ||
| procEnumDisplaySettingsW = user32.NewProc("EnumDisplaySettingsW") |
There was a problem hiding this comment.
This should be placed up with the others
There was a problem hiding this comment.
I'm not really fond of all the extra functions being created unnecessarily. The one for the size and offset entries is perhaps warranted, but the other ones are introducing so much clutter. And it's not important to your PR, it's just there to be there. I also noticed a lot of unused parameters in the functions. Like why are settings and appSetting being passed into the entry functions when they aren't being used. I'm hesitant to point a finger at AI.
2 participants
Summary of Changes
1. New Feature: Presets for Screen Offsets
2. Fix: Reliable Screen Size Detection
EnumDisplaySettingsWto reliably retrieve the proper current screen resolution and boundaries.Summary by CodeRabbit
Chores
Improvements