Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adds some snap helpers to split windows to the left in various ways #1465

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Adds some snap helpers to split windows to the left in various ways #1465

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c2d28a3
Adds some snap helpers to split windows to the left in various ways
jaresty Jun 24, 2024
7ff5564
Add snap right command
jaresty Jun 24, 2024
e9c7119
Add counterclock and focus main content
jaresty Jun 24, 2024
2fd3a03
Used custom functions instead
jaresty Jul 6, 2024
a0ba238
✨ feat(window-management): enhance window snapping functionality
jaresty Aug 22, 2024
b0cd5a1
Merge branch 'main' into various-snap-helpers
jaresty Aug 22, 2024
3e1e786
Merge branch 'main' into various-snap-helpers
jaresty Sep 5, 2024
d16a023
Merge branch 'main' into various-snap-helpers
jaresty Sep 9, 2024
99595a1
Merge branch 'main' into various-snap-helpers
jaresty Oct 11, 2024
bf4c21a
Merge branch 'main' into various-snap-helpers
jaresty Oct 12, 2024
26e6557
Merge branch 'main' into various-snap-helpers
jaresty Oct 14, 2024
fb34ba6
Respond to pull request feedback
jaresty Oct 18, 2024
d50497b
Respond to pull request feedback
jaresty Oct 18, 2024
64ca1b0
Print a more helpful error message when a split layout is not yet def…
jaresty Oct 18, 2024
66f99bf
Make it so it's impossible to match snap split with a single application
jaresty Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions core/windows_and_tabs/window_management.talon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,9 @@ snap last [screen]: user.move_window_previous_screen()
snap screen <number>: user.move_window_to_screen(number)
snap <user.running_applications> <user.window_snap_position>:
user.snap_app(running_applications, window_snap_position)
snap <user.window_split_position> <user.running_applications> <user.running_applications>:
user.snap_split_two(running_applications_1, running_applications_2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
snap <user.window_split_position> <user.running_applications> <user.running_applications>:
user.snap_split_two(running_applications_1, running_applications_2)
snap split <user.running_applications> <user.running_applications>:
user.snap_split_two(running_applications_1, running_applications_2)

Copy link
Collaborator

@phillco phillco Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion, if the version with two windows only really supports splitting side by side, the grammar should look like this. Otherwise you're suggesting you could use any of the other layouts with two windows, and it would still silently give you the 2v2 split. (It shouldn't be possible to say that or it should be an error.)

But I think we had a much better idea detailed below, to make the grammar arbitrarily support layouts with different numbers of windows.

snap <user.window_split_position> <user.running_applications> <user.running_applications> <user.running_applications>:
user.snap_split_three(window_split_position, running_applications_1, running_applications_2, running_applications_3)
snap <user.running_applications> [screen] <number>:
user.move_app_to_screen(running_applications, number)
52 changes: 52 additions & 0 deletions core/windows_and_tabs/window_snap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
"window_snap_positions",
"Predefined window positions for the current window. See `RelativeScreenPos`.",
)
mod.list(
"window_split_positions",
"Predefined window positions when splitting the screen between three applications.",
)
mod.setting(
"window_snap_screen",
type=str,
Expand Down Expand Up @@ -272,14 +276,38 @@ def __init__(self, left, top, right, bottom):
"fullscreen": RelativeScreenPos(0, 0, 1, 1),
}

_split_positions = {
"split": [
_snap_positions["left third"],
_snap_positions["center third"],
_snap_positions["right third"],
],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"split": [
_snap_positions["left third"],
_snap_positions["center third"],
_snap_positions["right third"],
],
"split": [
[
_snap_positions["left"],
_snap_positions["right"],
],
[
_snap_positions["left third"],
_snap_positions["center third"],
_snap_positions["right third"],
]],

Let's make the values a list of lists, with different numbers of inputs. That way the data structure can support snapping layouts with 2/3/4/etc windows ergonomically. The action can then look at the number of windows that were passed, and find the layout that matches that length.

As an added bonus, if there isn't one, it can show a useful error explaining this ("layout 'clock' requires 3, 4, or 5 windows, but 2 were given")

The top level will still be string keys for simplicity (the spoken form) so we don't have to duplicate the list for each n-combination.

For the value, instead of a list of lists, we could use a dictionary with numeric keys (indicating the number of applications supported). But that's redundant given that the length of the value itself already has that information; it's less work for the user to just use list of lists. Make the code do the hard work of figuring out which one matches.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a thought that I've talked myself out of: we could simplify this a bit further. Instead of needing to index into _snap_positions over and over, you can just use the string keys (["left third", "center third", "right third"]). The action implementation already has access to _snap_positions so it could do a simple array map to get those values. This would make the definitions shorter and less duplicative.

The only downside from my perspective is that in your current version you can use an anonymous RelativeScreenPos here without having to put it in _snap_positions, i.e. if there was a layout that only made sense as part of arrangement, not individual snapping. Requiring things be in _snap_positions means it would have to have a spoken form, which I think is a worse requirement.

You could do something where the values could be either string keys (in which case they are assumed to be _snap_positions keys and the action would look them up, or RelativeScreenPos objects (in which case the action would use them as is), but .... may not be worth the magic.

So the current way is fine, just posting this comment in case the line of thought is interesting.

"clock": [
_snap_positions["left"],
_snap_positions["top right"],
_snap_positions["bottom right"],
],
"counterclock": [
_snap_positions["right"],
_snap_positions["top left"],
_snap_positions["bottom left"],
],
}


@mod.capture(rule="{user.window_snap_positions}")
def window_snap_position(m) -> RelativeScreenPos:
return _snap_positions[m.window_snap_positions]


@mod.capture(rule="{user.window_split_positions}")
def window_split_position(m) -> list[RelativeScreenPos]:
return _split_positions[m.window_split_positions]


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think you need a capture here, the "I want a dictionary so the spoken form is a certain string but the action gets the object that corresponds to that string" mapping can be done with a simple list.

(There are lots of places in community that seem to define unnecessary captures that just wrap a list, for reasons I'm not sure about.)

But I'll follow up to make sure. Feel free to hold off until I've checked 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lists are a string->string mapping, they can't map to objects

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derp, right.

ctx = Context()
ctx.lists["user.window_snap_positions"] = _snap_positions.keys()
ctx.lists["user.window_split_positions"] = _split_positions.keys()


@mod.action_class
Expand Down Expand Up @@ -310,6 +338,30 @@ def snap_app(app_name: str, position: RelativeScreenPos):
_bring_forward(window)
_snap_window_helper(window, position)

def snap_split_two(app_one: str, app_two: str):
"""Split the screen between two applications."""
window = _get_app_window(app_one)
window_two = _get_app_window(app_two)
_snap_window_helper(window_two, _snap_positions["right"])
_snap_window_helper(window, _snap_positions["left"])
window_two.focus()
window.focus()

def snap_split_three(
positions: list[RelativeScreenPos], app_one: str, app_two: str, app_three: str
):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just make this one single action with lots of application parameters, and just make the ones greater than 2 optional:

Suggested change
def snap_split_three(
positions: list[RelativeScreenPos], app_one: str, app_two: str, app_three: str
):
def snap_layout(
positions: list[RelativeScreenPos],
app_one: str,
app_two: str,
app_three: Optional[str],
app_four: Optional[str],
app_five: Optional[str]
):

Might as well support up to 5 in case people want to make more interesting layouts.

I don't think there's an explicit null in Talonscript but the shorter-n versions can just leave those parameters off:

user.snap_layout("abc", "def", "jkl")

app_four and app_five will thus be None.

That way you don't have to duplicate the action for each number of applications.

Also I think it'd be helpful to give this functionality a useful name, to distinguish it from the existing single-window snapping functionality. Since "split" is itself a layout I think it should probably have a broader name like "layout" or "arrangement" (not strongly opinionated on the name itself, just that it has one). Do any of those sound good to you, or perhaps do you have a better one? 😁

"""Split the screen between three applications."""
window = _get_app_window(app_one)
window_two = _get_app_window(app_two)
window_three = _get_app_window(app_three)
_snap_window_helper(window, positions[0])
_snap_window_helper(window_two, positions[1])
_snap_window_helper(window_three, positions[2])

window_three.focus()
window_two.focus()
window.focus()

def move_app_to_screen(app_name: str, screen_number: int):
"""Move a specific application to another screen."""
window = _get_app_window(app_name)
Expand Down