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

Implement resize by increment #2131

Merged
merged 2 commits into from
Jan 7, 2020
Merged

Conversation

aes
Copy link
Contributor

@aes aes commented Nov 13, 2019

This makes increments-based resize work for me (on X11).

(Let's discuss before merge, but it makes me a lot happier)

@aes aes force-pushed the resize-by-increments branch 2 times, most recently from 47e29a5 to e471c7c Compare November 13, 2019 21:32
@kovidgoyal
Copy link
Owner

cell width/cell height can change you have to account for that. And this need to be made optional, I for one hate resize by increments and it does not work well with say tiling window managers.

glfw/window.c Outdated

window->widthincr = widthincr;
window->heightincr = heightincr;
_glfwPlatformSetWindowSizeIncrements(window, window->numer, window->denom);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be

_glfwPlatformSetWindowSizeIncrements(window, window->widthincr, window->heightincr);

@aes
Copy link
Contributor Author

aes commented Nov 14, 2019 via email

@aes
Copy link
Contributor Author

aes commented Nov 14, 2019 via email

@kovidgoyal
Copy link
Owner

kovidgoyal commented Nov 14, 2019 via email

@@ -2034,6 +2042,14 @@ void _glfwPlatformSetWindowAspectRatio(_GLFWwindow* window, int numer UNUSED, in
XFlush(_glfw.x11.display);
}

void _glfwPlatformSetWindowSizeIncrements(_GLFWwindow* window, int numer UNUSED, int denom UNUSED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameters should be widthincr and heightincr.

@Luflosi
Copy link
Contributor

Luflosi commented Nov 15, 2019

What should happen when the window size is not a multiple of the cell width/height? This can happen when opening new OS windows or when changing the font size.

  1. Live with practically random margin sizes? This somewhat defeats the purpose of this feature IMO.
  2. Should the next resize align to the cell grid again so that the first resize "step" makes the margins go away/to the default again?
  3. Should the OS window be resized in the two situations above so that the size of the margins doesn't change?

Not sure what the best solution is.
At least on macOS (with the little fix above to make it work and a fix to change the resize increments when the font size is changed), option 1 is currently used. I think I would prefer option 2 but that seems a little difficult or complicated to implement.
In iTerm2, the window is resized when changing the font size, so this isn't really an issue there.

Here are my changes to your PR so far, feel free to use them if you like:

diff --git a/glfw/window.c b/glfw/window.c
index ec306846..20d9926a 100644
--- a/glfw/window.c
+++ b/glfw/window.c
@@ -703,7 +703,7 @@ GLFWAPI void glfwSetWindowSizeIncrements(GLFWwindow* handle, int widthincr, int
 
     window->widthincr  = widthincr;
     window->heightincr = heightincr;
-    _glfwPlatformSetWindowSizeIncrements(window, window->numer, window->denom);
+    _glfwPlatformSetWindowSizeIncrements(window, window->widthincr, window->heightincr);
 }
 
 GLFWAPI void glfwGetFramebufferSize(GLFWwindow* handle, int* width, int* height)
diff --git a/glfw/x11_window.c b/glfw/x11_window.c
index 87e66366..4e82a753 100644
--- a/glfw/x11_window.c
+++ b/glfw/x11_window.c
@@ -2042,7 +2042,7 @@ void _glfwPlatformSetWindowAspectRatio(_GLFWwindow* window, int numer UNUSED, in
     XFlush(_glfw.x11.display);
 }
 
-void _glfwPlatformSetWindowSizeIncrements(_GLFWwindow* window, int numer UNUSED, int denom UNUSED)
+void _glfwPlatformSetWindowSizeIncrements(_GLFWwindow* window, int widthincr UNUSED, int heightincr UNUSED)
 {
     int width, height;
     _glfwPlatformGetWindowSize(window, &width, &height);
diff --git a/kitty/state.c b/kitty/state.c
index 251dd7c8..aec104e3 100644
--- a/kitty/state.c
+++ b/kitty/state.c
@@ -780,6 +780,7 @@ PYWRAP1(os_window_font_size) {
                     resize_screen(os_window, w->render_data.screen, true);
                 }
             }
+            glfwSetWindowSizeIncrements(os_window->handle, os_window->fonts_data->cell_width, os_window->fonts_data->cell_height);
         }
         return Py_BuildValue("d", os_window->font_sz_in_pts);
     END_WITH_OS_WINDOW

@aes
Copy link
Contributor Author

aes commented Nov 18, 2019 via email

@aes
Copy link
Contributor Author

aes commented Nov 18, 2019

Hmm, on second thought, I think that's a good observation. The feature I want is a terminal of a given size, @kovidgoyal wants (if I understood correctly) pixel-based resizing (because of window manager behaviors). I can imagine there being other ideas, like if I (like you pointed out) change the font size, do I want to resize the window, or change the terminal size?

Are fixed terminal size vs. fixed window size the only sensible combinations, or are there others?

@Luflosi
Copy link
Contributor

Luflosi commented Nov 19, 2019

@kovidgoyal What are your thoughts on my comment #2131 (comment)?

@kovidgoyal
Copy link
Owner

With resize hints there is nothing kitty has to do on window resize. It's
upto the window manager to follow the hints or not, kitty does not care.

Then comes the font size change.

There are two options, let the window resize itself when font size
changes to preserve number of cells or let the margins and number of
cells adjust automatically and dont resize the window when the font size
changes. kitty does the latter, which I think is much nicer, but I have
no objections to an option implementing the former.

@kovidgoyal
Copy link
Owner

Oh and if you just want a window of fixed size use the initial_window_width/height options that allow you to specify a size on cells. And mark kitty windows non-resizable in the windowmanager.

@Luflosi
Copy link
Contributor

Luflosi commented Nov 19, 2019

Ok, I'd say we just stick to the current behaviour when changing the font size. We can implement any special resize behaviour in a different PR, this PR doesn't need to be blocked by that, right?
@aes would you like help with making this feature optional via a config setting?

@kovidgoyal
Copy link
Owner

This PR is fine, once the behavior is made optional.

@aes
Copy link
Contributor Author

aes commented Nov 22, 2019

@Luflosi Oh, thank you, that would be nice! I hope to get back to this this weekend, and start digging in to how the project is actually structured. (as opposed to just grepping)

@Luflosi
Copy link
Contributor

Luflosi commented Nov 22, 2019

I came up with the name resize_in_steps for the config option. Is that a good name? Can someone think of a better one?

@kovidgoyal
Copy link
Owner

Sounds ok to me.

@Luflosi
Copy link
Contributor

Luflosi commented Nov 24, 2019

This does not currently work on Wayland. The aspect ratio constraint is implemented by changing the window size after resizing. A similar hack would need to be implemented for this as well to make it work on Wayland. Should we instead just add a note to the option that this does not currently work on Wayland?

@aes
Copy link
Contributor Author

aes commented Nov 24, 2019 via email

@Luflosi
Copy link
Contributor

Luflosi commented Nov 24, 2019

There is actually special code in Wayland to make the aspect ratio stuff work:

kitty/glfw/wl_window.c

Lines 553 to 561 in c140e17

if (window->numer != GLFW_DONT_CARE && window->denom != GLFW_DONT_CARE)
{
aspectRatio = (float)width / (float)height;
targetRatio = (float)window->numer / (float)window->denom;
if (aspectRatio < targetRatio)
height = (int32_t)((float)width / targetRatio);
else if (aspectRatio > targetRatio)
width = (int32_t)((float)height * targetRatio);
}

@Luflosi
Copy link
Contributor

Luflosi commented Nov 24, 2019

But I can't be bothered trying to implement a similar hack for the resize in steps.

@Luflosi
Copy link
Contributor

Luflosi commented Nov 24, 2019

Feel free to rebase onto the latest master if you like.
I'll make a PR based on yours probably tomorrow making this configurable and updating the resize step size when changing the font size.

Luflosi added a commit to Luflosi/kitty that referenced this pull request Nov 24, 2019
@aes aes force-pushed the resize-by-increments branch 2 times, most recently from e925314 to 7be7afd Compare December 1, 2019 21:16
@aes
Copy link
Contributor Author

aes commented Dec 2, 2019

Is there anything else we need to do, or is this ready?

@Luflosi
Copy link
Contributor

Luflosi commented Dec 3, 2019

You accidentally added an extra newline in the changelog while rebasing my commit. You can try resetting by one commit and then merging again or let @kovidgoyal remove it while merging. Other than that nitpick, this should be ready to be merged.

@Luflosi
Copy link
Contributor

Luflosi commented Dec 8, 2019

@kovidgoyal alright, please review, I don't want to wait any longer. You should consider removing the extra line in the change log.

@kovidgoyal
Copy link
Owner

I will review and merge this when I have some time.

@Luflosi
Copy link
Contributor

Luflosi commented Dec 29, 2019

Could you give a very rough estimate on when you will have some time to review this please?

@kovidgoyal
Copy link
Owner

Sometime in the next couple of months.

@kovidgoyal kovidgoyal merged commit c28ff52 into kovidgoyal:master Jan 7, 2020
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.

None yet

3 participants