Repository navigation
Conversation
ec814a2 to
21d4671
Compare
|
I like the idea of adding the option to center lone windows, but I have some concerns about the implementation. The PR changes nearly 16 files, which makes it unnecessarily difficult to review and increases the risk of merge conflicts or unintended regressions. AI-generated code is acceptable, but the implementation should remain minimal and consistent with the existing codebase. Please remove unnecessary comments where the code is self-explanatory and consider whether the feature can be implemented with a smaller, more focused diff. I'd prefer a simpler implementation that's easier to review, maintain, and merge safely. |
|
Use the following as your System Prompt for a Coding Assistant to ensure it follows best practices, understands the existing codebase, avoids unnecessary changes, and delivers clean, reliable, and maintainable code: |
|
Sure, thank you for the feedback. Let me see what I can do to refine this a bit. The two biggest things that bloat the size of this feature are the window/display exception settings. I think they are both important to making the feature usable but fortunately I think there are some opportunities to simplify a bit (with a few tradeoffs). I'll also go ahead and move the bugfix to its own PR. It was exposed when I was testing but isn't essential. |
Problem:
Many applications and websites are not designed to handle ultrawide apsect ratios gracefully.
This problem is most apparent when auto-tiling is enabled because it sometimes means that the useful elements are at the extreme periphery with a large void in the center of the screen.
Here you can see how bad the problem is for GNOME Files, with around 90% of the space being wasted, right in the center of the screen where the user is likely to be looking! On large ultrawide displays this means you're often forced to physically turn your head back and forth just to interact with a single application.
You can work around this problem somewhat by either disabling auto-tiling or by opening a second window and using it to resize the first but both options are a bit cumbersome.
Solution:
This PR adds a new option to the auto-tiling settings to center lone windows and limit their width either by a percentage of the total screen size or by an exact pixel count.
With this setting enabled lone windows now sit comfortably in the users field of view on ultrawide displays. Tiling works normally as new windows are added to the workspace and when windows are closed the last remaining window moves back to the center of the screen and resizes based on the configured settings.
o-tiling-feature-screencast.mp4
This setting allows for manually resizing lone windows and keeps them centered within the workspace. This also introduces per-window exceptions (similar to the floating window exceptions) and per-display exceptions (to handle scenarios where connected displays may have different aspect ratios).
Additional considerations:
Minor fix
This PR also includes a fix for a bug in
window/movement.tswhich was comparing window width against window height when detecting an unchanged resize (instead of comparing the two height values).I fixed this while adding lone-window resize tracking so the resize now bails cleanly when a manual drag changes nothing.
Per-window exceptions
I added the per-window exceptions by mostly duplicating the code for the floating window exceptions. It may be worth a follow-up to refactor this to avoid the duplicated code. I'm not sure if new features may also leverage the same style of window picker in the future but for now I opted to keep it separate to avoid a large refactor in the PR.
It's also worth considering that I placed the new exceptions list in settings rather than the extension to avoid clutter. It requires some back-and-forth between the two apps but settings felt like the better home for a more niche feature.