Added feature to resize horizontally & apply XY position offset #1833
Open
meepak wants to merge 2 commits into
Open
Added feature to resize horizontally & apply XY position offset #1833meepak wants to merge 2 commits into
meepak wants to merge 2 commits into
Conversation
meepak
commented
Mar 21, 2026
meepak
commented
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a secondary, independent Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Application
participant Prefs as GSettings
participant WM as WindowManager
participant Geo as WindowGeometry
User->>App: Trigger secondary inc/dec action (shortcut)
App->>Prefs: Update `window-size-secondary`
Prefs-->>App: Emit settings-changed
App->>WM: Notify settings change
WM->>Geo: get_target_rect(workarea, monitor_scale, size, size_secondary, pos)
Geo-->>WM: Return computed rect (primary + secondary scaling)
WM->>Window: Apply computed target rect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddterm/app/application.js`:
- Around line 340-349: Handlers 'window-size-dec' and 'window-size-inc' always
modify 'window-size-y', which breaks left/right placements; change them to
choose the axis key based on the active window edge: use 'window-size-x' when
placement is left/right and 'window-size-y' when placement is top/bottom, then
call `#settings.set_double`(...) or `#adjust_double_setting`(...) with that selected
key. Keep the existing maximize check (`#settings.get_boolean`('window-maximize'))
and logic for decrement/increment (using 1.0 - SIZE_STEP for set_double on
maximize and `#adjust_double_setting` for non-maximized), but replace the
hardcoded 'window-size-y' with the computed axis key so side windows adjust
width instead of height. Ensure both 'window-size-dec' and 'window-size-inc' use
the same axis-selection logic.
In `@ddterm/pref/positionsize.js`:
- Around line 283-297: The two slider config objects passed to add_slider (keys
'window-size-x' and 'window-size-y') are missing trailing commas after the final
property; update each object so the last property line "upper: 1" has a trailing
comma (i.e., change the object literals in the add_slider calls for
window-size-x and window-size-y to include a comma after upper: 1) to satisfy
the project's lint/CI rules.
In `@ddterm/shell/geometry.js`:
- Around line 173-209: get_target_rect's side-position branch (Meta.Side.LEFT /
RIGHT) never applies size_x so target_rect.width remains workarea.width; update
that branch in get_target_rect to scale target_rect.width by size_x and reduce
it by target_rect.width % monitor_scale (same pattern used for height/else
branch), then for RIGHT compute target_rect.x += workarea.width -
target_rect.width (keeping the existing RIGHT adjustment) so the width reduction
actually shifts the rect inward; keep the existing height scaling and the
existing pos_x_offset/pos_y_offset and clamping logic unchanged.
In `@ddterm/shell/wm.js`:
- Around line 568-569: The code unconditionally calls
this.#unmaximize_for_resize(Meta.MaximizeFlags.BOTH) when a resize grab starts,
which wipes both saved size axes; change it to derive the axis from the incoming
grab flags and only unmaximize that axis (e.g., if flags indicate a horizontal
grab use Meta.MaximizeFlags.HORIZONTAL, if vertical use
Meta.MaximizeFlags.VERTICAL), and pass that derived flag into
`#unmaximize_for_resize` so the untouched dimension's saved size is preserved;
apply the same change to the other similar unmaximize-for-resize call sites in
this file (the block around the other resize handling).
In `@schemas/com.github.amezin.ddterm.gschema.xml`:
- Around line 137-143: Add a startup migration in the preferences/settings
initialization code to detect the legacy key "window-size" and seed the new keys
"window-size-x" and "window-size-y" before any defaults are applied: read the
legacy value once, parse it accepting formats like "W", "W,H" or "WxH", and then
set "window-size-x" to the first numeric component and "window-size-y" to the
second (or leave height as the existing default if only one value is present);
ensure this runs in the configuration load path so existing users' custom sizes
are preserved and remove/ignore the legacy key afterwards.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b051904-af84-4234-9755-1793670ba1ab
📒 Files selected for processing (5)
ddterm/app/application.jsddterm/pref/positionsize.jsddterm/shell/geometry.jsddterm/shell/wm.jsschemas/com.github.amezin.ddterm.gschema.xml
📜 Review details
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2026-03-03T07:34:39.235Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1788
File: ddterm/app/application.js:709-744
Timestamp: 2026-03-03T07:34:39.235Z
Learning: In ddterm/app/, prefer using underscore-prefixed names for methods intended to be overridden (e.g., _ensure_window) rather than private `#private` fields, when inheritance in dev-application.js or other subclasses is expected. Private class fields (#) cannot be overridden by subclasses, so use the underscore convention for 'protected' methods that need inheritance support. Apply this pattern to all JavaScript files under ddterm/app where subclassing or overrides are anticipated.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: In the ddterm/app directory, treat code as GTK3-only and do not introduce GTK3/4 compatibility wrappers. For files under this directory (e.g., ddterm/app/tablabel.js), use direct imports from gi://Handy when needed; avoid adding compatibility shims. This pattern applies to all JavaScript files within ddterm/app.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: In ddterm/app/ files (e.g., ddterm/app/tablabel.js), do not specify versions in gi:// imports. Versioning is centralized in ddterm/app/init.js. Ensure all gi:// imports in this directory omit version specifications to maintain consistent, centralized version management.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-19T04:39:16.517Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/notebook.js:162-164
Timestamp: 2026-02-19T04:39:16.517Z
Learning: In GJS (GNOME JavaScript), when disconnecting C-level signal handlers from GTK/libhandy widgets with GObject.signal_handlers_disconnect_matched(), you can rely on matching by signalId alone for JavaScript handlers. Do not require func in this case; using only { signalId: 'signal-name' } will disconnect the corresponding handlers (e.g., disconnecting libhandy’s built-in key-press-event from Handy.TabView with { signalId: 'key-press-event' }). Apply this pattern in JavaScript files under ddterm/app/ to identify and safely detach signal handlers by signalId when appropriate, and verify the specific library behavior for each signal.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: In ddterm, require using set_visual() with an RGBA visual to enable transparency on the X11 backend (including XWayland and older GNOME Shell sessions). This guideline applies to files under ddterm/app (e.g., ddterm/app/appwindow.js). Ensure the visual is RGBA where transparency is needed and that this applies to X11/XWayland backends rather than GNOME Shell-only assumptions.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-19T03:46:35.526Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/terminalpage.js:116-119
Timestamp: 2026-02-19T03:46:35.526Z
Learning: In the ddterm/gnome-shell-extension-ddterm repository, in GJS-based signal definitions (param_types), it is acceptable to use JavaScript primitives like String and Boolean directly. Do not suggest replacing them with GObject.TYPE_STRING or GObject.TYPE_BOOLEAN. This guidance applies across JavaScript files in this project (not just this file) where signal parameter types rely on native JS constructors.
Applied to files:
ddterm/app/application.jsddterm/shell/wm.jsddterm/pref/positionsize.jsddterm/shell/geometry.js
📚 Learning: 2026-03-02T03:21:20.247Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1774
File: ddterm/app/appwindow.js:41-47
Timestamp: 2026-03-02T03:21:20.247Z
Learning: In GJS-based code, EXPLICIT_NOTIFY suppresses automatic notify only for properties with manually defined getters/setters. If a property has EXPLICIT_NOTIFY but no manually defined setter, GJS will generate a setter that calls notify() when the value changes. Apply this understanding to ddterm's JavaScript files to avoid assuming no notify emission for such properties; verify whether a manual setter exists before deciding if notify() should be emitted or suppressed.
Applied to files:
ddterm/app/application.jsddterm/shell/wm.jsddterm/pref/positionsize.jsddterm/shell/geometry.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: For the ddterm GNOME Shell extension, ensure all design decisions and code reviews assume GNOME Shell as the only supported environment. Do not introduce runtime or UI assumptions for other desktops; verify compatibility with GNOME Shell APIs and extension environment in all JavaScript files under the ddterm directory (e.g., ddterm/app/appwindow.js).
Applied to files:
ddterm/app/application.jsddterm/shell/wm.jsddterm/pref/positionsize.jsddterm/shell/geometry.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: Ensure GTK3/GTK4 compatibility wrappers exist in the ddterm/pref/ directory so that modules can be loaded by the GNOME Shell extensions service (GTK4). This applies to all JavaScript files under ddterm/pref (e.g., ddterm/pref/*.js).
Applied to files:
ddterm/pref/positionsize.js
📚 Learning: 2026-01-07T04:34:03.958Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:34:03.958Z
Learning: For ddterm/app/, versions of gi:// imports are set centrally in ddterm/app/init.js, not in individual files. Individual files in ddterm/app/ should not include version specifications in their gi:// imports.
Applied to files:
ddterm/pref/positionsize.js
📚 Learning: 2026-02-24T06:34:22.064Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1778
File: ddterm/app/terminal.js:332-338
Timestamp: 2026-02-24T06:34:22.064Z
Learning: In ddterm/app/terminal.js, when adding properties to TerminalBase that need to be bound from TerminalSettings (via PROPERTIES_CLONE in terminalsettings.js), the property should be declared as READWRITE | EXPLICIT_NOTIFY in TerminalBase, and the implementation logic should be inside TerminalBase itself (e.g., in _init()), not in external files like terminalpage.js. This ensures proper encapsulation and allows TerminalSettings to bind GSettings values to the terminal's properties.
<!-- [add_learning]
In the ddterm/gnome-shell-extension-ddterm repository, behavioral properties like copy-on-selection should be implemented within TerminalBase (ddterm/app/terminal.js) rather than in terminalpage.js. The terminal widget should own and encapsulate its own behavior by connecting to its signals in _init() and reacting to its own property changes.
Applied to files:
ddterm/pref/positionsize.jsddterm/shell/geometry.js
📚 Learning: 2026-01-05T05:06:57.054Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1665
File: ddterm/pref/shortcuts.js:256-262
Timestamp: 2026-01-05T05:06:57.054Z
Learning: In GNOME Shell Extensions using GTK via GJS, Gtk.Label and Gtk.ShortcutLabel cannot receive keyboard focus; calling grab_focus() on them has no effect. When reviewing code in this project (e.g., ddterm/pref/*.js), avoid relying on focus on these label widgets. If you need to manage focus, apply it to a focusable widget (like an Entry or a button), or move the focus handling to a container that can receive focus. Do not call grab_focus() on Gtk.Label or Gtk.ShortcutLabel, and verify any focus-related logic targets appropriate widgets.
Applied to files:
ddterm/pref/positionsize.js
📚 Learning: 2026-01-07T04:34:03.958Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:34:03.958Z
Learning: The ddterm/pref/ directory needs GTK 3/4 compatibility wrappers because it can be loaded by GNOME Shell extensions service, which is GTK 4.
Applied to files:
schemas/com.github.amezin.ddterm.gschema.xml
🪛 GitHub Actions: pr
ddterm/pref/positionsize.js
[error] 288-288: ESLint (comma-dangle): Missing trailing comma at 288:21.
🪛 GitHub Check: ci / lint
ddterm/pref/positionsize.js
[failure] 296-296:
Missing trailing comma comma-dangle
[failure] 288-288:
Missing trailing comma comma-dangle
ddterm/shell/geometry.js
[failure] 173-173:
This line has a length of 109. Maximum allowed is 100 max-len
[failure] 414-414:
Unexpected trailing comma comma-dangle
Comment on lines
340
to
349
| 'window-size-dec': () => { | ||
| if (this.#settings.get_boolean('window-maximize')) | ||
| this.#settings.set_double('window-size', 1.0 - SIZE_STEP); | ||
| this.#settings.set_double('window-size-y', 1.0 - SIZE_STEP); | ||
| else | ||
| this.#adjust_double_setting('window-size', -SIZE_STEP); | ||
| this.#adjust_double_setting('window-size-y', -SIZE_STEP); | ||
| }, | ||
| 'window-size-inc': () => { | ||
| if (!this.#settings.get_boolean('window-maximize')) | ||
| this.#adjust_double_setting('window-size', SIZE_STEP); | ||
| this.#adjust_double_setting('window-size-y', SIZE_STEP); | ||
| }, |
There was a problem hiding this comment.
Choose the resize key from the active window edge.
These handlers now always update window-size-y. That regresses left/right placements: the existing window-size-inc/dec actions change height instead of the side window's width.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ddterm/app/application.js` around lines 340 - 349, Handlers 'window-size-dec'
and 'window-size-inc' always modify 'window-size-y', which breaks left/right
placements; change them to choose the axis key based on the active window edge:
use 'window-size-x' when placement is left/right and 'window-size-y' when
placement is top/bottom, then call `#settings.set_double`(...) or
`#adjust_double_setting`(...) with that selected key. Keep the existing maximize
check (`#settings.get_boolean`('window-maximize')) and logic for
decrement/increment (using 1.0 - SIZE_STEP for set_double on maximize and
`#adjust_double_setting` for non-maximized), but replace the hardcoded
'window-size-y' with the computed axis key so side windows adjust width instead
of height. Ensure both 'window-size-dec' and 'window-size-inc' use the same
axis-selection logic.
Comment on lines
+568
to
+569
| if (MOUSE_RESIZE_GRABS.includes(flags)) | ||
| this.#unmaximize_for_resize(this.geometry.maximize_flag); | ||
| this.#unmaximize_for_resize(Meta.MaximizeFlags.BOTH); |
There was a problem hiding this comment.
Don't overwrite both saved size axes on resize-from-maximize.
Unmaximizing here is reasonable, but forcing both window-size-x and window-size-y to 1.0 throws away the user's saved cross-axis size before the drag even starts. A maximized top window with a custom width becomes full-width as soon as the user grabs only the bottom edge. Please derive the reset axis from the grab op and leave the untouched dimension alone.
Also applies to: 590-601
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ddterm/shell/wm.js` around lines 568 - 569, The code unconditionally calls
this.#unmaximize_for_resize(Meta.MaximizeFlags.BOTH) when a resize grab starts,
which wipes both saved size axes; change it to derive the axis from the incoming
grab flags and only unmaximize that axis (e.g., if flags indicate a horizontal
grab use Meta.MaximizeFlags.HORIZONTAL, if vertical use
Meta.MaximizeFlags.VERTICAL), and pass that derived flag into
`#unmaximize_for_resize` so the untouched dimension's saved size is preserved;
apply the same change to the other similar unmaximize-for-resize call sites in
this file (the block around the other resize handling).
Comment on lines
137
to
143
| <key name="window-size-x" type="d"> | ||
| <default>1.0</default> | ||
| <range min="0.0" max="1.0"/> | ||
| </key> | ||
| <key name="window-size-y" type="d"> | ||
| <default>0.6</default> | ||
| <range min="0.0" max="1.0"/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Exact legacy key references:\n'
rg -nP --glob '!po/**' --type js --type xml '(?<!-)window-size(?!-)' .
printf '\nPossible migration / compatibility hooks:\n'
rg -n -C2 --glob '!po/**' --type js --type xml 'migrat|legacy|compat|upgrade|dconf' .Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 1502
🏁 Script executed:
cat -n ./ddterm/pref/compatibility.jsRepository: ddterm/gnome-shell-extension-ddterm
Length of output: 2910
🏁 Script executed:
find . -name "*.xml" -path "*/dbus/*" ! -path "*/tests/*" | head -20Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 61
🏁 Script executed:
rg -n 'window-size' --type js --glob '!po/**' -A 2 -B 2 .Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 5908
🏁 Script executed:
rg -n 'settings|reset|migrate' --type js ./ddterm/pref/ -A 2 -B 2 | head -100Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 4263
🏁 Script executed:
rg -n 'GSettings|dconf' --type js --glob '!po/**' | head -50Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 61
🏁 Script executed:
rg -n 'schema|version|initialize|new_settings|Settings' --type js ./ddterm/shell/ ./ddterm/app/ -A 1 -B 1 | head -80Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 4571
🏁 Script executed:
ls -la ./ddterm/ && cat -n ./ddterm/shell.js 2>/dev/null || head -50 ./ddterm/extension.jsRepository: ddterm/gnome-shell-extension-ddterm
Length of output: 650
🏁 Script executed:
rg -n 'relocatable_schema|schema.version|migrate' --type js --type xmlRepository: ddterm/gnome-shell-extension-ddterm
Length of output: 61
🏁 Script executed:
cat -n schemas/com.github.amezin.ddterm.gschema.xml | head -80Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 3016
🏁 Script executed:
find ./ddterm -name "*.js" -type f -exec grep -l 'init\|reset.*settings\|migrate' {} \;Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 333
🏁 Script executed:
cat -n ./ddterm/app/application.js | head -150Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 5731
🏁 Script executed:
rg -n 'reset|default' --type js ./ddterm/app/ ./ddterm/shell/ -B 2 -A 2 | head -80Repository: ddterm/gnome-shell-extension-ddterm
Length of output: 4535
Verify the upgrade path for the legacy window-size value.
There is no migration code in the codebase. If users upgrade with a custom window-size value, it will silently reset to the new defaults (1.0 for width, 0.6 for height). The legacy key only appears in test fixtures; all production code uses window-size-x and window-size-y exclusively. Before merging, add migration logic to read the old key once during initialization and seed the new keys, or document that this is a breaking change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schemas/com.github.amezin.ddterm.gschema.xml` around lines 137 - 143, Add a
startup migration in the preferences/settings initialization code to detect the
legacy key "window-size" and seed the new keys "window-size-x" and
"window-size-y" before any defaults are applied: read the legacy value once,
parse it accepting formats like "W", "W,H" or "WxH", and then set
"window-size-x" to the first numeric component and "window-size-y" to the second
(or leave height as the existing default if only one value is present); ensure
this runs in the configuration load path so existing users' custom sizes are
preserved and remove/ignore the legacy key afterwards.
Member
|
Please fix issues reported by ESLint, testing won't start without it. Also, as discussed previously in similar pull request(s), an approach with "primary size" and "secondary size" will likely be better - i.e. for top/bottom position, height is "primary" (and controlled by Also, maximizing/unmaximizing the window with Regarding comments made by AI - feel free to ignore them if you think they're wrong. |
amezin
requested changes
Mar 21, 2026
Member
amezin
left a comment
amezin
left a comment
There was a problem hiding this comment.
Let's try to:
- Reduce the amount of changes in this PR (keep
window-sizebehavior, don't settings for offsets for now) - This change needs tests - and getting them to pass will, most likely, be the hardest part
| </key> | ||
| <key name="pos-x-offset" type="d"> | ||
| <default>0.0</default> | ||
| <range min="-10000.0" max="10000.0"/> |
Member
There was a problem hiding this comment.
To simplify things, let's start with centered (along the "secondary" axis) window
|
|
||
| <schema path="/com/github/amezin/ddterm/" id="com.github.amezin.ddterm"> | ||
| <key name="window-size" type="d"> | ||
| <key name="window-size-x" type="d"> |
Member
There was a problem hiding this comment.
Let's keep window-size setting, and add window-size-secondary. When position is top/bottom, window-size will control height, window-size-secondary will control width; and when position is left/right, window-size should control width, and window-size-secondary should control height. I.e. just keep the existing behavior for window-size, so, for example, keybinding actions will keep working.
Author
There was a problem hiding this comment.
Implemented as you suggested. I removed position offset from this branch.
Some of the tests are failing, for me those are failing in master branch too. Maybe you can have a look and suggest something. Thanks.
Member
There was a problem hiding this comment.
for me those are failing in master branch too
Could you tell me which tests exactly?
All tests pass on master, both in CI environment(s), and on my laptop
meepak
force-pushed
the
allow-width-resize-and-position-xy
branch
from
9bd15f2 to
29e4d63
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ddterm/shell/wm.js (1)
564-569:⚠️ Potential issue | 🟠 MajorChoose the unmaximize/reset axis from the grabbed edge.
#grab_op_begin()still passesthis.geometry.maximize_flag, so a top/bottom window dragged from the left/right edge unmaximizes the vertical axis instead of the horizontal one. The newwindow-size-secondary = 1.0reset then also wipes the saved cross-axis size before the drag starts. DeriveMeta.MaximizeFlags.HORIZONTAL/VERTICAL/BOTHfrom the grab op, and only reset the matching setting(s).Suggested fix
`#grab_op_begin`(display, win, flags) { if (win !== this.window) return; - if (MOUSE_RESIZE_GRABS.includes(flags)) - this.#unmaximize_for_resize(this.geometry.maximize_flag); + let maximize_flags = 0; + + switch (flags) { + case Meta.GrabOp.RESIZING_W: + case Meta.GrabOp.RESIZING_E: + maximize_flags = Meta.MaximizeFlags.HORIZONTAL; + break; + case Meta.GrabOp.RESIZING_N: + case Meta.GrabOp.RESIZING_S: + maximize_flags = Meta.MaximizeFlags.VERTICAL; + break; + case Meta.GrabOp.RESIZING_NW: + case Meta.GrabOp.RESIZING_NE: + case Meta.GrabOp.RESIZING_SW: + case Meta.GrabOp.RESIZING_SE: + maximize_flags = Meta.MaximizeFlags.BOTH; + break; + } + + if (maximize_flags) + this.#unmaximize_for_resize(maximize_flags); } @@ // There is a _update_window_geometry() call after successful unmaximize. // It must set window size to 100%. - this.settings.set_double('window-size', 1.0); - this.settings.set_double('window-size-secondary', 1.0); + const primary_flag = this.geometry.orientation === Clutter.Orientation.HORIZONTAL + ? Meta.MaximizeFlags.HORIZONTAL + : Meta.MaximizeFlags.VERTICAL; + const secondary_flag = primary_flag === Meta.MaximizeFlags.HORIZONTAL + ? Meta.MaximizeFlags.VERTICAL + : Meta.MaximizeFlags.HORIZONTAL; + + if (flags & primary_flag) + this.settings.set_double('window-size', 1.0); + if (flags & secondary_flag) + this.settings.set_double('window-size-secondary', 1.0);Also applies to: 595-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddterm/shell/wm.js` around lines 564 - 569, The current `#grab_op_begin`(display, win, flags) always passes this.geometry.maximize_flag to `#unmaximize_for_resize`, causing the wrong axis to be reset; instead inspect the grab operation flags (the flags parameter) to derive the axis-specific maximize mask (Meta.MaximizeFlags.HORIZONTAL, VERTICAL, or BOTH) and pass that derived mask into `#unmaximize_for_resize` so only the grabbed axis is unmaximized/reset (and only clear the corresponding window-size-secondary axis). Update the same logic in the other grab handling block (the similar code around the second occurrence) to use the grab op flags rather than geometry.maximize_flag when deciding which axis to reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddterm/app/application.js`:
- Around line 350-353: The handler for 'window-size-secondary-dec' currently
does nothing when '#settings.get_boolean('window-maximize')' is true; change it
to mirror 'window-size-dec' by seeding the secondary size to 1.0 - SIZE_STEP
when maximize is set, then call '#adjust_double_setting('window-size-secondary',
-SIZE_STEP)' so the shortcut can shrink a maximized window; update the
'window-size-secondary-dec' branch to set the 'window-size-secondary' initial
value when 'window-maximize' is true (same approach used by 'window-size-dec'
and consistent with ddterm/shell/wm.js behavior).
In `@schemas/com.github.amezin.ddterm.gschema.xml`:
- Around line 141-144: The new GSettings key "window-size-secondary" has a
compatibility-risky default of 0.6; change its <default> value to 1.0 in the
schema (key name "window-size-secondary") so existing installations keep current
layout sizes on upgrade, or alternatively implement a one-time migration that
writes 1.0 into existing user profiles for "window-size-secondary" before the
new geometry code uses it.
---
Duplicate comments:
In `@ddterm/shell/wm.js`:
- Around line 564-569: The current `#grab_op_begin`(display, win, flags) always
passes this.geometry.maximize_flag to `#unmaximize_for_resize`, causing the wrong
axis to be reset; instead inspect the grab operation flags (the flags parameter)
to derive the axis-specific maximize mask (Meta.MaximizeFlags.HORIZONTAL,
VERTICAL, or BOTH) and pass that derived mask into `#unmaximize_for_resize` so
only the grabbed axis is unmaximized/reset (and only clear the corresponding
window-size-secondary axis). Update the same logic in the other grab handling
block (the similar code around the second occurrence) to use the grab op flags
rather than geometry.maximize_flag when deciding which axis to reset.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c59a8b7f-c25b-411e-8dc1-19f15dd8b2f5
📒 Files selected for processing (7)
ddterm/app/application.jsddterm/pref/positionsize.jsddterm/pref/shortcuts.jsddterm/pref/util.jsddterm/shell/geometry.jsddterm/shell/wm.jsschemas/com.github.amezin.ddterm.gschema.xml
📜 Review details
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2026-01-05T05:06:57.054Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1665
File: ddterm/pref/shortcuts.js:256-262
Timestamp: 2026-01-05T05:06:57.054Z
Learning: In GNOME Shell Extensions using GTK via GJS, Gtk.Label and Gtk.ShortcutLabel cannot receive keyboard focus; calling grab_focus() on them has no effect. When reviewing code in this project (e.g., ddterm/pref/*.js), avoid relying on focus on these label widgets. If you need to manage focus, apply it to a focusable widget (like an Entry or a button), or move the focus handling to a container that can receive focus. Do not call grab_focus() on Gtk.Label or Gtk.ShortcutLabel, and verify any focus-related logic targets appropriate widgets.
Applied to files:
ddterm/pref/shortcuts.jsddterm/pref/util.jsddterm/pref/positionsize.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: Ensure GTK3/GTK4 compatibility wrappers exist in the ddterm/pref/ directory so that modules can be loaded by the GNOME Shell extensions service (GTK4). This applies to all JavaScript files under ddterm/pref (e.g., ddterm/pref/*.js).
Applied to files:
ddterm/pref/shortcuts.jsddterm/pref/util.jsddterm/pref/positionsize.js
📚 Learning: 2026-02-19T03:46:35.526Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/terminalpage.js:116-119
Timestamp: 2026-02-19T03:46:35.526Z
Learning: In the ddterm/gnome-shell-extension-ddterm repository, in GJS-based signal definitions (param_types), it is acceptable to use JavaScript primitives like String and Boolean directly. Do not suggest replacing them with GObject.TYPE_STRING or GObject.TYPE_BOOLEAN. This guidance applies across JavaScript files in this project (not just this file) where signal parameter types rely on native JS constructors.
Applied to files:
ddterm/pref/shortcuts.jsddterm/app/application.jsddterm/pref/util.jsddterm/shell/geometry.jsddterm/pref/positionsize.jsddterm/shell/wm.js
📚 Learning: 2026-03-02T03:21:20.247Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1774
File: ddterm/app/appwindow.js:41-47
Timestamp: 2026-03-02T03:21:20.247Z
Learning: In GJS-based code, EXPLICIT_NOTIFY suppresses automatic notify only for properties with manually defined getters/setters. If a property has EXPLICIT_NOTIFY but no manually defined setter, GJS will generate a setter that calls notify() when the value changes. Apply this understanding to ddterm's JavaScript files to avoid assuming no notify emission for such properties; verify whether a manual setter exists before deciding if notify() should be emitted or suppressed.
Applied to files:
ddterm/pref/shortcuts.jsddterm/app/application.jsddterm/pref/util.jsddterm/shell/geometry.jsddterm/pref/positionsize.jsddterm/shell/wm.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: For the ddterm GNOME Shell extension, ensure all design decisions and code reviews assume GNOME Shell as the only supported environment. Do not introduce runtime or UI assumptions for other desktops; verify compatibility with GNOME Shell APIs and extension environment in all JavaScript files under the ddterm directory (e.g., ddterm/app/appwindow.js).
Applied to files:
ddterm/pref/shortcuts.jsddterm/app/application.jsddterm/pref/util.jsddterm/shell/geometry.jsddterm/pref/positionsize.jsddterm/shell/wm.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: In the ddterm/app directory, treat code as GTK3-only and do not introduce GTK3/4 compatibility wrappers. For files under this directory (e.g., ddterm/app/tablabel.js), use direct imports from gi://Handy when needed; avoid adding compatibility shims. This pattern applies to all JavaScript files within ddterm/app.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: In ddterm/app/ files (e.g., ddterm/app/tablabel.js), do not specify versions in gi:// imports. Versioning is centralized in ddterm/app/init.js. Ensure all gi:// imports in this directory omit version specifications to maintain consistent, centralized version management.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-19T04:39:16.517Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/notebook.js:162-164
Timestamp: 2026-02-19T04:39:16.517Z
Learning: In GJS (GNOME JavaScript), when disconnecting C-level signal handlers from GTK/libhandy widgets with GObject.signal_handlers_disconnect_matched(), you can rely on matching by signalId alone for JavaScript handlers. Do not require func in this case; using only { signalId: 'signal-name' } will disconnect the corresponding handlers (e.g., disconnecting libhandy’s built-in key-press-event from Handy.TabView with { signalId: 'key-press-event' }). Apply this pattern in JavaScript files under ddterm/app/ to identify and safely detach signal handlers by signalId when appropriate, and verify the specific library behavior for each signal.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: In ddterm, require using set_visual() with an RGBA visual to enable transparency on the X11 backend (including XWayland and older GNOME Shell sessions). This guideline applies to files under ddterm/app (e.g., ddterm/app/appwindow.js). Ensure the visual is RGBA where transparency is needed and that this applies to X11/XWayland backends rather than GNOME Shell-only assumptions.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-03-03T07:34:39.235Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1788
File: ddterm/app/application.js:709-744
Timestamp: 2026-03-03T07:34:39.235Z
Learning: In ddterm/app/, prefer using underscore-prefixed names for methods intended to be overridden (e.g., _ensure_window) rather than private `#private` fields, when inheritance in dev-application.js or other subclasses is expected. Private class fields (#) cannot be overridden by subclasses, so use the underscore convention for 'protected' methods that need inheritance support. Apply this pattern to all JavaScript files under ddterm/app where subclassing or overrides are anticipated.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-01-07T04:34:03.958Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:34:03.958Z
Learning: EntryRow is not implemented in libhandy. When needed, it must be imported from ddterm/pref/util.js which provides a custom implementation.
Applied to files:
ddterm/pref/util.js
📚 Learning: 2026-01-07T04:34:03.958Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:34:03.958Z
Learning: The ddterm/pref/ directory needs GTK 3/4 compatibility wrappers because it can be loaded by GNOME Shell extensions service, which is GTK 4.
Applied to files:
schemas/com.github.amezin.ddterm.gschema.xml
📚 Learning: 2026-03-08T15:45:34.487Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1801
File: meson.build:166-180
Timestamp: 2026-03-08T15:45:34.487Z
Learning: In ddterm/gnome-shell-extension-ddterm (meson.build), the `typelibs` list passed to the `gjs-typelib-installer` subproject and the `require({...})` call in `ddterm/app/init.js` do not need a shared single source of truth. If a typelib is in `require()` but missing from the meson `typelibs` list, the app will fail to start and all CI tests will fail (immediately detectable). If a typelib is in the meson `typelibs` list but absent from `require()`, nothing bad happens — only a few unnecessary lines are added to the bundle. The asymmetric failure mode makes the duplication safe.
Applied to files:
schemas/com.github.amezin.ddterm.gschema.xml
📚 Learning: 2026-02-24T06:34:22.064Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1778
File: ddterm/app/terminal.js:332-338
Timestamp: 2026-02-24T06:34:22.064Z
Learning: In ddterm/app/terminal.js, when adding properties to TerminalBase that need to be bound from TerminalSettings (via PROPERTIES_CLONE in terminalsettings.js), the property should be declared as READWRITE | EXPLICIT_NOTIFY in TerminalBase, and the implementation logic should be inside TerminalBase itself (e.g., in _init()), not in external files like terminalpage.js. This ensures proper encapsulation and allows TerminalSettings to bind GSettings values to the terminal's properties.
<!-- [add_learning]
In the ddterm/gnome-shell-extension-ddterm repository, behavioral properties like copy-on-selection should be implemented within TerminalBase (ddterm/app/terminal.js) rather than in terminalpage.js. The terminal widget should own and encapsulate its own behavior by connecting to its signals in _init() and reacting to its own property changes.
Applied to files:
schemas/com.github.amezin.ddterm.gschema.xmlddterm/shell/geometry.js
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 `@ddterm/shell/geometry.js`:
- Around line 81-89: The in-memory ParamSpec for 'window-size-secondary' in
DDTermWindowGeometry is still set to 0.6 while the schema default is 1.0,
causing instances to compute target_rect with the wrong secondary size before
bind_settings() runs; update the GObject.ParamSpec.double default value for
'window-size-secondary' from 0.6 to 1.0 so the in-memory default matches the
schema and the constructor uses the correct initial size.
- Around line 152-180: In get_target_rect, the top/bottom branch incorrectly
multiplies target_rect.height twice instead of applying size_secondary to width;
replace the second height-scaling block with width scaling so target_rect.width
is multiplied by size_secondary, snapped to monitor_scale (subtract modulus),
floored, and then center horizontally (adjust target_rect.x as already done);
keep the existing conditional that applies size when size !== 1 and the
bottom-position offset for target_rect.y unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1b683ee-b4e0-4aad-b0fd-847ed6391873
📒 Files selected for processing (3)
ddterm/app/application.jsddterm/shell/geometry.jsschemas/com.github.amezin.ddterm.gschema.xml
📜 Review details
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: In the ddterm/app directory, treat code as GTK3-only and do not introduce GTK3/4 compatibility wrappers. For files under this directory (e.g., ddterm/app/tablabel.js), use direct imports from gi://Handy when needed; avoid adding compatibility shims. This pattern applies to all JavaScript files within ddterm/app.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: In ddterm/app/ files (e.g., ddterm/app/tablabel.js), do not specify versions in gi:// imports. Versioning is centralized in ddterm/app/init.js. Ensure all gi:// imports in this directory omit version specifications to maintain consistent, centralized version management.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-19T04:39:16.517Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/notebook.js:162-164
Timestamp: 2026-02-19T04:39:16.517Z
Learning: In GJS (GNOME JavaScript), when disconnecting C-level signal handlers from GTK/libhandy widgets with GObject.signal_handlers_disconnect_matched(), you can rely on matching by signalId alone for JavaScript handlers. Do not require func in this case; using only { signalId: 'signal-name' } will disconnect the corresponding handlers (e.g., disconnecting libhandy’s built-in key-press-event from Handy.TabView with { signalId: 'key-press-event' }). Apply this pattern in JavaScript files under ddterm/app/ to identify and safely detach signal handlers by signalId when appropriate, and verify the specific library behavior for each signal.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: In ddterm, require using set_visual() with an RGBA visual to enable transparency on the X11 backend (including XWayland and older GNOME Shell sessions). This guideline applies to files under ddterm/app (e.g., ddterm/app/appwindow.js). Ensure the visual is RGBA where transparency is needed and that this applies to X11/XWayland backends rather than GNOME Shell-only assumptions.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-03-03T07:34:39.235Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1788
File: ddterm/app/application.js:709-744
Timestamp: 2026-03-03T07:34:39.235Z
Learning: In ddterm/app/, prefer using underscore-prefixed names for methods intended to be overridden (e.g., _ensure_window) rather than private `#private` fields, when inheritance in dev-application.js or other subclasses is expected. Private class fields (#) cannot be overridden by subclasses, so use the underscore convention for 'protected' methods that need inheritance support. Apply this pattern to all JavaScript files under ddterm/app where subclassing or overrides are anticipated.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-19T03:46:35.526Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/terminalpage.js:116-119
Timestamp: 2026-02-19T03:46:35.526Z
Learning: In the ddterm/gnome-shell-extension-ddterm repository, in GJS-based signal definitions (param_types), it is acceptable to use JavaScript primitives like String and Boolean directly. Do not suggest replacing them with GObject.TYPE_STRING or GObject.TYPE_BOOLEAN. This guidance applies across JavaScript files in this project (not just this file) where signal parameter types rely on native JS constructors.
Applied to files:
ddterm/app/application.jsddterm/shell/geometry.js
📚 Learning: 2026-03-02T03:21:20.247Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1774
File: ddterm/app/appwindow.js:41-47
Timestamp: 2026-03-02T03:21:20.247Z
Learning: In GJS-based code, EXPLICIT_NOTIFY suppresses automatic notify only for properties with manually defined getters/setters. If a property has EXPLICIT_NOTIFY but no manually defined setter, GJS will generate a setter that calls notify() when the value changes. Apply this understanding to ddterm's JavaScript files to avoid assuming no notify emission for such properties; verify whether a manual setter exists before deciding if notify() should be emitted or suppressed.
Applied to files:
ddterm/app/application.jsddterm/shell/geometry.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: For the ddterm GNOME Shell extension, ensure all design decisions and code reviews assume GNOME Shell as the only supported environment. Do not introduce runtime or UI assumptions for other desktops; verify compatibility with GNOME Shell APIs and extension environment in all JavaScript files under the ddterm directory (e.g., ddterm/app/appwindow.js).
Applied to files:
ddterm/app/application.jsddterm/shell/geometry.js
📚 Learning: 2026-03-08T15:45:34.487Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1801
File: meson.build:166-180
Timestamp: 2026-03-08T15:45:34.487Z
Learning: In ddterm/gnome-shell-extension-ddterm (meson.build), the `typelibs` list passed to the `gjs-typelib-installer` subproject and the `require({...})` call in `ddterm/app/init.js` do not need a shared single source of truth. If a typelib is in `require()` but missing from the meson `typelibs` list, the app will fail to start and all CI tests will fail (immediately detectable). If a typelib is in the meson `typelibs` list but absent from `require()`, nothing bad happens — only a few unnecessary lines are added to the bundle. The asymmetric failure mode makes the duplication safe.
Applied to files:
schemas/com.github.amezin.ddterm.gschema.xml
📚 Learning: 2026-02-24T06:34:22.064Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1778
File: ddterm/app/terminal.js:332-338
Timestamp: 2026-02-24T06:34:22.064Z
Learning: In ddterm/app/terminal.js, when adding properties to TerminalBase that need to be bound from TerminalSettings (via PROPERTIES_CLONE in terminalsettings.js), the property should be declared as READWRITE | EXPLICIT_NOTIFY in TerminalBase, and the implementation logic should be inside TerminalBase itself (e.g., in _init()), not in external files like terminalpage.js. This ensures proper encapsulation and allows TerminalSettings to bind GSettings values to the terminal's properties.
<!-- [add_learning]
In the ddterm/gnome-shell-extension-ddterm repository, behavioral properties like copy-on-selection should be implemented within TerminalBase (ddterm/app/terminal.js) rather than in terminalpage.js. The terminal widget should own and encapsulate its own behavior by connecting to its signals in _init() and reacting to its own property changes.
Applied to files:
schemas/com.github.amezin.ddterm.gschema.xmlddterm/shell/geometry.js
🔇 Additional comments (3)
ddterm/app/application.js (1)
350-360: Secondary-size actions are wired cleanly end-to-end.The maximize-path seeding now mirrors the primary resize action, and the new shortcut keys map directly to the new app actions. That also lines up with the unmaximize flow in
ddterm/shell/wm.js.Also applies to: 430-431
schemas/com.github.amezin.ddterm.gschema.xml (1)
141-144: The schema additions look consistent.
window-size-secondarynow uses the compatibility-safe full-span default, and the new shortcut keys line up with the app-side action names.Also applies to: 392-398
ddterm/shell/geometry.js (1)
136-136: The secondary-size update path is wired through correctly.The new property now participates in notify handling, settings binding, and target-rect recomputation.
Also applies to: 186-195, 377-382
… sizing - Add 'window-size-secondary' GSettings key for controlling secondary dimension - Add keyboard shortcuts for window-size-secondary-inc/dec actions - Update WindowGeometry to use both size and size_secondary for dimension calculation: * For LEFT/RIGHT positions: size controls width, size_secondary controls height * For TOP/BOTTOM positions: size controls height, size_secondary controls width - Update window manager to properly track and persist secondary size on resize - Add preferences UI controls for the new secondary size setting - Existing test suite covers functionality through window resize mechanisms Fix for failing test_resize_maximize_unmaximize Fix: window-size-secondary value defaults to 1 Fix: floating point error in TestWaylandTwoMonitors tests Fix: floating point error in TestWaylandTwoMonitors tests window-size-secondory-dec seeding issue Fix: TOP/BOTTOM case should apply size_secondary to width, not height
meepak
force-pushed
the
allow-width-resize-and-position-xy
branch
from
0395304 to
d4e6ddf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ddterm/shell/wm.js (1)
605-606:⚠️ Potential issue | 🟠 MajorUnconditionally resetting both axes may discard user's saved cross-axis size.
When unmaximizing for resize, both
window-sizeandwindow-size-secondaryare set to1.0. If a user has a maximized top window with a custom width (e.g., 80%) and drags only the bottom edge, the saved width is lost before the drag starts.Consider deriving which axis to reset from the grab operation flags passed to
#grab_op_begin, and only reset the axis being resized.,
Potential approach
- `#unmaximize_for_resize`(flags) { + `#unmaximize_for_resize`(flags, grab_op = null) { this.#cancel_geometry_fixup(); if (!(this.#get_maximize_flags() & flags)) return; this.logger?.log('Unmaximizing for resize'); - // There is a _update_window_geometry() call after successful unmaximize. - // It must set window size to 100%. - this.settings.set_double('window-size', 1.0); - this.settings.set_double('window-size-secondary', 1.0); + // Reset only the axis being resized + const is_vertical_grab = grab_op && [ + Meta.GrabOp.RESIZING_N, Meta.GrabOp.RESIZING_S, + Meta.GrabOp.RESIZING_NW, Meta.GrabOp.RESIZING_NE, + Meta.GrabOp.RESIZING_SW, Meta.GrabOp.RESIZING_SE, + ].includes(grab_op); + const is_horizontal_grab = grab_op && [ + Meta.GrabOp.RESIZING_E, Meta.GrabOp.RESIZING_W, + Meta.GrabOp.RESIZING_NW, Meta.GrabOp.RESIZING_NE, + Meta.GrabOp.RESIZING_SW, Meta.GrabOp.RESIZING_SE, + ].includes(grab_op); + + if (is_vertical_grab || !grab_op) + this.settings.set_double('window-size', 1.0); + if (is_horizontal_grab || !grab_op) + this.settings.set_double('window-size-secondary', 1.0); Main.wm.skipNextEffect(this.#actor); this.#set_unmaximize_flags(flags); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddterm/shell/wm.js` around lines 605 - 606, The code unconditionally resets both axes via settings.set_double('window-size', 1.0) and settings.set_double('window-size-secondary', 1.0), which loses the user's saved cross-axis size; change the logic in the resize/unmaximize flow (the grab_op_begin handler) to inspect the grab operation flags passed to grab_op_begin and only reset the axis being resized (primary vs secondary) instead of both. Locate the call site that currently sets both window-size keys (symbols: settings.set_double, 'window-size', 'window-size-secondary', and function grab_op_begin) and replace it with conditional resets based on the grab flags (e.g., if flags indicate vertical resize reset the vertical/primary key, if horizontal reset the horizontal/secondary key), leaving the other axis untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddterm/pref/positionsize.js`:
- Around line 283-301: The secondary scale row added via add_scale_row with key
'window-size-secondary' is missing a mnemonic underscore; change its title to
use gettext('Window _Size Secondary') (matching the primary 'Window _Size'
mnemonic) so keyboard access is consistent for the add_scale_row entry with key
'window-size-secondary'.
---
Duplicate comments:
In `@ddterm/shell/wm.js`:
- Around line 605-606: The code unconditionally resets both axes via
settings.set_double('window-size', 1.0) and
settings.set_double('window-size-secondary', 1.0), which loses the user's saved
cross-axis size; change the logic in the resize/unmaximize flow (the
grab_op_begin handler) to inspect the grab operation flags passed to
grab_op_begin and only reset the axis being resized (primary vs secondary)
instead of both. Locate the call site that currently sets both window-size keys
(symbols: settings.set_double, 'window-size', 'window-size-secondary', and
function grab_op_begin) and replace it with conditional resets based on the grab
flags (e.g., if flags indicate vertical resize reset the vertical/primary key,
if horizontal reset the horizontal/secondary key), leaving the other axis
untouched.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5fa6ff71-33b0-48ef-9e93-550a0a86ec35
📒 Files selected for processing (7)
ddterm/app/application.jsddterm/pref/positionsize.jsddterm/pref/shortcuts.jsddterm/pref/util.jsddterm/shell/geometry.jsddterm/shell/wm.jsschemas/com.github.amezin.ddterm.gschema.xml
📜 Review details
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2026-01-05T05:06:57.054Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1665
File: ddterm/pref/shortcuts.js:256-262
Timestamp: 2026-01-05T05:06:57.054Z
Learning: In GNOME Shell Extensions using GTK via GJS, Gtk.Label and Gtk.ShortcutLabel cannot receive keyboard focus; calling grab_focus() on them has no effect. When reviewing code in this project (e.g., ddterm/pref/*.js), avoid relying on focus on these label widgets. If you need to manage focus, apply it to a focusable widget (like an Entry or a button), or move the focus handling to a container that can receive focus. Do not call grab_focus() on Gtk.Label or Gtk.ShortcutLabel, and verify any focus-related logic targets appropriate widgets.
Applied to files:
ddterm/pref/shortcuts.jsddterm/pref/positionsize.jsddterm/pref/util.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: Ensure GTK3/GTK4 compatibility wrappers exist in the ddterm/pref/ directory so that modules can be loaded by the GNOME Shell extensions service (GTK4). This applies to all JavaScript files under ddterm/pref (e.g., ddterm/pref/*.js).
Applied to files:
ddterm/pref/shortcuts.jsddterm/pref/positionsize.jsddterm/pref/util.js
📚 Learning: 2026-02-19T03:46:35.526Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/terminalpage.js:116-119
Timestamp: 2026-02-19T03:46:35.526Z
Learning: In the ddterm/gnome-shell-extension-ddterm repository, in GJS-based signal definitions (param_types), it is acceptable to use JavaScript primitives like String and Boolean directly. Do not suggest replacing them with GObject.TYPE_STRING or GObject.TYPE_BOOLEAN. This guidance applies across JavaScript files in this project (not just this file) where signal parameter types rely on native JS constructors.
Applied to files:
ddterm/pref/shortcuts.jsddterm/pref/positionsize.jsddterm/shell/wm.jsddterm/app/application.jsddterm/pref/util.jsddterm/shell/geometry.js
📚 Learning: 2026-03-02T03:21:20.247Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1774
File: ddterm/app/appwindow.js:41-47
Timestamp: 2026-03-02T03:21:20.247Z
Learning: In GJS-based code, EXPLICIT_NOTIFY suppresses automatic notify only for properties with manually defined getters/setters. If a property has EXPLICIT_NOTIFY but no manually defined setter, GJS will generate a setter that calls notify() when the value changes. Apply this understanding to ddterm's JavaScript files to avoid assuming no notify emission for such properties; verify whether a manual setter exists before deciding if notify() should be emitted or suppressed.
Applied to files:
ddterm/pref/shortcuts.jsddterm/pref/positionsize.jsddterm/shell/wm.jsddterm/app/application.jsddterm/pref/util.jsddterm/shell/geometry.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: For the ddterm GNOME Shell extension, ensure all design decisions and code reviews assume GNOME Shell as the only supported environment. Do not introduce runtime or UI assumptions for other desktops; verify compatibility with GNOME Shell APIs and extension environment in all JavaScript files under the ddterm directory (e.g., ddterm/app/appwindow.js).
Applied to files:
ddterm/pref/shortcuts.jsddterm/pref/positionsize.jsddterm/shell/wm.jsddterm/app/application.jsddterm/pref/util.jsddterm/shell/geometry.js
📚 Learning: 2026-02-24T06:34:22.064Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1778
File: ddterm/app/terminal.js:332-338
Timestamp: 2026-02-24T06:34:22.064Z
Learning: In ddterm/app/terminal.js, when adding properties to TerminalBase that need to be bound from TerminalSettings (via PROPERTIES_CLONE in terminalsettings.js), the property should be declared as READWRITE | EXPLICIT_NOTIFY in TerminalBase, and the implementation logic should be inside TerminalBase itself (e.g., in _init()), not in external files like terminalpage.js. This ensures proper encapsulation and allows TerminalSettings to bind GSettings values to the terminal's properties.
<!-- [add_learning]
In the ddterm/gnome-shell-extension-ddterm repository, behavioral properties like copy-on-selection should be implemented within TerminalBase (ddterm/app/terminal.js) rather than in terminalpage.js. The terminal widget should own and encapsulate its own behavior by connecting to its signals in _init() and reacting to its own property changes.
Applied to files:
ddterm/shell/wm.jsschemas/com.github.amezin.ddterm.gschema.xmlddterm/shell/geometry.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: In the ddterm/app directory, treat code as GTK3-only and do not introduce GTK3/4 compatibility wrappers. For files under this directory (e.g., ddterm/app/tablabel.js), use direct imports from gi://Handy when needed; avoid adding compatibility shims. This pattern applies to all JavaScript files within ddterm/app.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: In ddterm/app/ files (e.g., ddterm/app/tablabel.js), do not specify versions in gi:// imports. Versioning is centralized in ddterm/app/init.js. Ensure all gi:// imports in this directory omit version specifications to maintain consistent, centralized version management.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-19T04:39:16.517Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/notebook.js:162-164
Timestamp: 2026-02-19T04:39:16.517Z
Learning: In GJS (GNOME JavaScript), when disconnecting C-level signal handlers from GTK/libhandy widgets with GObject.signal_handlers_disconnect_matched(), you can rely on matching by signalId alone for JavaScript handlers. Do not require func in this case; using only { signalId: 'signal-name' } will disconnect the corresponding handlers (e.g., disconnecting libhandy’s built-in key-press-event from Handy.TabView with { signalId: 'key-press-event' }). Apply this pattern in JavaScript files under ddterm/app/ to identify and safely detach signal handlers by signalId when appropriate, and verify the specific library behavior for each signal.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: In ddterm, require using set_visual() with an RGBA visual to enable transparency on the X11 backend (including XWayland and older GNOME Shell sessions). This guideline applies to files under ddterm/app (e.g., ddterm/app/appwindow.js). Ensure the visual is RGBA where transparency is needed and that this applies to X11/XWayland backends rather than GNOME Shell-only assumptions.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-03-03T07:34:39.235Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1788
File: ddterm/app/application.js:709-744
Timestamp: 2026-03-03T07:34:39.235Z
Learning: In ddterm/app/, prefer using underscore-prefixed names for methods intended to be overridden (e.g., _ensure_window) rather than private `#private` fields, when inheritance in dev-application.js or other subclasses is expected. Private class fields (#) cannot be overridden by subclasses, so use the underscore convention for 'protected' methods that need inheritance support. Apply this pattern to all JavaScript files under ddterm/app where subclassing or overrides are anticipated.
Applied to files:
ddterm/app/application.js
📚 Learning: 2026-01-07T04:34:03.958Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:34:03.958Z
Learning: The ddterm/pref/ directory needs GTK 3/4 compatibility wrappers because it can be loaded by GNOME Shell extensions service, which is GTK 4.
Applied to files:
schemas/com.github.amezin.ddterm.gschema.xml
📚 Learning: 2026-03-08T15:45:34.487Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1801
File: meson.build:166-180
Timestamp: 2026-03-08T15:45:34.487Z
Learning: In ddterm/gnome-shell-extension-ddterm (meson.build), the `typelibs` list passed to the `gjs-typelib-installer` subproject and the `require({...})` call in `ddterm/app/init.js` do not need a shared single source of truth. If a typelib is in `require()` but missing from the meson `typelibs` list, the app will fail to start and all CI tests will fail (immediately detectable). If a typelib is in the meson `typelibs` list but absent from `require()`, nothing bad happens — only a few unnecessary lines are added to the bundle. The asymmetric failure mode makes the duplication safe.
Applied to files:
schemas/com.github.amezin.ddterm.gschema.xml
📚 Learning: 2026-01-07T04:34:03.958Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:34:03.958Z
Learning: EntryRow is not implemented in libhandy. When needed, it must be imported from ddterm/pref/util.js which provides a custom implementation.
Applied to files:
ddterm/pref/util.js
🔇 Additional comments (15)
ddterm/shell/wm.js (2)
118-118: LGTM - consistent handler for secondary size.The
changed::window-size-secondarysignal is correctly routed to#disable_window_maximize_setting, mirroring the existingwindow-sizehandler. This ensures that changing either dimension via preferences or shortcuts will properly clear the maximize state when the target rect becomes smaller than the workarea.
582-592: LGTM - dual-axis persistence on grab end.The logic correctly computes separate primary and secondary ratios based on orientation:
- For horizontal orientation: primary = width/workarea.width, secondary = height/workarea.height
- For vertical orientation: primary = height/workarea.height, secondary = width/workarea.width
Both values are clamped to 1.0 before writing to settings.
ddterm/pref/shortcuts.js (1)
536-544: LGTM - secondary size shortcut rows.The new shortcut configuration rows for
shortcut-window-size-secondary-incandshortcut-window-size-secondary-decfollow the established pattern and are logically placed after the primary size shortcuts.ddterm/pref/positionsize.js (1)
10-10: LGTM - import cleanup.
ScaleRowimport removed since the newadd_scale_rowhelper inutil.jshandles row creation internally.ddterm/app/application.js (2)
350-360: LGTM - secondary size actions.The new
window-size-secondary-decandwindow-size-secondary-incactions correctly mirror the primary size actions:
decseeds the value to1.0 - SIZE_STEPwhen maximized, allowing shrink-from-maximizeincis guarded to only apply when not maximizedThis behavior is consistent with the existing
window-size-dec/incpattern.
430-431: LGTM - secondary size shortcut bindings.The shortcut-to-action mappings correctly wire the new schema keys to the corresponding application actions.
schemas/com.github.amezin.ddterm.gschema.xml (2)
141-144: LGTM - secondary size schema key.The
window-size-secondarykey correctly defaults to1.0for backward compatibility, ensuring existing installations retain their current window dimensions on upgrade.
392-398: LGTM - secondary size shortcut keys.The default shortcuts
<Ctrl><Alt>Down/Upfollow the same semantic pattern as the primary size shortcuts (<Ctrl>Down/Up) and don't conflict with existing bindings.ddterm/pref/util.js (2)
561-591: LGTM -create_scale_rowhelper.The helper follows established patterns in this file:
- Binds the settings key to the adjustment's
value- Uses
bind_writableto disable the row when the key is locked- Uses
Intl.NumberFormatfor locale-aware percent formatting
711-718: LGTM -add_scale_rowmethod.The method correctly delegates to
create_scale_rowand follows the same pattern asadd_switch_rowandadd_combo_text_row.ddterm/shell/geometry.js (5)
81-89: LGTM - secondary size property.The in-memory default of
1now matches the schema default of1.0, ensuring correct initial geometry beforebind_settings()runs.
136-136: LGTM - notify handler for secondary size.Correctly triggers
#update_target_rectwhen the secondary size property changes.
152-181: LGTM - dual-axis geometry calculation.The updated
get_target_rectcorrectly applies:
- LEFT/RIGHT positions: primary
sizecontrols width;size_secondarycontrols height with vertical centering- TOP/BOTTOM positions: primary
sizecontrols height;size_secondarycontrols width with horizontal centeringThe floor operations and monitor-scale snapping ensure pixel-perfect rendering.
189-189: LGTM - settings binding.
window-size-secondaryis correctly included in the settings binding list.
381-381: LGTM - passing secondary size to geometry calculation.The
window_size_secondaryproperty is correctly passed toget_target_rect.
Member
|
Now you need to fix existing tests. Why does a solution like this not work for you: #129 (comment)? |
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.