command chaining functionality in config#89
command chaining functionality in config#89harindu95 wants to merge 10 commits intossokolow:masterfrom
Conversation
|
Sorry for taking so long to reply. The last few days have been busy. I'll try to find time to review this within the next couple of days. |
ssokolow
left a comment
ssokolow
left a comment
There was a problem hiding this comment.
The main problem I see with these changes is that they are inconsistent with the existing naming conventions within QuickTile.
I've attached instructions to the specific problem sites for how to fix the problem.
quicktile/commands.py
Outdated
| geometry_mask=gravity_mask) | ||
|
|
||
| @commands.add('WithBorder', True) | ||
| @commands.add('borderless', False) |
There was a problem hiding this comment.
The use of capitals rather than dashes to indicate word boundaries in WithBorder is inconsistent with the naming convention for the rest of the commands.
Also, using borderless implies that bordered isn't a toggle.
Please change these to bordered-set and bordered-unset so there's no ambiguity and their naming is consistent if I decide to generalize the naming pattern to all of the toggles. (eg. maximize-set and maximize-unset)
There was a problem hiding this comment.
Yeah. I wasn't sure about how to name it considering some options were capitalized while some were not.
There was a problem hiding this comment.
There shouldn't be any. I certainly don't see any capitals in the output of quicktile --show-actions on my end.
There was a problem hiding this comment.
mmm not the actions. But some of the [general ] config are capitalized while some are not. Since I was considering adding "boderless" as a general state, I guess it kind of bled through on my end.
quicktile/commands.py
Outdated
|
|
||
| def call(self, command, winman, *args, **kwargs): | ||
| def check_command(self, command, winman, *args, **kwargs): | ||
| """ check if the command is valid and execute it""" |
There was a problem hiding this comment.
The name check_command implies that it won't execute the command if it does exist. (A view supported by the docstring saying "check ... and execute it".)
Please either leave this as call or rename it to try_call.
quicktile/commands.py
Outdated
|
|
||
| def call(self, command, winman, *args, **kwargs): | ||
| # type: (str, WindowManager, *Any, **Any) -> bool | ||
| """Resolve a textual positioning command and execute it.""" |
There was a problem hiding this comment.
Please update this docstring to make it clear that the method accepts a comma-separated list of commands.
quicktile/commands.py
Outdated
| logging.error("Unrecognized command: %s", command) | ||
| return False | ||
|
|
||
| def call(self, command, winman, *args, **kwargs): |
There was a problem hiding this comment.
There's no expectation that a completely unadorned name like call on a command registry should support a list of calls.
Please rename this to something like call_multiple. The places you'll need to adjust to support the new name are:
- At the end of
mainin__main__.py, where command-line input is handled - At the end of
doCommandindbus_api.py - In the
callclosure at the end ofkeybinder.py
ssokolow
left a comment
ssokolow
left a comment
There was a problem hiding this comment.
Just one last thing to correct and then I can merge it.
| def call(self, command, winman, *args, **kwargs): | ||
| """Check if the command is valid and execute it.""" | ||
| # type: (str, WindowManager, *Any, **Any) -> bool | ||
| """Resolve a textual positioning command and execute it.""" |
There was a problem hiding this comment.
ssokolow@monolith quicktile [master] % ./run_tests.sh
[...]
quicktile/commands.py:172: error: misplaced type annotation
|
That said, next time I revise the docs, I'll probably also want to add a note that space-separated command-chaining on the command-line is deprecated and may be removed some day in the distant future. |
|
Sorry for remaining silent after your changes. It's been a very messy week. I'll try to do a final check and then get this merged within the next few days. |
| # type: (List[str], WindowManager, *Any, **Any) -> bool | ||
| """Resolve a textual positioning command and execute it. | ||
| Accepts a comma seperated list as the command.""" | ||
| # type: (str, WindowManager, *Any, **Any) -> bool |
There was a problem hiding this comment.
I'll probably need another day or two.
If I'm seeing List[str] turn into str in type signatures without any explanation (whether it's a correction or a mistake), it suggests that, as a responsible project maintainer, I need to do a full audit before merging to make sure I wouldn't be letting in any new bugs.
|
Travis-CI is dying with the current version of your pull request with Given how long this has already taken, if I can find the time within the next week, I'll just clean it up myself. |
|
Sorry for the lack of progress. I haven't forgotten about you. I just have some deadlines looming on other projects. |
This reverts commit f4892f262eb984cad9add54602c4f3c71c0dbc82.
This reverts commit cdf6ea0ac272443e53ed8e23c48039459e14f755.
|
Sorry for being so clumsy. I am still a newbie so forgive me :) . I think I cleaned up the code and the type annotations and it totally works on my machine at least. |
3 participants
I wanted to make windows borderless when I tile them. Now you can do command chaining with ','
KP_2 = borderless,bottom
KP_5 = WithBorder,center
yay!