Skip to content

Conversation

@crimist
Copy link
Contributor

@crimist crimist commented Dec 10, 2025

Disclosure

I am not familiar with gnome extension programming an used an LLM to generate these changes. Please review thoroughly.

Test plan

I've been using this patch on multiple machines for the last week and have experienced no adverse behavior. The patch works as intended.

Description

The bug was introduced in commit 8ad9c9b (PR 484) when refactoring to fix
dangling pointers. The refactoring changed addFloatOverride() and
removeFloatOverride() to work with fresh data from ConfigManager via
currentProps, but forgot to update the local this.windowProps cache.

This caused toggleFloatingMode() to fail because:

  1. First toggle: addFloatOverride() saves to config but doesn't update cache
  2. isFloatingExempt() still reads from old cache, doesn't see the new override
  3. Second toggle: thinks window is still not floating, tries to add again
  4. addFloatOverride() sees duplicate in fresh config data and returns early
  5. No visual change occurs because the window state never actually toggles

The fix updates this.windowProps after modifying overrides in both
addFloatOverride() and removeFloatOverride() to keep the cache in sync.

Additionally, the early return check in addFloatOverride() was too aggressive
and would prevent adding wmId overrides for different windows of the same class.
The fix now properly checks wmId when withWmId is true, allowing multiple
windows of the same class to have individual wmId overrides.

Fixes: Super+C and Super+Shift+C shortcuts now properly toggle and persist
floating window state as intended.

@jmmaranan
Copy link
Collaborator

@crimist - looks good to me. @juarezr - would you be able to review?

@juarezr
Copy link
Contributor

juarezr commented Dec 16, 2025

@crimist - looks good to me. @juarezr - would you be able to review?

I'm a little bit busy right now.
I have seen this PR before, but I was unable to find some spare time to review it.
Please feel free to move forward if, at the moment you read, I haven't reviewed it yet.

@crimist
Copy link
Contributor Author

crimist commented Dec 16, 2025

FWIW I've now been using this PR for just over 3 weeks with no adverse behavior on both single display and multi-display setups.

@jmmaranan jmmaranan requested review from jmmaranan and removed request for juarezr December 18, 2025 01:22
@jmmaranan jmmaranan merged commit a2fabeb into forge-ext:main Dec 18, 2025
2 checks passed
@cah-kangkung
Copy link

cah-kangkung commented Jan 6, 2026

it's merged to main but not yet applied in the latest release build, have to manually install it. confirmed working on GNOME v46

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants