gtk: use AdwAlertDialog for close dialogs, fix incorrect close dialogs #5741
Conversation
pluiedev
force-pushed
the
push-mslmmoznqtvo
branch
from
e92f30a to
3d69f24
Compare
There was a problem hiding this comment.
I decided to take a look at what GNOME Console does in these situations:
Maybe we could align our wording a little more closely with what they do, except without the run-on sentence 😄. One quick suggestion is to remove the this.
What do you think about waiting to merge this PR until after we remove vanilla GTK and add zig-gobject?
|
Maybe the second commit in this series should be its own PR? It doesn't seem related to GTK other than implementing the action which we could then do in a subsequent PR. |
|
Could you also provide some screenshots so we can see everything put together? |
|
We've already added zig-gobject, and honestly IMO this can be the first step to removing non-Adwaita builds — an advantage of zig-gobject is that because the bindings are all statically generated extern functions, we don't actually need to compile with |
|
I agree that removing "this" looks better xD
|
pluiedev
force-pushed
the
push-mslmmoznqtvo
branch
from
3d69f24 to
bed620a
Compare
|
What's the difference between "close window" and "quit ghostty"? Also what are your thoughts on |
|
Close window closes one window, quit Ghostty quits all windows. I think Close and Cancel would probably look better, but the current labels are just for the sake of being the same as vanilla GTK, which used Yes/No buttons. EDIT: The GNOME HIG suggests using Cancel/Close. Will follow that then. |
pluiedev
force-pushed
the
push-mslmmoznqtvo
branch
from
bed620a to
162ba06
Compare
Loving the prose updates! I also like the idea of the |
| @@ -4242,7 +4242,11 @@ pub fn performBindingAction(self: *Surface, action: input.Binding.Action) !bool | |||
|
|
|||
| .close_surface => self.close(), | |||
|
|
|||
| .close_window => self.app.closeSurface(self), | |||
| .close_window => return try self.rt_app.performAction( | |||
There was a problem hiding this comment.
Ah! I remember the history here. Close window was implemented for macOS only for a time because its a common macOS-ism. The handling is all done in the apprt and not here, so I just put whatever here since nothing ever triggered it. That is, until GTK added a menu item to do so. 😄
src/apprt/gtk/Window.zig
Outdated
| return switch (self) { | ||
| .window => "Close this Window?", | ||
| .tab => "Close this Tab?", | ||
| .surface => "Close this Surface?", |
There was a problem hiding this comment.
Recommend changing this to "Terminal" (the copy, not the enum). It will be more instantly recognizable to users I think than surface...
There was a problem hiding this comment.
"Terminal" is kind of a confusing term since it can refer to the entire terminal... That's part of the terminology problem I've been bringing up in discussions lately — we need a better term than "split" or "terminal" since the former isn't exactly accurate (a surface directly under a tab wouldn't be considered as a "split") and the latter is too vague. Maybe we should lean into iTerm2's terminology of panes and such?
There was a problem hiding this comment.
I agree that with Mitchell that surface probably shouldn't be surfaced to users. I'm not sure terminal is the right word to use, but I like panel over surface.
pluiedev
force-pushed
the
push-mslmmoznqtvo
branch
4 times, most recently
from
69fbec1 to
948d019
Compare
For *some* reason we have a binding for close_window but it merely closes the surface and not the entire window. That is not only misleading but also just wrong. Now we make a separate apprt action for close_window that would make it show a close confirmation prompt identical to as if the user had clicked the (X) button on the window titlebar.
pluiedev
force-pushed
the
push-mslmmoznqtvo
branch
from
948d019 to
7d74b0c
Compare
|
|
||
| pub fn closeWithConfirmation(self: *Window) void { |
There was a problem hiding this comment.
I don't love the name of this function given the next comment. What about just close with documentation in a function level comment?
|
@pluiedev if you can resolve the conflicts and take a look at the open comments I'd like to review this one next. |
AdwAlertDialog is the recommended way to do alert/message dialogs starting from libadwaita 1.5, and is much easier to manage than GtkMessageDialog. (The latter is also deprecated since GTK 4.10, but this PR does not migrate it to use GtkAlertDialog, mostly because of its obtuse interface and that we'll remove the GtkMessageDialog code anyway in 1.2 when we remove non-Adwaita builds.)
We also had two bugs where tabs with only one split would display the "close surface" confirmation dialog, and windows would do the same when closed via the "Close Window" menu item or by the
close_windowkeybind action. (The "close window" dialog only appears when the user clicks on the close button on the titlebar.) Initially I was very confused by this, but it turns out that we don't have any apprt action related to closing a window, and it was simply closing surfaces...