-
Notifications
You must be signed in to change notification settings - Fork 535
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
feat: customize quick terminal size #3742
base: main
Are you sure you want to change the base?
feat: customize quick terminal size #3742
Conversation
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
/// Control the size of the quick terminal. | ||
/// | ||
/// The size can expressed in two units: | ||
/// * A value ending in `%` specifies a percentage of the screen size. | ||
/// * A value ending in `px` specifies a fixed size in pixel, which is clamped by the | ||
/// screen's maximum width or height. | ||
/// | ||
/// The configuration accept one or two dimensions: | ||
/// * A single value specifies the dimension that is growable based on the quick terminal | ||
/// position: | ||
/// * For `top` and `bottom` positions, the value applies to the height. | ||
/// * For `right` and `left` positions, the value applies to the width. | ||
/// * For `center`, the size applied to both the width and height. | ||
/// * Two comma separated value specifies the width and height. | ||
/// | ||
/// Examples: | ||
/// | ||
/// ``` | ||
/// quick-terminal-size = 25% // 25% of the maximum size for the growable dimension | ||
/// quick-terminal-size = 42px // 42 pixels for the growable dimension | ||
/// quick-terminal-size = 25%,75% // 25% for the primary dimension, 75% for the secondary | ||
/// quick-terminal-size = 300px,80% // 300px for the primary dimension, 80% for the secondary | ||
/// ``` |
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.
I am not a native English speaker, so feel free to change the wording 😅.
func apply(value: CGFloat) -> CGFloat { | ||
switch(self) { | ||
case .pixel(let fixed_size): | ||
return CGFloat(fixed_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.
return CGFloat(fixed_size); | |
return min(value, CGFloat(fixed_size)); |
}; | ||
} | ||
|
||
fn parseValue(input: []const u8) !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.
[Nitpick] Parsing could be improve, quick-terminal-size: 20%, 30%
will probably fail due to the space after the comma. I could/should fix it, but other config suffers from the same issue and should probably be fixed in a subsequent PR.
init?(c_dimension: ghostty_config_quick_terminal_dimension_s) { | ||
switch(c_dimension.unit) { | ||
case GHOSTTY_QUICK_TERMINAL_PIXEL_UNIT: | ||
self = .pixel(value: UInt(c_dimension.value)) |
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.
[TODO] Use screen scale (IIRC retina has a scale of 2-3 while standard display is 1. backingScaleFactor
?
Hi I've been trying this out, thank you for working on this.
only the width is modified using two values works as expected
|
Hey @Jaycedam
This is the expected behaviour, howver, I agree for NOTE: I have an ultrawide monitor.
The PR doesn't include any changes related to global keybindings, so that would be a surprising side effect. Unfortunately, I am not able to reproduce the issue. Could you share the steps to reproduce it with you configuration? Thank you. |
I was confused by the wording of the documentation added here, but as long as it reflects the usage I think it will be easy to understand.
The global keybind was a problem on my end so ignore it. Re-enabling the accesibility toggle fixed it. |
I know I'm a little late to the party but I just started using ghostty today. Shouldn't we also allow to specify the size of the quick terminal in rows and cols? So
would create a window that has 120 columns and 50 rows. |
This was mentioned as a future enhancement due to it being more complex #2384 (comment) Also thank you dmehala for working on this. I always use a fullscreen hotkey toggle for my terminal needs |
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.
locally rebased this onto tip (at 8f5f432)
there were some detected-conflicts in QuickTerminalController.swift
(vs #4055 )
they are all fairly obvious resolutions but to (potentially) save you the trouble (modulo if you have strong feelings about a different order)
[see below for one ~substantive question]
@@ -390,25 +393,32 @@ class QuickTerminalController: BaseTerminalController { | |||
|
|||
// Update our derived config | |||
self.derivedConfig = DerivedConfig(config) | |||
|
|||
self.position = self.derivedConfig.quickTerminalPosition | |||
|
|||
syncAppearance(config) |
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.
syncAppearance(config) | |
syncAppearance() |
let quickTerminalPosition: QuickTerminalPosition | ||
let quickTerminalSize: QuickTerminalSize |
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.
let quickTerminalPosition: QuickTerminalPosition | |
let quickTerminalSize: QuickTerminalSize | |
let quickTerminalPosition: QuickTerminalPosition | |
let quickTerminalSize: QuickTerminalSize | |
let windowColorspace: String | |
let backgroundOpacity: Double |
or your preferred order
self.quickTerminalPosition = config.quickTerminalPosition | ||
self.quickTerminalSize = config.quickTerminalSize |
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.
self.quickTerminalPosition = config.quickTerminalPosition | |
self.quickTerminalSize = config.quickTerminalSize | |
self.quickTerminalPosition = config.quickTerminalPosition | |
self.quickTerminalSize = config.quickTerminalSize | |
self.windowColorspace = config.windowColorspace | |
self.backgroundOpacity = config.backgroundOpacity |
self.quickTerminalPosition = .top | ||
self.quickTerminalSize = .init() |
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.
self.quickTerminalPosition = .top | |
self.quickTerminalSize = .init() | |
self.quickTerminalPosition = .top | |
self.quickTerminalSize = .init() | |
self.windowColorspace = "" | |
self.backgroundOpacity = 1.0 |
|
||
// Update the quick terminal size right away | ||
config.quickTerminalSize.apply(window, config.quickTerminalPosition) |
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.
FYI, since you removed the function param this config
symbol no longer resolves and tanks the swift compilation.
not sure if the invocation up at 75 (72 in my rebased version) is adequate or if this wants to stay but go:
// Update the quick terminal size right away | |
config.quickTerminalSize.apply(window, config.quickTerminalPosition) | |
// Update the quick terminal size right away | |
self.derivedConfig.quickTerminalSize.apply(window, self.position) |
or something else
Description
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 #2384