raboof/volare
6
22
Fork
You've already forked volare
2

Empty tabbed containers #35

Merged
raboof merged 24 commits from :edmonds/empty-tabbed-containers into main 2023年06月05日 23:24:03 +02:00
Collaborator
Copy link

This branch implements the empty tabbed container behavior described in #34. It inserts a "placeholder" container into an L_TABBED container when it would otherwise become empty, and removes the placeholder when a window is added to the "empty" L_TABBED container. There were a number of additional changes required to accommodate these placeholder containers, but overall the intrusiveness doesn't seem to be too bad. The behavior seems to be a lot closer to Notion now.

Things that are working:

  • A tabbed container can have all of its windows removed, but it will still exist and prevent the layout from being rearranged.
  • Closing an empty tabbed container.
  • Keybindings (focus right etc.) work to focus into or out of an empty tabbed container.
  • The keybindings in #5 work to move a window into an empty tabbed container, or out of a tabbed container so that an empty tab container is left behind.
  • Dragging a window into a tabbed container, by dropping it on any of the drag targets that appear over the destination container.
  • Dragging the last window out of a tabbed container into another, empty tabbed container.
  • Empty tabbed containers have a normal looking titlebar with a single tab and the titlebar text <empty>.
  • Empty tabbed containers cannot be dragged into other containers.
  • Mousing over an empty tabbed container focuses it, when focus-follows-mouse is enabled.
  • When the sole, focused window of a tabbed container is closed, the focus stays in the now-empty tabbed container.
  • If the last window in a tabbed container is not focused, closing it does not reset the focus.
  • If mouse_warping container is configured, the mouse cursor will consider warping to the newly focused child container after a split.
  • A container with a single window in it can be split into a container with the original window and an empty tabbed container.
This branch implements the empty tabbed container behavior described in #34. It inserts a "placeholder" container into an `L_TABBED` container when it would otherwise become empty, and removes the placeholder when a window is added to the "empty" `L_TABBED` container. There were a number of additional changes required to accommodate these placeholder containers, but overall the intrusiveness doesn't seem to be too bad. The behavior seems to be a lot closer to Notion now. Things that are working: - [x] A tabbed container can have all of its windows removed, but it will still exist and prevent the layout from being rearranged. - [x] Closing an empty tabbed container. - [x] Keybindings (`focus right` etc.) work to focus into or out of an empty tabbed container. - [x] The keybindings in #5 work to move a window into an empty tabbed container, or out of a tabbed container so that an empty tab container is left behind. - [x] Dragging a window into a tabbed container, by dropping it on any of the drag targets that appear over the destination container. - [x] Dragging the last window out of a tabbed container into another, empty tabbed container. - [x] Empty tabbed containers have a normal looking titlebar with a single tab and the titlebar text `<empty>`. - [x] Empty tabbed containers cannot be dragged into other containers. - [x] Mousing over an empty tabbed container focuses it, when focus-follows-mouse is enabled. - [x] When the sole, focused window of a tabbed container is closed, the focus stays in the now-empty tabbed container. - [x] If the last window in a tabbed container is **not** focused, closing it does **not** reset the focus. - [x] If `mouse_warping container` is configured, the mouse cursor will consider warping to the newly focused child container after a split. - [x] A container with a single window in it can be split into a container with the original window and an empty tabbed container.
edmonds force-pushed edmonds/empty-tabbed-containers from dd0ff5fd62 to 4d4611f806 2023年06月04日 07:44:46 +02:00 Compare
edmonds changed title from (削除) WIP: Empty tabbed containers (削除ここまで) to Empty tabbed containers 2023年06月04日 08:03:49 +02:00
Author
Collaborator
Copy link

Hi, @raboof! Please take a look when you get a chance. This seems to work well for me and implements the desired Notion-style empty tabbed containers behavior.

(I did force-push my branch. For some reason Codeberg seems to be showing the old commits in the Conversation view and the new commits in the Commits view.)

Hi, @raboof! Please take a look when you get a chance. This seems to work well for me and implements the desired Notion-style empty tabbed containers behavior. (I did force-push my branch. For some reason Codeberg seems to be showing the old commits in the Conversation view and the new commits in the Commits view.)

Looks awesome at first glance, and indeed less intrusive than I expected - test-driving it for a while!

