Skip to content

window.py: Fix window group toggle keybind#883

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

window.py: Fix window group toggle keybind#883
mattrose merged 1 commit intognome-terminator:masterfrom
rcalixte:fix_groupwindow

Conversation

@rcalixte
Copy link
Copy Markdown
Contributor

@rcalixte rcalixte commented Feb 6, 2024

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
Copy link
Copy Markdown
Member

mattrose commented Feb 9, 2024

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
Copy link
Copy Markdown
Contributor Author

rcalixte commented Feb 9, 2024

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
Copy link
Copy Markdown
Member

mattrose commented Feb 9, 2024

Hmm, I will poke, gimme a few minutes

@mattrose
Copy link
Copy Markdown
Member

mattrose commented Feb 9, 2024

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
Copy link
Copy Markdown
Contributor Author

rcalixte commented Feb 9, 2024

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

001

@rcalixte
Copy link
Copy Markdown
Contributor Author

rcalixte commented Feb 9, 2024

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
Copy link
Copy Markdown
Contributor Author

rcalixte commented Feb 9, 2024

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
Copy link
Copy Markdown
Member

mattrose commented Feb 9, 2024

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
Copy link
Copy Markdown
Contributor Author

rcalixte commented Feb 9, 2024

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