Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuring window size (width/height) #889

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

martinn
Copy link

@martinn martinn commented Apr 15, 2024

First of all, thanks for a great extension!

This PR adds the ability to configure both horizontal and vertical window size (previously, only one was configurable - depending on your window orientation). It can be very useful for people with large/ultrawide monitors.

For now, the window position is always centered, but that could be made configurable in a future iteration, if desired.

Screenshot from 2024-04-15 12-17-07
Screenshot from 2024-04-15 12-21-06
Screenshot from 2024-04-15 12-21-36
Screenshot from 2024-04-15 12-22-21

I still need to dive into the automated testing setup and add/update tests for this change, but wanted to get some thoughts/feedback to see if happy with the general approach before I do that.

I don't really know a whole lot about gnome shell extension development so please just let me know if any issues at all.

Thanks!

Resolves: #129

This can be useful for large or ultrawide monitors.

For now, the window position is always centered, but that could be made
configurable in a future iteration, if desired.
@martinn martinn force-pushed the feat/configurable-terminal-width branch from d790bde to 20b5e4a Compare April 15, 2024 04:46
@amezin
Copy link
Member

amezin commented Apr 15, 2024

Instead of "horizontal"/"vertical size" settings I'd prefer "primary" and "secondary size" or something like that. When I change window position from top to right, and the width was 100% - I expect the height to become 100%, and the width to be what the height was before. I.e. they should be swapped when switching from "horizontal" to "vertical" position.

It doesn't have to be "primary" and "secondary" in the preferences UI though, bindings can be changed dynamically.

@amezin
Copy link
Member

amezin commented Apr 15, 2024

Does this work when both width and height are set to 90%?

@martinn
Copy link
Author

martinn commented Apr 15, 2024

Instead of "horizontal"/"vertical size" settings I'd prefer "primary" and "secondary size" or something like that. When I change window position from top to right, and the width was 100% - I expect the height to become 100%, and the width to be what the height was before. I.e. they should be swapped when switching from "horizontal" to "vertical" position.

It doesn't have to be "primary" and "secondary" in the preferences UI though, bindings can be changed dynamically.

That's a great idea. I can look into swapping the values when changing position from top/bottom to left/right so it behaves that way. I can still leave it as "horizontal/vertical size" in the prefs UI as I think that wording makes more sense from a user perspective. What do you think?

Does this work when both width and height are set to 90%?

It should. I just tried it and seems to work fine on my end, though I do get a second resizing happening for some reason (i.e. first it tries to animate to almost 100% size, then it resizes back to 90%). I also see some odd behavior where if you set size to 100%, it ends up resizing to less than that. I'll look into it further.

@amezin
Copy link
Member

amezin commented Apr 15, 2024

It should. I just tried it and seems to work fine on my end, though I do get a second resizing happening for some reason (i.e. first it tries to animate to almost 100% size, then it resizes back to 90%). I also see some odd behavior where if you set size to 100%, it ends up resizing to less than that. I'll look into it further.

That's what stopped me from implementing this feature previously, because I was unable to find a working solution for this problem.

@martinn
Copy link
Author

martinn commented Apr 15, 2024

It should. I just tried it and seems to work fine on my end, though I do get a second resizing happening for some reason (i.e. first it tries to animate to almost 100% size, then it resizes back to 90%). I also see some odd behavior where if you set size to 100%, it ends up resizing to less than that. I'll look into it further.

That's what stopped me from implementing this feature previously, because I was unable to find a working solution for this problem.

Do you mean the first part (issues at 90%), the second part (issues at 100%) or both?

I'll have a look later to see if anything I can find. I also saw some parts of the codebase that talk about maximizing the window in some aspect. If you've got time, are you able to share a bit more about how that works?

@amezin
Copy link
Member

amezin commented Apr 16, 2024

Do you mean the first part (issues at 90%), the second part (issues at 100%) or both?

Honestly, I can't recall all the details. But it was all around (un)maximization.

I also saw some parts of the codebase that talk about maximizing the window in some aspect. If you've got time, are you able to share a bit more about how that works?

First of all, GNOME's window manager - mutter - has its own internal "smart" logic about maximization, about window placement, etc. For example, if a window covers >=80% of the screen (excluding the panel, so-called "workarea"), the window manager maximizes the window automatically. Also, it sometimes moves the window - either to a different location on the same monitor, or even to a different monitor. So the extension tries to cancel all that - unmaximizes the window if it shouldn't be maximized, tracks the window geometry and "fixes" it if/when necessary. To make things worse, mutter has some issues with synchronization on Wayland https://gitlab.gnome.org/GNOME/mutter/-/issues/1627 - so it ignores some move/resize requests when it shouldn't

@martinn martinn changed the title Allow configuring horizontal/vertical window size Allow configuring window size (width/height) Apr 17, 2024
Purely focusing on getting existing tests to pass by assuming that the
secondary size will always be 100% (which is how it used to work).
@martinn martinn force-pushed the feat/configurable-terminal-width branch from 5666b29 to 62df5a5 Compare April 17, 2024 02:13
@martinn
Copy link
Author

martinn commented Apr 17, 2024

Thank you!

I've pushed an update with a few changes:

  1. Changed prefs dialog from "Window horizontal/vertical size" to "Window width/height". For now this is purely in the dialog while still calling it hsize and vsize in code which is closer to what it used to be.
  2. Swap the window width/height values if changing from top/bottom to left/right and viceversa. Unfortunately the UI does not show the updated values and I'm not sure how to make it refresh (any tips here?)
  3. I think I've fixed or at least improved the resizing issues at 90%/100% though did not test extensively.
  4. Trying to make existing tests pass by simulating secondary size to always be 100% (like it works without this PR) for now. Though there's still more work to do here.

@amezin
Copy link
Member

amezin commented Apr 17, 2024

It would be a bit easier if you kept window-size setting as is, i.e. to always be the "primary size". Because you wouldn't need to modify tests in this case.

Maybe disabling Meta.Preference.AUTO_MAXIMIZE temporarily could help fighting the unnecessary maximization.

But, honestly, I doubt this entire feature is feasible with current state of Mutter.

@amezin
Copy link
Member

amezin commented Apr 17, 2024

BTW, if you have any idea how to make test code less awful, even a bit, please let me know

@martinn
Copy link
Author

martinn commented Apr 17, 2024

It would be a bit easier if you kept window-size setting as is, i.e. to always be the "primary size". Because you wouldn't need to modify tests in this case.

I was trying to do this from a test perspective (i.e. For tests to setup primary size to whatever used to be the window size, and for the secondary size to default to being 100%), but I don't think I've done it successfully yet and I need to dive more into the testing setup to understand it more. I guess I could move this logic to the extension instead but that doesn't feel like the best approach to me as of yet.

BTW, if you have any idea how to make test code less awful, even a bit, please let me know

To be honest, I haven't had a chance to properly familiarize myself with it. At first glance it does seem to be a bit complex, but I'm also really impressed by all of what it does.

@martinn
Copy link
Author

martinn commented Apr 17, 2024

By the way, if you have time and can provide any pointers or tips to get me in the right direction on the below issue, that would be really useful to me.

Swap the window width/height values if changing from top/bottom to left/right and viceversa. Unfortunately the UI does not show the updated values and I'm not sure how to make it refresh.

I tried to have a quick look at how it's done for resizing, as manually resizing the window seems to update the prefs UI instantly. But there's probably some things about how GJS works that I'm missing to understand how this needs to happen.

@amezin
Copy link
Member

amezin commented Apr 19, 2024

I guess I could move this logic to the extension

Which logic exactly? The extension already chooses to apply window-size to either width or height depending on the position.

Swap the window width/height values if changing from top/bottom to left/right and viceversa. Unfortunately the UI does not show the updated values and I'm not sure how to make it refresh (any tips here?)

Because you're not changing the settings. All WindowGeometry settings bindings are "read-only":

settings.bind(key, this, key, Gio.SettingsBindFlags.GET);

And I'd prefer them to stay read-only. This class was meant to transform settings to window geometry only in one direction.

@amezin
Copy link
Member

amezin commented Jul 4, 2024

@martinn Sorry, have you given up? (To be honest, I tried to solve this issue once, and given up)

BTW, if tests are failing, but in real use things are working as expected - that might be a mistake in the tests.

@martinn
Copy link
Author

martinn commented Jul 14, 2024

@martinn Sorry, have you given up? (To be honest, I tried to solve this issue once, and given up)

BTW, if tests are failing, but in real use things are working as expected - that might be a mistake in the tests.

From memory, I never got around to finish updating the tests to work with the new functionality. But the functionality has been working fine for my personal use case. It's been very busy at the moment so I never got around to working through those tests unfortunately.

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.

Feature request: Allow the width of the terminal to be configurable. (Useful for large/ultrawide monitors)
2 participants