Looks awesome at first glance, and indeed less intrusive than I expected - test-driving it for a while!

One thing I noticed is that closing an empty frame with Mod+X leaves things in a strange state:

Before:

$ swaymsg -t get_workspaces
Workspace 1 (focused)
 Output: eDP-1
 Layout: splith
 Representation: H[T[kitty] V[T[&lt;empty&gt;] T[&lt;empty&gt;]]]

Mod+X in the bottom-right container, then:

$ swaymsg -t get_workspaces
Workspace 1 (focused)
 Output: eDP-1
 Layout: splith
 Representation: H[T[kitty] V[T[&lt;empty&gt;] T[]]]

Now it is very possible that this isn't a regression in this PR, and that this already didn't work, but it was not so visible before when closing the last window in a container closed it.

One thing I noticed is that closing an empty frame with Mod+X leaves things in a strange state: Before: ``` $ swaymsg -t get_workspaces Workspace 1 (focused) Output: eDP-1 Layout: splith Representation: H[T[kitty] V[T[&lt;empty&gt;] T[&lt;empty&gt;]]] ``` `Mod+X` in the bottom-right container, then: ``` $ swaymsg -t get_workspaces Workspace 1 (focused) Output: eDP-1 Layout: splith Representation: H[T[kitty] V[T[&lt;empty&gt;] T[]]] ``` Now it is very possible that this isn't a regression in this PR, and that this already didn't work, but it was not so visible before when closing the last window in a container closed it.
Author
Collaborator
Copy link

What do you have Mod+X bound to?

What do you have Mod+X bound to?

What do you have Mod+X bound to?

Ha, I see I didn't even add it to the default config, probably because it doesn't work correctly yet also on main ;).

I have it bound to split none right now (though if we find out another approach is better that is of course totally fine).

How do you re-join split containers?

> What do you have Mod+X bound to? Ha, I see I didn't even add it to the default config, probably because it doesn't work correctly yet also on main ;). I have it bound to `split none` right now (though if we find out another approach is better that is of course totally fine). How do you re-join split containers?
Author
Collaborator
Copy link

I haven't investigated re-joining, or unsplitting containers yet.

I use $mod+c, from the default config, to close an empty tabbed container:

 # Kill focused window
 bindsym $mod+c kill

I think I replicated your workspace:

Workspace 3 (focused)
 Output: DP-1
 Layout: splith
 Representation: H[T[Alacritty] V[T[&lt;empty&gt;] T[&lt;empty&gt;]]]

And then pressed $mod+c with the bottom right container focused, and it became this:

Workspace 3 (focused)
 Output: DP-1
 Layout: splith
 Representation: H[T[Alacritty] T[&lt;empty&gt;]]

If you manage to get a T[] representation, that indicates an empty L_TABBED container without the placeholder container. It's hard to be sure that there aren't code paths that can result in this because it's easy for any code site to construct a new container and set the layout, or update the layout on an existing container.

I haven't investigated re-joining, or unsplitting containers yet. I use $mod+c, from the default config, to close an empty tabbed container: ``` # Kill focused window bindsym $mod+c kill ``` I think I replicated your workspace: ``` Workspace 3 (focused) Output: DP-1 Layout: splith Representation: H[T[Alacritty] V[T[&lt;empty&gt;] T[&lt;empty&gt;]]] ``` And then pressed $mod+c with the bottom right container focused, and it became this: ``` Workspace 3 (focused) Output: DP-1 Layout: splith Representation: H[T[Alacritty] T[&lt;empty&gt;]] ``` If you manage to get a `T[]` representation, that indicates an empty `L_TABBED` container without the placeholder container. It's hard to be sure that there aren't code paths that can result in this because it's easy for any code site to construct a new container and set the layout, or update the layout on an existing container.

I use $mod+c, from the default config, to close an empty tabbed container:

Ah gotcha that works great 👍

If you manage to get a T[] representation, that indicates an empty L_TABBED container without the placeholder container. It's hard to be sure that there aren't code paths that can result in this because it's easy for any code site to construct a new container and set the layout, or update the layout on an existing container.

Yeah we just have to look into split none and probably tweak something there - but since it's not part of the default config that can be independent of this PR and just be fixed separately under #19

