fix: stabilize X11 cursor handling and HiDPI window layout#1669
Open
Juanzitooh wants to merge 4 commits intoopentibiabr:mainfrom
Open
fix: stabilize X11 cursor handling and HiDPI window layout#1669Juanzitooh wants to merge 4 commits intoopentibiabr:mainfrom
Juanzitooh wants to merge 4 commits intoopentibiabr:mainfrom
Conversation
Implement robust Linux X11 display density resolution with env/Xft/physical fallbacks. Fix X11 cursor loading by correcting bitmap stride, improving RGBA-to-mono conversion, and adding a safe system-cursor fallback. Implement setSystemCursor on X11 with shape mapping and cursor caching. Scope startup window size/position scaling to X11 only to preserve behavior on other platforms.
📝 WalkthroughWalkthroughAdds X11-specific display-density resolution and window metrics scaling/persistence in startup; implements dynamic density detection, per-shape system cursor creation/caching, cursor restoration, and improved cursor bitmap/mask logic in the X11 window implementation. Changes
Sequence DiagramsequenceDiagram
participant Lua as Startup (Lua)
participant Win as X11Window (C++)
participant Resolver as DensityResolver
participant X11 as X11 Display
participant CursorMgr as CursorManager/Cache
Lua->>Win: init()
activate Win
Win->>Resolver: resolveDensity()
activate Resolver
Resolver->>X11: query Env / Xft / physical DPI
X11-->>Resolver: DPI / env values
Resolver-->>Win: computed density
deactivate Resolver
Win->>Win: scale & clamp saved/default window size and pos (X11 path)
Win-->>Lua: init complete
Lua->>Win: setSystemCursor(name)
activate Win
Win->>CursorMgr: lookup m_systemCursors
alt cached
CursorMgr-->>Win: return cached cursor
else not cached
CursorMgr->>X11: create cursor (shape/bitmap/mask)
X11-->>CursorMgr: cursor handle or failure
CursorMgr-->>Win: store & return
end
Win->>X11: apply cursor via restoreMouseCursorNow()
deactivate Win
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/startup/startup.lua`:
- Around line 25-31: Persisted X11 geometry keys were changed from raw X11
pixels to density-normalized values, so on upgrade stored values must be
migrated to avoid being multiplied by density; implement a one-time migration or
versioning when reading 'window-size' and 'window-pos' in startup.lua: detect an
un-migrated X11 value (use isX11 and a new stored flag/key like
'window-geometry-version' or compare against screen bounds), and if legacy,
divide the stored width/height/x/y by density and write back the normalized
values and set the migration flag/version; apply the same migration logic for
the other block referenced (lines ~90-101) and ensure
getUnmaximizedSize()/getUnmaximizedPos() continue to return raw ConfigureNotify
values while storage uses the normalized values after migration.
- Around line 41-54: The code multiplies pos by density even when pos is the
newly computed defaultPos (causing double-scaling); change the logic so the
density scaling block (the isX11 and density ~= 1 branch that adjusts
pos.x/pos.y) only runs when the loaded g_settings.getPoint('window-pos',
defaultPos) returned a stored setting (i.e. pos is not the defaultPos) — one
simple fix is to compare pos to defaultPos and skip the scaling and clamping
when they are equal; update the code that references defaultPos, pos, isX11 and
density so default centered positions are not multiplied twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3eb60e62-e89d-4562-b596-f95509a4287e
📒 Files selected for processing (3)
modules/startup/startup.luasrc/framework/platform/x11window.cppsrc/framework/platform/x11window.h
Comment on lines
25
to
+31
| size = g_settings.getSize('window-size', size) | ||
| if isX11 and density ~= 1 then | ||
| size = { | ||
| width = math.floor((size.width * density) + 0.5), | ||
| height = math.floor((size.height * density) + 0.5) | ||
| } | ||
| end |
There was a problem hiding this comment.
Persisted X11 geometry needs a migration path.
These changes reuse window-size and window-pos but switch them from raw X11 pixels to density-normalized values. getUnmaximizedSize() / getUnmaximizedPos() still reflect raw ConfigureNotify geometry, so existing HiDPI X11 settings will be multiplied by density on the first launch after upgrade and can reopen clamped or off-screen until shutdown rewrites the keys. Please version these keys or add a one-time migration.
Also applies to: 90-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/startup/startup.lua` around lines 25 - 31, Persisted X11 geometry
keys were changed from raw X11 pixels to density-normalized values, so on
upgrade stored values must be migrated to avoid being multiplied by density;
implement a one-time migration or versioning when reading 'window-size' and
'window-pos' in startup.lua: detect an un-migrated X11 value (use isX11 and a
new stored flag/key like 'window-geometry-version' or compare against screen
bounds), and if legacy, divide the stored width/height/x/y by density and write
back the normalized values and set the migration flag/version; apply the same
migration logic for the other block referenced (lines ~90-101) and ensure
getUnmaximizedSize()/getUnmaximizedPos() continue to return raw ConfigureNotify
values while storage uses the normalized values after migration.
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 `@modules/startup/startup.lua`:
- Around line 74-78: The code currently skips reading stored X11 window position
because of the "if not isX11" guard around g_settings.getPoint; remove or invert
that guard so pos = g_settings.getPoint('window-pos', defaultPos) runs when
hasSavedWindowPos is true (or unconditionally using hasSavedWindowPos) so X11
restores/migration (the legacy scaling path in the block around the code labeled
by 95-99) can operate on the actual saved coordinates; also stop deleting
'window-pos' unconditionally at shutdown (the deletion in the shutdown block
around the 161-167 area) — only remove the key when truly obsolete or when
migrating its values, otherwise preserve it for X11 restore and legacy scaling
code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91e306b5-6761-41fc-bbb9-56e612d44776
📒 Files selected for processing (1)
modules/startup/startup.lua
Comment on lines
+74
to
+78
| local hasSavedWindowPos = g_settings.exists('window-pos') | ||
| local pos = defaultPos | ||
| if not isX11 then | ||
| pos = g_settings.getPoint('window-pos', defaultPos) | ||
| end |
There was a problem hiding this comment.
Re-enable X11 window-pos persistence/migration.
Line 76 skips reading saved X11 position, and Line 162 deletes it at shutdown. This prevents position restore on X11 and makes the legacy scaling path at Lines 95-99 effectively non-functional.
💡 Proposed fix
@@
- local hasSavedWindowPos = g_settings.exists('window-pos')
- local pos = defaultPos
- if not isX11 then
- pos = g_settings.getPoint('window-pos', defaultPos)
- end
+ local hasSavedWindowPos = g_settings.exists('window-pos')
+ local pos = g_settings.getPoint('window-pos', defaultPos)
@@
- if isX11 then
- g_settings.remove('window-pos')
- g_settings.set('window-metrics-space', 'physical-v1')
- g_logger.info('[X11WindowMetrics][terminate] window-pos persistence disabled on X11; window-metrics-space=physical-v1')
- else
- g_settings.set('window-pos', windowPos)
- g_settings.remove('window-metrics-space')
- end
+ g_settings.set('window-pos', windowPos)
+ if isX11 then
+ g_settings.set('window-metrics-space', 'physical-v1')
+ g_logger.info('[X11WindowMetrics][terminate] window-pos persisted; window-metrics-space=physical-v1')
+ else
+ g_settings.remove('window-metrics-space')
+ endAlso applies to: 95-99, 161-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/startup/startup.lua` around lines 74 - 78, The code currently skips
reading stored X11 window position because of the "if not isX11" guard around
g_settings.getPoint; remove or invert that guard so pos =
g_settings.getPoint('window-pos', defaultPos) runs when hasSavedWindowPos is
true (or unconditionally using hasSavedWindowPos) so X11 restores/migration (the
legacy scaling path in the block around the code labeled by 95-99) can operate
on the actual saved coordinates; also stop deleting 'window-pos' unconditionally
at shutdown (the deletion in the shutdown block around the 161-167 area) — only
remove the key when truly obsolete or when migrating its values, otherwise
preserve it for X11 restore and legacy scaling code paths.
Juanzitooh
force-pushed
the
fix/linux-window-mouse
branch
from
12fb247 to
b838946
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/framework/platform/x11window.cpp (2)
1111-1202: Improved cursor bitmap generation with proper stride handling.The refactored implementation correctly handles non-8-aligned widths with
(width + 7) / 8forbytesPerRow, and the alpha/luma thresholding provides better RGBA→monochrome conversion than the previous approach. The fallback toXC_left_ptrfor empty/invalid cursors is a solid defensive measure.Minor cleanup: Several counters are computed but never used:
Remove unused counter variables
- int opaqueWhitePixels = 0; - int opaqueBlackPixels = 0; - int transparentPixels = 0; - int translucentClippedPixels = 0; + int opaquePixelCount = 0; for (int i = 0; i < numbits; ++i) { // ... if (a <= kCursorAlphaThreshold) { - ++transparentPixels; - if (a > 0) - ++translucentClippedPixels; continue; } const int luma = (r * 299 + g * 587 + b * 114) / 1000; if (luma >= kCursorLumaThreshold) { setCursorBitmapBit(maskBits, bytesPerRow, x, y); - ++opaqueWhitePixels; + ++opaquePixelCount; } else { setCursorBitmapBit(mapBits, bytesPerRow, x, y); setCursorBitmapBit(maskBits, bytesPerRow, x, y); - ++opaqueBlackPixels; + ++opaquePixelCount; } } // ... - const int maskPixels = opaqueWhitePixels + opaqueBlackPixels; - const int mapPixels = opaqueBlackPixels; // ... - if (maskPixels == 0) { + if (opaquePixelCount == 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/x11window.cpp` around lines 1111 - 1202, The function internalLoadMouseCursor declares and updates unused counters (transparentPixels, translucentClippedPixels) and computes mapPixels which is never used; remove the declarations and any increments for transparentPixels and translucentClippedPixels, and delete the unused mapPixels computation while keeping opaqueWhitePixels and opaqueBlackPixels (used to compute maskPixels/map logic) and ensure no remaining references to the removed symbols in internalLoadMouseCursor, setCursorBitmapBit, or related logic.
366-368: UnuseddensitySourcevariable.The
densitySourceis populated byresolveDisplayDensitybut never used afterward. Consider removing it or logging it for debugging purposes.Option 1: Remove unused variable
void X11Window::init() { internalOpenDisplay(); - DensitySource densitySource = DensitySource::Fallback; - setDisplayDensity(resolveDisplayDensity(m_display, m_screen, densitySource)); + setDisplayDensity(resolveDisplayDensity(m_display, m_screen));This would also require updating
resolveDisplayDensityto not take thesourceparameter.Option 2: Log the source for debugging
DensitySource densitySource = DensitySource::Fallback; - setDisplayDensity(resolveDisplayDensity(m_display, m_screen, densitySource)); + const float density = resolveDisplayDensity(m_display, m_screen, densitySource); + g_logger.debug("X11 display density: {} (source: {})", density, static_cast<int>(densitySource)); + setDisplayDensity(density);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/x11window.cpp` around lines 366 - 368, The local variable densitySource is assigned by resolveDisplayDensity(m_display, m_screen, densitySource) but never used; either remove densitySource and call setDisplayDensity(resolveDisplayDensity(m_display, m_screen)) and update resolveDisplayDensity signature and all callers to drop the out/source parameter, or keep it and use it (e.g., log the value) after the call to aid debugging; update references to the function/method resolveDisplayDensity and the local symbol densitySource accordingly, and ensure setDisplayDensity is still passed the returned value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/framework/platform/x11window.cpp`:
- Around line 1111-1202: The function internalLoadMouseCursor declares and
updates unused counters (transparentPixels, translucentClippedPixels) and
computes mapPixels which is never used; remove the declarations and any
increments for transparentPixels and translucentClippedPixels, and delete the
unused mapPixels computation while keeping opaqueWhitePixels and
opaqueBlackPixels (used to compute maskPixels/map logic) and ensure no remaining
references to the removed symbols in internalLoadMouseCursor,
setCursorBitmapBit, or related logic.
- Around line 366-368: The local variable densitySource is assigned by
resolveDisplayDensity(m_display, m_screen, densitySource) but never used; either
remove densitySource and call setDisplayDensity(resolveDisplayDensity(m_display,
m_screen)) and update resolveDisplayDensity signature and all callers to drop
the out/source parameter, or keep it and use it (e.g., log the value) after the
call to aid debugging; update references to the function/method
resolveDisplayDensity and the local symbol densitySource accordingly, and ensure
setDisplayDensity is still passed the returned value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41372bb1-e4ff-4d8b-a90b-d86b01fab0e1
📒 Files selected for processing (2)
modules/startup/startup.luasrc/framework/platform/x11window.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/startup/startup.lua
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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
2 participants
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.
This PR fixes Linux/X11 window scaling and cursor instability issues that were causing incorrect window sizing/positioning and intermittent invisible or incorrect mouse cursors.
Summary of changes:
OTCLIENT_DPI_SCALEoverrideXft.dpi1.0setSystemCursor(...)for X11:Motivation/context:
setSystemCursorhad no X11 implementation, so native cursor mode behavior was inconsistent.Dependencies:
Behavior
Actual
setSystemCursor(...)calls were not effective on X11.Expected
setSystemCursor(...)works consistently on X11.Fixes
(issue)
Type of change
How Has This Been Tested
cmake --build build/linux-release -j4cross,text,handtransitionsTest Configuration:
ae1543664Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores