ddterm / gnome-shell-extension-ddterm

Another drop down terminal extension for GNOME Shell. With tabs. Works on Wayland natively
https://extensions.gnome.org/extension/3780/ddterm/
GNU General Public License v3.0
289 stars 26 forks source link

Allow configuring window size (width/height) #889

Open martinn opened 2 months ago

martinn commented 2 months ago

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: https://github.com/ddterm/gnome-shell-extension-ddterm/issues/129

amezin commented 2 months ago

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 commented 2 months ago

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

martinn commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

martinn commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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": https://github.com/ddterm/gnome-shell-extension-ddterm/blob/02c962ed213c93976db0d276ef32268c22581e98/ddterm/shell/geometry.js#L167

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

amezin commented 5 days ago

@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.