- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.2k
 
gtk: add GSettings support for window state persistence #9451
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
base: main
Are you sure you want to change the base?
gtk: add GSettings support for window state persistence #9451
Conversation
| <schemalist> | ||
| <schema id="com.mitchellh.ghostty" path="/com/mitchellh/ghostty/"> | ||
| <key name="window-size" type="(uu)"> | ||
| <default>(0, 0)</default> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By setting the initial value to (0, 0) we allow the terminal to first lauch with the default size, on close we will replace the (0, 0) with the default size (110, 25) or the current size, if the user resized his terminal in this first launch
| --prefix /app | ||
| --search-prefix /app | ||
| --system $PWD/vendor/p | ||
| - glib-compile-schemas /app/share/glib-2.0/schemas | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For package managers this should be handled automatically, but the AppImage maintainer will probably need to run glib-compile-schemas when packaging.
        
          
                src/apprt/gtk/settings.zig
              
                Outdated
          
        
      | schema_id, | ||
| @intFromBool(false), | ||
| ) orelse { | ||
| log.info("GSettings schema '{s}' not installed, window state will not persist", .{schema_id}); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails gracefully if the GSettings schema is not present (ex: appimage maintainer forgot to compile), in the failure case Ghostty will just have its current behaviour of not restoring or saving its size!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall
| --prefix /app | ||
| --search-prefix /app | ||
| --system $PWD/vendor/p | ||
| - glib-compile-schemas /app/share/glib-2.0/schemas | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be something we can do within the Zig build system. Meson does it automatically, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good generally. Some questions that I plan on researching myself, but feel free to provide some color as well to help me:
- 
How does versioning work? In macOS, it's standard to put a "version" monotonic Int into the settings schema so that you can provide upgrade paths or ignore unknown versions. This also lets you change structure. Do we need that here? Seems like we would but I'm not sure if GSettings provides another way.
 - 
I think the compiled schema is something we'd want to put in our dist tarball and it doesn't look like we're doing it here.
 - 
Not required for this PR: but is there any way we can localize the description string? Asking since we already localize many other things in GTK.
 
          
 As far as I am aware GGSettings does not have a built-in, automated schema versioning mechanism with explicit version numbers in the schema files themselves, instead they are managed via some best practices to make or life easier: 
 In case of breaking changes: 
 eg: <!-- Old key - deprecated -->
<key name="window-size" type="(uu)" deprecated="true">
  <default>(0, 0)</default>
</key>
<!-- New key with different structure -->
<key name="window-state-v2" type="a{sv}">
  <default>{}</default>
</key>Kinda of a pain in the ass if you ask me, but its the gnome way :< 
 If making glib-compile-schemas a build-time dep is not a problem, I think we can do it, but package managers typically handle schema compilation automatically via post-install hooks and we are already shipping the schema source, so that can be an option too, if we keep it as it is we would probably just need to update in PACKAGING.md that package managers should run glib-compile-schemas post-install hook and we should be fine. I think its a common practice to do that. 
 
 
 eg: <?xml version="1.0" encoding="UTF-8"?>
<schemalist gettext-domain="com.mitchellh.ghostty">
  <schema id="com.mitchellh.ghostty" path="/com/mitchellh/ghostty/">
    <key name="window-size" type="(uu)">
      <default>(0, 0)</default>
      <_summary>Last window size</_summary>
      <_description>
        The last window size in terminal columns and rows. This is used to
        restore the window size when window-save-state is enabled. A value
        of (0, 0) means no size has been saved yet.
      </_description>
    </key>
  </schema>
</schemalist>
 Thinking better about it, it is not that simple because I will actually need to convert the xml to a template to fill in the schema-id via build time from the current build_config.bundle_id, it is possible but I will need to thinker with it a bit more to get to a viable solution :>  | 
    
          
 I managed to get translations working for the GSchemas, but it needed some changes in how mo files are generated, I made a draft PR at #9464 using the current PR as base, so it can be reviewed after we merge this one.  | 
    
Implement GSettings to save and restore the window size in terminal grid dimensions on Linux. Include the necessary schema and update the application to handle window state management.
My solution is similar to how ptyxis does it in gnome.