Skip to content

window.py: Fix window group toggle keybind - #883

Merged
mattrose merged 1 commit into
gnome-terminator:masterfrom
rcalixte:fix_groupwindow
Feb 11, 2024
Merged

mattrose merged 1 commit into
gnome-terminator:masterfrom
rcalixte:fix_groupwindow

Conversation

@rcalixte

@rcalixte rcalixte commented Feb 6, 2024

Copy link
Copy Markdown
Contributor

The current conditional always evaluates to False, resulting in the ungroup_win() function never being called

@rcalixte
rcalixte force-pushed the fix_groupwindow branch 3 times, most recently from 8676386 to 1ad6c89 Compare February 9, 2024 19:23
@mattrose

mattrose commented Feb 9, 2024

Copy link
Copy Markdown
Member

Looking at the commit, that will ungroup the window if the window is any string, but it specifically has to be 'Window'. Unless there's something I'm missing.

@rcalixte

rcalixte commented Feb 9, 2024 •

Copy link
Copy Markdown
Contributor Author

Looking at the commit, that will ungroup the window if the window is any string, but it specifically has to be 'Window'. Unless there's something I'm missing.

That's what I'd done originally but if you create a new group using the menu, it defaults to a letter from the Greek alphabet. It also would miss the condition of custom window group titles. My understanding is that the intended function is to unset a group if one is currently set when the toggle action is triggered.

If I have groups currently set to some miscellaneous combination, I expect that a toggle will ungroup them, not create a new group. That would require two "toggles" to ungroup a custom grouping.

@mattrose

mattrose commented Feb 9, 2024

Copy link
Copy Markdown
Member

Hmm, I will poke, gimme a few minutes

@mattrose

mattrose commented Feb 9, 2024 •

Copy link
Copy Markdown
Member

The "Group All in Window" and "Ungroup All in Window" works as I would expect it. When Selected, it creates a group named "Window Group x" where x is a number starting from 1.

When you select "New Group" It creates a new group named after a greek letter, and then you need to add all the rest of the terminals in that window to the group by hand. Unfortunately, Terminator doesn't keep track of this as a Window group, and so the "Ungroup all in Window" doesn't behave as you expect.

If you want to use "Ungroup All in Window, you first need to create a Window Group.

I hope I'm explaining this well enough and it makes sense to you.

@rcalixte

rcalixte commented Feb 9, 2024

Copy link
Copy Markdown
Contributor Author

Adding a screenshot so we can start from the same page. 😃

001

@rcalixte

rcalixte commented Feb 9, 2024

Copy link
Copy Markdown
Contributor Author

Are you saying that the two left terminals here are not considered a window group? If I hit the keybinding to toggle the window grouping, I would expect it to ungroup these windows, not create "Window group 2" for all the terminals. The same with the grouped terminals on the right.

@rcalixte

rcalixte commented Feb 9, 2024

Copy link
Copy Markdown
Contributor Author

Unfortunately, Terminator doesn't keep track of this as a Window group, and so the "Ungroup all in Window" doesn't behave as you expect.

I just tested this using the screenshot above as a base and it does indeed work as I expect. It ungrouped all of the terminals in the window, both "Lambda" and "Window group 1" in this instance.

@mattrose

mattrose commented Feb 9, 2024

Copy link
Copy Markdown
Member

Are you hitting the group_win_toggle keybind, or are you going up to the "Ungroup All in Window" menu item in the titlebar menu.

They ... do different things apparently

@rcalixte

rcalixte commented Feb 9, 2024

Copy link
Copy Markdown
Contributor Author

Are you hitting the group_win_toggle keybind, or are you going up to the "Ungroup All in Window" menu item in the titlebar menu.

I hit the menu item just now to make sure that it worked as expected. It did. The group_win_toggle keybind as it currently is would instead have created a new group named "Window group 2" if I recall correctly. (I can set it up the same way and then verify that.)

They ... do different things apparently

That's what this PR is trying to fix. 😉

The current conditional always evaluates to False, resulting in the
ungroup_win() function never being called
@mattrose

Copy link
Copy Markdown
Member

Thanks for working through this with me on gitter.im, @rcalixte

@mattrose
mattrose merged commit d9aaa54 into gnome-terminator:master Feb 11, 2024
@rcalixte
rcalixte deleted the fix_groupwindow branch February 11, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants