-
Notifications
You must be signed in to change notification settings - Fork 782
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.
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
Adds some snap helpers to split windows to the left in various ways #1465
Conversation
I changed this to use the internal apis directly instead. I got rid of snap left in favor of using a custom snap split function. The snapping seems to work pretty well but sometimes the focus is not where I expect it to be. I think that's probably okay though. I think there is still value in having the snap functions available to use at any moment. |
This seems to be working pretty well-is there anything that I'm missing that needs to be looked at? |
- operates opposite of snap left
- We're concerned about using the existing names in talonscript since folks may want to remap the names - This relies on existing internal apis instead
36d833f
to
2fd3a03
Compare
This consolidates the logic into a single rule in the talon file and reused keys of the existing map for readability.
I learned how to clean this up a bit using talon captures and talon lists. There are only two rules now for splitting with two apps or splitting with three apps. I reused the keys of the the snap window dictionary in python so that I would have some human readable names for the positions that I am snapping to. In addition I switched to |
@phillco was I able to address your concerns? Let me know if I can switch things up to make it reasonable for you :) |
snap <user.window_split_position> <user.running_applications> <user.running_applications>: | ||
user.snap_split_two(running_applications_1, running_applications_2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
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.
core/windows_and_tabs/window_snap.py
Outdated
"split": [ | ||
_snap_positions["left third"], | ||
_snap_positions["center third"], | ||
_snap_positions["right third"], | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
There was a problem hiding this comment.
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.
core/windows_and_tabs/window_snap.py
Outdated
def snap_split_three( | ||
positions: list[RelativeScreenPos], app_one: str, app_two: str, app_three: str | ||
): |
There was a problem hiding this comment.
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:
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? 😁
core/windows_and_tabs/window_snap.py
Outdated
@mod.capture(rule="{user.window_split_positions}") | ||
def window_split_position(m) -> list[RelativeScreenPos]: | ||
return _split_positions[m.window_split_positions] | ||
|
||
|
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
derp, right.
No description provided.