fix: Chromium effect reapply after lock/unlock and minimize, misc fixes#144
fix: Chromium effect reapply after lock/unlock and minimize, misc fixes#144GrzegorzKozub wants to merge 10 commits into
Conversation
00148c2 to
98559d1
Compare
|
Hi @flexagoon. I've been testing this for a couple of days now and find the fix working without any visible negative visual or usability impact. There's also no issues in journal. Take a look, please 😃 |
| deferredRefreshId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, 250, () => { | ||
| const focusedWin = global.display.get_focus_window(); | ||
| const chromiumWindows = global | ||
| .get_window_actors() | ||
| .map(a => a.metaWindow) | ||
| .filter( | ||
| (win): win is Meta.Window => | ||
| win != null && | ||
| win !== focusedWin && | ||
| isChromiumWindow(win), | ||
| ); | ||
|
|
||
| if (chromiumWindows.length > 0) { | ||
| const timestamp = global.get_current_time(); | ||
| for (const [i, win] of chromiumWindows.entries()) { | ||
| GLib.timeout_add(GLib.PRIORITY_DEFAULT, i * 100, () => { | ||
| win.focus(timestamp); | ||
| return GLib.SOURCE_REMOVE; | ||
| }); | ||
| } | ||
|
|
||
| // Restore focus to the originally focused window. | ||
| GLib.timeout_add( | ||
| GLib.PRIORITY_DEFAULT, | ||
| chromiumWindows.length * 100, | ||
| () => { | ||
| focusedWin?.focus(global.get_current_time()); | ||
| return GLib.SOURCE_REMOVE; | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| deferredRefreshId = 0; | ||
| return GLib.SOURCE_REMOVE; |
There was a problem hiding this comment.
I'm not sure this is the best way to redraw a window
There is an actor.queue_redraw() function, could you check if that works instead? (There's also .queue_relayout(), not sure which one of those would be correct here)
There was a problem hiding this comment.
So this suggests actor.queue_redraw() or queue_relayout() instead of focus cycling. I already tried actor.queue_redraw() earlier and it didn't work for background windows. The compositor skips repainting GLSL effects for occluded actors even with queue_redraw(). Only focusing the window forces the compositor to actually process the repaint.
|
I also added some performance improvements now @flexagoon. Details in the description. Been using this version while keeping a look at the journal and it seems fine. |
|
Hi @flexagoon 😃 I rebased and added AI disclosure. I've been running the version with my change for a month now on 3 different machines and seen no issues. Can we take a look into merging this? |
|
@GrzegorzKozub yeah, I'll review it when I get time, sorry, Im somewhat busy with uni rn |
|
I tested this PR on my Gnome 50 + Fedora system, does not fix the minimize glitch mentioned in #124 . It seems to be a glitch from upstream, since when I disabled the extension, this glitch is still there just without the border issues. |
c5db71f to
ea996f7
Compare
3e13f1f to
01c3821
Compare
I was able to fix this partially, as described in the first paragraph of this PR description @yuelinxin |
b7df411 to
afb3d14
Compare
GNOME Shell disables and re-enables extensions during screen lock/unlock. When the extension re-enables, Chromium-based browsers (Brave, Chrome, Edge) may render stale surfaces for unfocused windows. The compositor skips repainting GLSL effects for these windows, resulting in scrambled borders and a doubled frame. Fix by briefly focusing each unfocused Chromium window after re-enable, which forces the compositor to repaint the GLSL effect. Also promote onFocusChanged from refreshShadow to refreshRoundedCorners so that any subsequent focus change recomputes shader bounds, not just the shadow. Fixes flexagoon#124
vfunc_allocate on the overview shadow clone can be called before the window has a valid frame rect, causing division by zero which produces NaN values in the allocation box and triggers Clutter assertion failures: Can't update stage views actor Shadow Actor (Overview) is on because it needs an allocation. clutter_actor_set_allocation_internal: assertion '!isnan (box->x1) && !isnan (box->x2) && !isnan (box->y1) && !isnan (box->y2)' failed
getPref() reads go through D-Bus and GLib Variant unpacking, which is expensive when done in hot paths that run per-frame (shader uniform updates) or per-event (focus/resize). This commit addresses the most frequent offenders. - Cache unpacked pref values in settings.ts. The cache is invalidated automatically by connecting to the GSettings 'changed' signal, so values stay correct across the preferences UI. This single change eliminates repeated unpacks for debug-mode (called from logDebug on every hot-path statement), blacklist/whitelist (read on every shouldEnableEffect call), keep-shadow-for-maximized-fullscreen, tweak-kitty-terminal, and the focused/unfocused shadow dictionaries. - Simplify updateShadowActorStyle to read global-rounded-corner-settings once and derive borderRadius/padding/smoothing from it, instead of four separate getPref calls (three via default parameters, one inside the function body). Caller-provided overrides are still honored. - Reorder the kitty check in computeBounds to compare wm_class first. The pref read is cheap now but the early wm_class comparison avoids it entirely for every non-kitty window, which is almost all of them.
getAppType reads /proc/<pid>/maps to detect LibHandy/LibAdwaita windows. For processes owned by another user (flatpak sandboxes, root-owned apps) the file is unreadable, producing a benign PERMISSION_DENIED that was logged with a full stack trace on every such window. Treat PERMISSION_DENIED and NOT_FOUND as expected and log them at debug level; keep logError for everything else.
…crash On mutter 50.2 (Wayland-only), actor.metaWindow can be null during window actor lifecycle transitions. Previously, applyEffectTo connected notify::size and size-changed before reaching the actor.metaWindow signal connections. When metaWindow was null, the TypeError aborted the function mid-way, leaving size signals connected but the effect never added. On the next notify::size (fired during Clutter layout), onAddEffect was called while the actor was mid-paint, corrupting the effect's actor pointer and causing clutter_actor_node_new(NULL) → SIGABRT. Fix by guarding actor.metaWindow before connecting any signals in applyEffectTo so the function either completes fully or returns early with no signals connected. Add matching null guards to onAddEffect and refreshRoundedCorners. Also guard actor from get_compositor_private() in the window-created handler and re-fetch it fresh in the wm-class callback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
On mutter 50.2, Clutter can emit notify::size and size-changed during a layout pass that runs inside a paint frame — specifically during meta_window_actor_paint_to_content, which GNOME Shell calls to capture a window snapshot at the start of the maximize animation. If keepRoundedCorners.maximized is false (the default), the notify::size callback triggered refreshRoundedCorners → onRemoveEffect for the newly- maximized window. onRemoveEffect called actor.remove_effect_by_name() while CLUTTER_ACTOR_IN_PAINT was set, which corrupted the RoundedCornersEffect's actor pointer via clutter_actor_meta_set_actor. The subsequent clutter_actor_continue_paint call then hit clutter_actor_node_new(NULL) → SIGABRT. The same class of bug exists in the notify::first-child and notify::wm-class deferred callbacks: both call applyEffectTo synchronously, which can invoke onAddEffect (add_effect_with_name) while the actor is mid-paint if the Wayland event or layout notification fires inside a paint frame. Fix by wrapping all seven signal callbacks in applyEffectTo, and the two deferred-apply callbacks in enableEffect, with GLib.idle_add at PRIORITY_DEFAULT_IDLE. This pushes effect add/remove out of any active paint frame into the next GLib main loop iteration, where it is safe to modify the actor's effect list. The previous fix (4af9a02) addressed the null-metaWindow path that led to the same assertion. This commit addresses the remaining path where metaWindow is valid but effect mutations still race with an active paint. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nimize Chromium-based browsers use xdg_toplevel.set_minimized for their own title bar minimize, which causes Wayland surface staleness. On restore, the stale surface makes the GLSL rounded corner shader render with wrong bounds, producing a visible glitch until focus fires and self-corrects. Fix by scheduling a refreshRoundedCorners call 250ms after unminimize for Chromium windows, giving the Wayland surface time to deliver a fresh frame before the shader bounds are recomputed. Compositor-initiated minimizes (Super+H/D) do not cause surface staleness and are unaffected. The overview thumbnail continues to show the stale Chromium surface while minimized — this is a known Chromium/Wayland limitation outside the extension's control. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Conflicts: # src/manager/utils.ts # src/patch/add_shadow_in_overview.ts
|
@GrzegorzKozub could you please try this build? https://github.com/flexagoon/rounded-window-corners/actions/runs/29527132270 It fixes the minimize glitch for me, and I wasn't able to reproduce the screen lock one |
|
Hi @flexagoon 😃 Please notice that I have been monitoring this extension not only by reproducing reported issues but also looking at crashes and journal issues as I've been using it across my machines. The result of this is a bundle of fixes and guards for different issues I came across. These fixes are listed and documented in this PR body. I do believe the changes here have stabilized this great extension a lot. I've been running the code version from this PR since its creation and kept the code rebased on your changes for good measure. I have also disclosed that AI has been used to analyse these bugs, find the relevant docs and e.g. GNOME code, and fix them. Maybe we could leverage this work and allow other users to benefit instead of bisecting. If we get crashes or side effects we can always revert. |
|
@GrzegorzKozub I understand, I appreciate your work and I'm using your PR as reference. I just believe there are simpler ways to fix especially the Chromium issue, which is what I tried to do in the build I linked. Could you please try it and see if it works? As for the other issues like crashes and optimizations, some of those have already been fixed (eg. in 25bcb35), and for the rest I will rebase your fixes as separate commits and add them to the code (you'll still be listed as the commit author). I just want to deal with the Chromium issue first. |
|
Looked at the actual build ( event_handlers.ts: +6 -1
utils.ts: +25 -0It adds an Two things that make me less confident it fully replaces that fix:
Scope-wise, it only targets one of the six fixes bundled here:
Happy to test the build regardless — just flagging that a pass/fail result there only speaks to the first item, not the rest of this PR. 🤖 This comment was drafted with AI assistance (Claude Code). |
|
@GrzegorzKozub yes, I know what my commit does, please test it |
|
@GrzegorzKozub please don't add more stuff to this pull requests, extra changes should be submitted as separate PRs |
|
Sorry friend, but most likely I won't be contributing to this fork anymore. My fork remains active and I'll keep it up to date with yours for my own use. Feel free to integrate any of my changes at your preferred pace. You can close this merge request or keep it to track the diff, up to you. I've also switched the AUR package I maintain to my fork so you may want to remark this in the |
|
Fix: Chromium corner glitch on restore after app-initiated minimize Fixed by f71ef3f Fix: Chromium effect reapply after screen lock/unlock Can't reproduce Fix: Overview shadow allocation crash Can't reproduce Fix: Silence expected permission errors on /proc//maps Fixed in 639bace Fix: clutter_actor_node_new crash on new browser windows Can't reproduce Fix: effect add/remove mid-paint crash Can't reproduce Performance improvements Probably hallucinated. GSettings are designed to be fast. The system itself reads those preferences many times a second with no performance impact, so this is not a real performance issue. |
|
@GrzegorzKozub could you please also update the extension metadata in your fork (repository URL and my username) to avoid confusion? https://github.com/GrzegorzKozub/rounded-window-corners/blob/main/resources/metadata.json |
Upstream has not integrated these fixes (flexagoon#144), so document them here for users of this fork and link from the README. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Upstream has not integrated these fixes (flexagoon#144), so document them here for users of this fork and link from the README. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Bundle of small fixes and performance improvements accumulated while chasing #124.
Changes
Fix: Chromium corner glitch on restore after app-initiated minimize (#124)
Chromium-based browsers implement their own title bar and context menu, using
xdg_toplevel.set_minimizedto request minimize from the compositor. This is distinct from compositor-initiated minimizes (Super+H, Super+D), which do not exhibit the issue. When Chromium sendsxdg_toplevel.set_minimized, the compositor acknowledges the minimized state and Chromium stops updating its Wayland surface buffer — the last rendered frame is held. On restore, the stale surface causes the GLSL rounded corner shader to render with wrong bounds, producing a visible glitch until the window receives focus and self-corrects.event_handlers.ts: InonUnminimize, detect Chromium windows outside the magic lamp path and schedule a 250msrefreshRoundedCornerscall. This gives Chromium time to deliver a fresh post-restore frame before the shader uniforms are recomputed. The effect is never disabled during the minimize state, so there is no hover delay in the overview and intermediate focus/size change callbacks are not blocked.Known limitation: The overview thumbnail for a minimized Chromium window shows square corners — the
Shell.GLSLEffectshader does not apply throughClutter.Clone, so the raw window surface is rendered. This is a compositor-level constraint that cannot be addressed from the extension side.Fix: Chromium effect reapply after screen lock/unlock (#124)
GNOME Shell disables and re-enables extensions during screen lock/unlock. When the extension re-enables, Chromium-based browsers (Brave, Chrome, Edge) may render stale surfaces for unfocused windows. The compositor skips repainting GLSL effects for these windows, resulting in scrambled borders and a doubled frame.
event_manager.ts: After applying effects to existing windows on re-enable, wait 250ms for surfaces to settle, then briefly focus each unfocused Chromium window (detected bywm_class) to force the compositor to repaint the GLSL effect, then restore focus to the original window.event_handlers.ts: PromoteonFocusChangedfromrefreshShadowtorefreshRoundedCornersso that any focus change recomputes shader bounds, not just the shadow.utils.ts: AddisChromiumWindowhelper matching known Chromium-based browserwm_classvalues via regex, includingbrave-origin(Brave PWAs / site-specific browser instances).Fix: Overview shadow allocation crash
add_shadow_in_overview.ts: Guard against zero frame width invfunc_allocateon the overview shadow clone, which previously causedNaNvalues in the allocation box and triggered Clutter assertion failures.Fix: Silence expected permission errors on
/proc/<pid>/mapsgetAppTypereads/proc/<pid>/mapsto detect LibHandy / LibAdwaita windows. For processes owned by another user (flatpak sandboxes, root-owned apps), the file is unreadable and GIO raisesPERMISSION_DENIED. The existing fallback already handled the outcome, but the error was logged with a full stack trace on every such window.manager/utils.ts: TreatPERMISSION_DENIEDandNOT_FOUNDfrom/proc/<pid>/mapsas expected and log them at debug level; keeplogErrorfor everything else.Fix:
clutter_actor_node_newcrash on new browser windows (GNOME 50.2)On mutter 50.2 (Wayland-only),
actor.metaWindowcan be null during window actor lifecycle transitions. Previously,applyEffectToconnectednotify::sizeandsize-changedbefore checkingactor.metaWindow. WhenmetaWindowwas null, the resulting TypeError aborted the function mid-way, leaving size signals connected but the effect never added. On the nextnotify::size(fired during Clutter layout),onAddEffectwas called while the actor was mid-paint, corrupting the effect's actor pointer and causingclutter_actor_node_new(NULL)→ SIGABRT.event_manager.ts: Guardactor.metaWindowbefore connecting any signals inapplyEffectToso the function either completes fully or returns early with nothing connected. Also null-guardactorfromget_compositor_private()in thewindow-createdhandler, and re-fetch a fresh actor in thewm-classdeferred callback instead of using the captured (potentially stale) reference.event_handlers.ts: Null-guardactor.metaWindowat the top ofonAddEffectandrefreshRoundedCorners.Fix: effect add/remove mid-paint crash (GNOME 50.2,
keepRoundedCorners.maximized = false)A second crash path in the same assertion (
clutter_actor_node_new(NULL)) was observed after the null-metaWindowfix above was deployed. Root cause: Clutter emitsnotify::sizeandsize-changedduring a layout pass that runs inside a paint frame — specifically duringmeta_window_actor_paint_to_content, which GNOME Shell calls to capture a window snapshot at the start of the maximize animation.With
keepRoundedCorners.maximized = false(the default), thenotify::sizecallback triggeredrefreshRoundedCorners→onRemoveEffectfor the newly-maximized window.onRemoveEffectcalledactor.remove_effect_by_name()whileCLUTTER_ACTOR_IN_PAINTwas set, which corrupted theRoundedCornersEffect's actor pointer viaclutter_actor_meta_set_actor. The subsequentclutter_actor_continue_paintcall then hitclutter_actor_node_new(NULL)→ SIGABRT.event_manager.ts: Wrap all seven signal callbacks inapplyEffectTo, and the two deferred-apply callbacks inenableEffect, withGLib.idle_add(GLib.PRIORITY_DEFAULT_IDLE, ...). This pushes effect add/remove out of any active paint frame into the next GLib main loop iteration, where it is safe to modify the actor's effect list.Performance improvements
While investigating the above, noticed several hot paths that re-read GSettings per-frame or per-event. Each
getPref()call deserializes through D-Bus and GLib Variant unpacking, which adds up quickly during shader updates and resize/focus events.settings.ts: Cache deserialized pref values, invalidated automatically via the GSettingschangedsignal. This eliminates repeated unpacks fordebug-mode(hit fromlogDebugin every hot-path statement),blacklist/whitelist(read pershouldEnableEffect),keep-shadow-for-maximized-fullscreen,tweak-kitty-terminal, and the shadow dictionaries.utils.ts—updateShadowActorStyle: Readglobal-rounded-corner-settingsonce and deriveborderRadius,padding, andsmoothingfrom it, instead of 4 separategetPrefcalls.utils.ts—computeBounds: Reorder the kitty check to comparewm_classfirst, so non-kitty windows skip thegetPrefentirely.Test plan
journalctl --user -f -u gnome-shell— noGio.IOErrorEnum: Error opening file /proc/<pid>/mapsstack traces.clutter_actor_node_newcrash / gnome-shell SIGABRT.keepRoundedCorners.maximized = false(default) — no crash on window creation or focus change.Fixes #124
AI contribution disclosure
This pull request was developed with significant AI assistance (Claude). All changes were manually reviewed, tested on a live GNOME Shell session, and verified to be necessary and correct.
🤖 Generated with Claude Code