> I use $mod+c, from the default config, to close an empty tabbed container: Ah gotcha that works great 👍 > If you manage to get a T[] representation, that indicates an empty L_TABBED container without the placeholder container. It's hard to be sure that there aren't code paths that can result in this because it's easy for any code site to construct a new container and set the layout, or update the layout on an existing container. Yeah we just have to look into `split none` and probably tweak something there - but since it's not part of the default config that can be independent of this PR and just be fixed separately under #19
@ -33,0 +36,4 @@
cont->is_placeholder = true;
cont->pending.layout = L_NONE;
cont->formatted_title = strdup("&lt;empty&gt;");
Owner
Copy link

Interestingly Pango doesn't seem to be processing these characters for me - does that mean this behavior is inconsistent between build configurations? I'm on pango 1.50.14

Interestingly Pango doesn't seem to be processing these characters for me - does that mean this behavior is inconsistent between build configurations? I'm on pango 1.50.14
Author
Collaborator
Copy link

Hm, I'm using pango 1.50.12 from Debian unstable. Are you seeing the literal string &lt;empty&gt; rendered to the titlebar texture or is it properly rendering it as <empty>? I know it doesn't show up correctly over the IPC socket.

Hm, I'm using pango 1.50.12 from Debian unstable. Are you seeing the literal string `&lt;empty&gt;` rendered to the titlebar texture or is it properly rendering it as `<empty>`? I know it doesn't show up correctly over the IPC socket.
Owner
Copy link

Yeah I actually see &lt;empty&gt; in the titlebar. Versions are:

Run-time dependency json-c found: YES 0.16
Run-time dependency libpcre2-8 found: YES 10.42
Run-time dependency wayland-server found: YES 1.22.0
Run-time dependency wayland-client found: YES 1.22.0
Run-time dependency wayland-cursor found: YES 1.22.0
Run-time dependency wayland-protocols found: YES 1.31
Run-time dependency wlroots found: YES 0.16.2
Run-time dependency xkbcommon found: YES 1.5.0
Run-time dependency cairo found: YES 1.16.0
Run-time dependency pango found: YES 1.50.14
Run-time dependency pangocairo found: YES 1.50.14
Run-time dependency gdk-pixbuf-2.0 found: YES 2.42.10
Run-time dependency pixman-1 found: YES 0.42.2
Run-time dependency glesv2 found: YES 1.6.0
Run-time dependency libevdev found: YES 1.13.1
Run-time dependency libinput found: YES 1.23.0
Run-time dependency xcb found: YES 1.14
Run-time dependency libdrm found: YES 2.4.115
Run-time dependency libudev found: YES 253
Did not find CMake 'cmake'
Found CMake: NO
Run-time dependency bash-completion found: NO (tried pkgconfig and cmake)
Run-time dependency fish found: NO (tried pkgconfig and cmake)
Library m found: YES
Library rt found: YES
Run-time dependency xcb-icccm found: YES 0.4.2
Run-time dependency threads found: YES
Run-time dependency libsystemd found: YES 253
Yeah I actually see `&lt;empty&gt;` in the titlebar. Versions are: ``` Run-time dependency json-c found: YES 0.16 Run-time dependency libpcre2-8 found: YES 10.42 Run-time dependency wayland-server found: YES 1.22.0 Run-time dependency wayland-client found: YES 1.22.0 Run-time dependency wayland-cursor found: YES 1.22.0 Run-time dependency wayland-protocols found: YES 1.31 Run-time dependency wlroots found: YES 0.16.2 Run-time dependency xkbcommon found: YES 1.5.0 Run-time dependency cairo found: YES 1.16.0 Run-time dependency pango found: YES 1.50.14 Run-time dependency pangocairo found: YES 1.50.14 Run-time dependency gdk-pixbuf-2.0 found: YES 2.42.10 Run-time dependency pixman-1 found: YES 0.42.2 Run-time dependency glesv2 found: YES 1.6.0 Run-time dependency libevdev found: YES 1.13.1 Run-time dependency libinput found: YES 1.23.0 Run-time dependency xcb found: YES 1.14 Run-time dependency libdrm found: YES 2.4.115 Run-time dependency libudev found: YES 253 Did not find CMake 'cmake' Found CMake: NO Run-time dependency bash-completion found: NO (tried pkgconfig and cmake) Run-time dependency fish found: NO (tried pkgconfig and cmake) Library m found: YES Library rt found: YES Run-time dependency xcb-icccm found: YES 0.4.2 Run-time dependency threads found: YES Run-time dependency libsystemd found: YES 253 ```
Author
Collaborator
Copy link

Yeah I actually see &lt;empty&gt; in the titlebar.

It looks like there is a pango_markup flag in struct sway_config that I would guess is controlling whether the title is getting escaped or not. It looks like that flag is controlled by whether there is a pango: prefix to the font argument in the config (see sway(5)).

What do you have font set to in your config (if anything)?

Mine is set to pango:Arimo Bold 9.

> Yeah I actually see `&lt;empty&gt;` in the titlebar. It looks like there is a `pango_markup` flag in `struct sway_config` that I would guess is controlling whether the title is getting escaped or not. It looks like that flag is controlled by whether there is a `pango:` prefix to the `font` argument in the config (see `sway(5)`). What do you have `font` set to in your config (if anything)? Mine is set to `pango:Arimo Bold 9`.
Author
Collaborator
Copy link

I was able to replicate this by leaving out the pango: prefix in the argument to font in the config and 218fb298a4 seems to fix it.

I was able to replicate this by leaving out the `pango:` prefix in the argument to `font` in the config and 218fb298a4aa78d5b51846901b59c0ed9dd2dcbb seems to fix it.
Owner
Copy link

It looks like there is a pango_markup flag in struct sway_config that I would guess is controlling whether the title is getting escaped or not. It looks like that flag is controlled by whether there is a pango: prefix to the font argument in the config (see sway(5)).

Ha, good detective work

What do you have font set to in your config (if anything)?

I don't have a font set

> It looks like there is a `pango_markup` flag in `struct sway_config` that I would guess is controlling whether the title is getting escaped or not. It looks like that flag is controlled by whether there is a `pango:` prefix to the `font` argument in the config (see `sway(5)`). Ha, good detective work > What do you have `font` set to in your config (if anything)? I don't have a `font` set
edmonds marked this conversation as resolved 
If `font pango:[...]` is specified in the config, which enables pango
markup in titlebars, the `formatted_title` of placeholder containers
needs to be escaped, otherwise pango error messages will be written to
the log.
Previously, if pango markup was not enabled, the `formatted_title`
wouldn't be unescaped and the literal string `&lt;empty&gt;` would be
rendered to the titlebar texture.
This commit sets the placeholder container's `formatted_title` to either
the XML-escaped or unescaped variant of the string "<empty>" depending
on whether pango markup is enabled.

I think this is a great improvement and in a state where be merged, and additional improvements can be done as separate PRs.

The commits in this PR are pretty fine-grained - I think that is fine and we can merge the history as-is. Do you agree?

I think this is a great improvement and in a state where be merged, and additional improvements can be done as separate PRs. The commits in this PR are pretty fine-grained - I think that is fine and we can merge the history as-is. Do you agree?
Author
Collaborator
Copy link

I think this is a great improvement and in a state where be merged, and additional improvements can be done as separate PRs.

The commits in this PR are pretty fine-grained - I think that is fine and we can merge the history as-is. Do you agree?

I'm not aware of any showstoppers and I really like the new behavior, too, so please go ahead and merge if you think it's ready.

I would recommend always merging with a merge commit even if the PR branch is up-to-date and can be merged as a fast-forward, just so that there is a record in the volare git history of everything that is merged in case we need to extract all the volare changes and rebase them onto a new upstream release branch (thinking of possibilities for #8).

> I think this is a great improvement and in a state where be merged, and additional improvements can be done as separate PRs. > > The commits in this PR are pretty fine-grained - I think that is fine and we can merge the history as-is. Do you agree? I'm not aware of any showstoppers and I really like the new behavior, too, so please go ahead and merge if you think it's ready. I would recommend always merging with a merge commit even if the PR branch is up-to-date and can be merged as a fast-forward, just so that there is a record in the volare git history of everything that is merged in case we need to extract all the volare changes and rebase them onto a new upstream release branch (thinking of possibilities for #8).
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
raboof/volare!35
Reference in a new issue
raboof/volare
No description provided.
Delete branch ":edmonds/empty-tabbed-containers"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?