Skip to content
27 changes: 23 additions & 4 deletions quicktile/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,9 @@ def decorate(func):
return func
return decorate

def call(self, command, winman, *args, **kwargs):
def check_command(self, command, winman, *args, **kwargs):
""" check if the command is valid and execute it"""

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

# type: (str, WindowManager, *Any, **Any) -> bool
"""Resolve a textual positioning command and execute it."""

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

ssokolow@monolith quicktile [master] % ./run_tests.sh 
[...]
quicktile/commands.py:172: error: misplaced type annotation

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Great! That's done.

cmd = self.commands.get(command, None)

if cmd:
Expand All @@ -183,6 +183,20 @@ def call(self, command, winman, *args, **kwargs):
logging.error("Unrecognized command: %s", command)
return False

def call(self, command, winman, *args, **kwargs):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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 main in __main__.py, where command-line input is handled
  • At the end of doCommand in dbus_api.py
  • In the call closure at the end of keybinder.py

# type: (str, WindowManager, *Any, **Any) -> bool

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

"""Resolve a textual positioning command and execute it."""

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please update this docstring to make it clear that the method accepts a comma-separated list of commands.

cmds = []
success = True
if ',' in command:
cmds = [i.strip() for i in command.split(',')]
for cmd in cmds:
success = self.check_command(cmd, winman, *args, **kwargs)
else:
return self.check_command(command, winman, *args, **kwargs)

return success


#: The instance of L{CommandRegistry} to be used in 99.9% of use cases.
commands = CommandRegistry()
Expand Down Expand Up @@ -341,12 +355,17 @@ def move_to_position(winman, # type: WindowManager
winman.reposition(win, result, use_rect, gravity=gravity,
geometry_mask=gravity_mask)

@commands.add('WithBorder', True)
@commands.add('borderless', False)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah. I wasn't sure about how to name it considering some options were capitalized while some were not.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

There shouldn't be any. I certainly don't see any capitals in the output of quicktile --show-actions on my end.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@commands.add('bordered')
def toggle_decorated(winman, win, state): # pylint: disable=unused-argument
def toggle_decorated(winman, win, state, decoration=None): # pylint: disable=unused-argument
# type: (WindowManager, wnck.Window, Any) -> None
"""Toggle window decoration state on the active window."""
win = gtk.gdk.window_foreign_new(win.get_xid())
win.set_decorations(not win.get_decorations())
if decoration is not None:
win.set_decorations(decoration)
else:
win.set_decorations(not win.get_decorations())

@commands.add('show-desktop', windowless=True)
def toggle_desktop(winman, win, state): # pylint: disable=unused-argument
Expand Down