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

Customize initial size for quick terminal #2384

Open
lucymhdavies opened this issue Oct 4, 2024 · 19 comments · May be fixed by #3742 or #4716
Open

Customize initial size for quick terminal #2384

lucymhdavies opened this issue Oct 4, 2024 · 19 comments · May be fixed by #3742 or #4716
Labels
gui GUI or app issue regardless of platform (i.e. Swift, GTK) os/macos

Comments

@lucymhdavies
Copy link
Contributor

In macOS, it looks like we're setting the default sizes here:
https://github.com/ghostty-org/ghostty/blob/main/macos/Sources/Features/QuickTerminal/QuickTerminalPosition.swift#L10-L29

For example, when showing at the top of the screen, we're setting the height to be 1/4 the height of the screen.
(specifically, this is whichever screen quick terminal first loads on. there's some edge cases around quick-terminal-screen=mouse if you have monitors of varying sizes, which I'm not going to go into right now)

I couldn't figure out from https://github.com/ghostty-org/ghostty/pull/2320/files how this is implemented on other systems. Am I missing something, or is this a macOS-only feature for now?

There are two options we could go for here:

  • Allow the user to specify a multiplier in their config, which would default to 0.25
  • Allow the user to specify a fixed height

(plus the option of both)

In my case, I would want my multiplier to be 1. i.e. the quick terminal takes up the entire screen. For my specific example, I see no value in specifying a fixed height. My instinct is that most people who use quick terminal have a preference for it to fill "a bit of the screen", "a lot of the screen" or "all of the screen". I imagine pixel-precision sizing of the quick terminal would be a niche desire.
(I could of course be wildly wrong these assumptions)

So were I to implement this myself (and I'm thinking I probably will), my proposal is:

  • Add a new config item, e.g. quick-terminal-size-multiplier, with default value of 0.25
  • Update where we set the window size, for example height: screen.frame.height * multiplier

There are a few edge cases I'd have to consider (e.g. making sure the multiplier is > 0, and <= 1), but otherwise this feels like it would be a fairly simple thing to implement.

If you're happy with the proposed implementation, I might get a PR raised for it over the weekend.

@mitchellh mitchellh added the needs-confirmation A reproduction has been reported, but the bug hasn't been confirmed or reproduced by a maintainer. label Oct 4, 2024
@mitchellh
Copy link
Contributor

We have a precedent for supporting multiple units within a single config, so my preference would be something like the following:

  • quick-terminal-size single configuration
  • A value ending in % is a percentage (similar to adust-* configs) of the max
  • A value ending in px is a raw pixel value, clamped by max height/width
  • A value with no unit is a config error
  • A single value is for the dimension that is growable depending on position (for top/bottom its height, for left/right its width).
  • A value separated by a comma specifies the secondary dimension with the same format.
    • This doesn't have to be part of an initial PR.

Example valid values:

quick-terminal-size = 25%
quick-terminal-size = 42px
quick-terminal-size = 25%,75%
quick-terminal-size = 300px,80%

Thoughts?

@mitchellh
Copy link
Contributor

Am I missing something, or is this a macOS-only feature for now?

It is macOS only.

@vibe
Copy link
Collaborator

vibe commented Oct 5, 2024

We have a precedent for supporting multiple units within a single config, so my preference would be something like the following:

  • quick-terminal-size single configuration

  • A value ending in % is a percentage (similar to adust-* configs) of the max

  • A value ending in px is a raw pixel value, clamped by max height/width

  • A value with no unit is a config error

  • A single value is for the dimension that is growable depending on position (for top/bottom its height, for left/right its width).

  • A value separated by a comma specifies the secondary dimension with the same format.

    • This doesn't have to be part of an initial PR.

Example valid values:

quick-terminal-size = 25%
quick-terminal-size = 42px
quick-terminal-size = 25%,75%
quick-terminal-size = 300px,80%

Thoughts?

Sounds good to me.

This approach is simple enough to allow configuration on one dimension by default, while remaining flexible enough to support full customization of both axes.

My only feedback would be that mixing units feels a bit unintuitive but I’m not opposed to it at all, especially since we already have existing precedents for handling mixed units.

@mitchellh
Copy link
Contributor

My only feedback would be that mixing units feels a bit unintuitive but I’m not opposed to it at all, especially since we already have existing precedents for handling mixed units.

I agree, I'd hope people don't do it, but I don't see a reason to outright reject it.

@mitchellh
Copy link
Contributor

I should add: I imagine people will want different aligned quick terminals too. My proposal with quick-terminal-size is to always center align it vertically or horizontally depending on the position. We can discuss an alignment configuration if that is requested enough but I think we can ignore it for this proposal.

@mitchellh mitchellh removed the needs-confirmation A reproduction has been reported, but the bug hasn't been confirmed or reproduced by a maintainer. label Oct 5, 2024
@mitchellh
Copy link
Contributor

Marking this as accepted, if someone wants to PR it as proposed I will accept it. I'm still open to further feedback though.

@vibe
Copy link
Collaborator

vibe commented Oct 5, 2024

Marking this as accepted, if someone wants to PR it as proposed I will accept it. I'm still open to further feedback though.

Quick question: Should the parsing and validation of quick-terminal-size happen in zig or swift?

@mitchellh
Copy link
Contributor

It should go in Config.zig with a dedicated type. That type should have tests. You can see some of the other types as examples. I think there is a function you can implement to control how it gets converted to C for the API.

@lucymhdavies
Copy link
Contributor Author

Good call on quick-terminal-size = 25% or quick-terminal-size = 42px. I hadn't considered that, and it's certainly a more elegant solution.

And...

  • A value separated by a comma specifies the secondary dimension with the same format.
    • This doesn't have to be part of an initial PR.

Agreed on that part, I think unless it turns out to be super easy to implement multi-value, the initial implementation can be single value only, and then multi-value support later on.

As for mixing units, quick-terminal-size = 300px,80%
Yeah, it does feel kinda unintuitive, but it's also the kind of thing I can't rule out somebody wanting to do. If we're not implementing this part yet, it's something we can decide on later if/when we do support multi-value.

The only other thing I'd suggest as part of this... unless I missed something, the docs don't explicitly mention that this is a macOS only feature. So I'd also suggest updating the docs somewhere to make that part explicit.

@mitchellh
Copy link
Contributor

The only other thing I'd suggest as part of this... unless I missed something, the docs don't explicitly mention that this is a macOS only feature. So I'd also suggest updating the docs somewhere to make that part explicit.

I thought I put it for the toggle_quick_terminal binding action but I don't see it so I'll add that there.

@mitchellh mitchellh added the gui GUI or app issue regardless of platform (i.e. Swift, GTK) label Oct 6, 2024
@JakeWharton
Copy link

We talked about this briefly on Discord in the context of a maximum quick terminal width, but for height as well being able to specify the number of display rows would be nice.

@mitchellh
Copy link
Contributor

For the initial work I suggest just doing % and pixels. Rows/columns can come later since the API around that is messier.

@lucymhdavies
Copy link
Contributor Author

lucymhdavies commented Oct 10, 2024

Making a start on this, by defining that new dedicated QuickTerminalSize

The initial implementation is only 1 dimensional, but as we've said, we can make it 2 dimensional later if we want to.

I'm still super new to Zig, so I've spent a lot of time figuring out how to even get this working. I'm expecting a lot of this code is not at all idiomatic.
(and similarly I wouldn't be surprised if there were much simpler ways of achieving this)

d2cf51a

But the tests pass... so I think this part at least works. Unless this needs major changes, the remaining work is on the macOS side.
(Plus docs updates, of course)

I don't know when I'll have time to do any further work, so if anybody wants to pick up what I've got so far and run with it, please feel free!

@futurepaul
Copy link
Contributor

Building on d2cf51a, I added the frontend macOS stuff so I can hardcode my own use case (fullscreen). Unfortunately my naive attempts at using ghostty_config_get to actually read the config didn't work, so I'll need to learn more about what's actually happening in the zig->swift config parsing.

https://github.com/futurepaul/ghostty/commit/66afff7988eb32498fc2a9641b0d359cfdba9007

@lucymhdavies
Copy link
Contributor Author

Nice! As may have been obvious... I just didn't have the time to finish what I was doing on this 😂
Teamwork :D

So remaining tasks as I see it:

  • Zig - Docs
  • Zig - implement the formatEntry function
  • Swift - Get the zig->swift config parsing working

@futurepaul
Copy link
Contributor

🤝

After playing with my version more I realized it was going off the screen on the bottom.

https://github.com/futurepaul/ghostty/commit/8f70f8a2bd464a37df617c497362a7a26ba10610

Getting the height from screen.visibleFrame fixes it for my use case (100%, coming from top) but it seems like the left orientation is busted and I haven't tried the others (likely something to do with the animation in / out?), so I'll add:

  • fix left position when full height
  • Test that most reasonable sizes and orientations display correctly,

mitchellh added a commit that referenced this issue Dec 25, 2024
# Description

This commit introduces the ability to launch the quick terminal in the
middle position.

![Screen Recording 2024-12-24 at 14 59
46](https://github.com/user-attachments/assets/6c733caf-34c7-4e80-ba16-24b41f98d2f0)

## Note to reviewer
- The quick terminal is currently centered. Should we consider adding a
top offset to better align with the screenshot in [the issue](#2494 )?
Should it be configurable?
- On large monitors, half the visible frame might be excessively large.
To mitigate, I am planning to implement #2384 but we should probably
agree on a good default maximum width/height in middle position.
- I also figured out, reloading the configuration does not update the
quick terminal configuration. That is also an issue I will try to fix.

Resolves #2494

I agree to relicense my commit to MIT.
dmehala added a commit to dmehala/ghostty that referenced this issue Dec 28, 2024
This commit introduce `quick-terminal-size` option which allows to
define the size of the quick terminal.

It also fixes an issue where the quick terminal position was not
properly updated when reloading the configuration.

Resolves ghostty-org#2384
dmehala added a commit to dmehala/ghostty that referenced this issue Dec 28, 2024
This commit introduce `quick-terminal-size` option which allows to
define the size of the quick terminal.

It also fixes an issue where the quick terminal position was not
properly updated when reloading the configuration.

Resolves ghostty-org#2384
@dmehala dmehala linked a pull request Dec 28, 2024 that will close this issue
@donaldguy
Copy link

I see the PR is in flight, so maybe preaching to the choir but

I would strongly appreciate some ability to dial in quick-terminal width when in top b/c

I use a 49" ultrawide (LG 49WL95C-W) and the ergonomics for top are pretty bad here:

Image

(cf iTerm:

Image
)

and center is ... okay:

Image

(but top IS much better than center when I unplug and am on the 13" display of the MacBook air that regularly drives that display; right now I deal with switching, but since its a requires-restart config, its not great)

[if anyone is wondering, Safari was being weird about previewing the image uploads; that's why it gets replaced by arc between screenshot 1 and 2)

@donaldguy
Copy link

Alas, having chosen to rebase and build first, read Swift later, I discover that even with the 2 number forms of quick-terminal-size that #3742 currently still hard-sticks top and bottom to full-width, e.g. here:

func restrictFrameSize(_ size: NSSize, on screen: NSScreen) -> NSSize {
var finalSize = size
switch (self) {
case .top, .bottom:
finalSize.width = screen.frame.width

(and the other 2 versions)

I suppose the ~problem is that [either you could still default to center OR] you need an additional config here (or a 4 number format?) to specify not only per-se width but also offset from edge

@donaldguy donaldguy linked a pull request Jan 6, 2025 that will close this issue
@donaldguy
Copy link

donaldguy commented Jan 6, 2025

Umm, so ... I opened #4716 as a draft PR because it does contain a partial implementation that I probably plan to continue on.

(and I originally was just gonna make a gist of a patch or indeed a ```diff block in a comment on #3742, as I mostly wanted to give @dmehala et al a heads up I was working on an alternative. What would have been the rest of that comment more or less ended up as first comment on this draft: #4716 (comment) )

but then I also got a little carried away and wrote, in order, 3 more lengthy comments:

  • 1 one which perhaps would belong more in one or more issues: Mouse resizing behavior

  • 2 which almost certainly really should be a discussion?
    "Is this whole approach [to quick terminal (size)] wrong?":

    • (pt 1.) - contemplating whether "quick terminal size" is/ought-to-be handled in terms of a window, or rather more like a pane within a fullscreen (partially transparent) "window"
    • (pt 2.) - contemplating wether adding any more than the existing (pretty specific) quick-terminal-* configs makes sense.

I… don't really think those are in the right place but I also don't know that they could be moved at this point and make sense (but mitchell or any other project admin types, feel free to do so anyway if you disagree)

I also think I'm burned out on thinking about this for the day or at least the next several hours, so … I wanted to go ahead and point these out to potentially interested parties before I give computing a break.

Cheers 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gui GUI or app issue regardless of platform (i.e. Swift, GTK) os/macos
Projects
None yet
6 participants