command chaining functionality in config #89

harindu95 · 2017-09-16T08:19:00Z

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!

coveralls · 2017-09-16T08:20:20Z

Coverage decreased (-0.5%) to 33.307% when pulling c1aeaaf on harindu95:master into dac1e1a on ssokolow:master.

coveralls · 2017-09-16T08:46:57Z

Coverage decreased (-0.5%) to 33.36% when pulling f4064c3 on harindu95:master into dac1e1a on ssokolow:master.

ssokolow · 2017-09-18T14:53:22Z

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

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.

ssokolow · 2017-09-21T05:35:34Z

quicktile/commands.py

@@ -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)


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)

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

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

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.

ssokolow · 2017-09-21T05:40:40Z

quicktile/commands.py

@@ -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"""


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.

ssokolow · 2017-09-21T05:41:26Z

quicktile/commands.py

@@ -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):
+        # type: (str, WindowManager, *Any, **Any) -> bool
+        """Resolve a textual positioning command and execute it."""


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

ssokolow · 2017-09-21T05:45:46Z

quicktile/commands.py

@@ -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):


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

coveralls · 2017-09-25T21:27:59Z

Coverage decreased (-0.5%) to 33.36% when pulling 8d0d0e9 on harindu95:master into dac1e1a on ssokolow:master.

coveralls · 2017-09-25T21:31:36Z

Coverage decreased (-0.5%) to 33.36% when pulling 76e97dc on harindu95:master into dac1e1a on ssokolow:master.

ssokolow

Just one last thing to correct and then I can merge it.

ssokolow · 2017-09-29T15:58:22Z

quicktile/commands.py

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


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

Great! That's done.

ssokolow · 2017-09-29T16:07:37Z

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.

ssokolow · 2017-10-29T22:32:42Z

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.

ssokolow · 2017-11-01T20:52:12Z

quicktile/commands.py

        """Resolve a textual positioning command and execute it.
           Accepts a comma seperated list as the command."""
+        # type: (str, WindowManager, *Any, **Any) -> bool


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.

ssokolow · 2017-11-01T20:54:40Z

Travis-CI is dying with the current version of your pull request with IndentationError: unexpected unindent. That's a flat-out parser error. (ie. It indicates that you didn't even try to run the code you submitted.)

Given how long this has already taken, if I can find the time within the next week, I'll just clean it up myself.

ssokolow · 2017-11-14T06:01:25Z

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.

coveralls · 2017-12-21T23:35:50Z

Coverage decreased (-0.5%) to 33.36% when pulling 418185a on harindu95:master into dac1e1a on ssokolow:master.

harindu95 · 2017-12-21T23:36:38Z

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.

command chaining functionality in config

c1aeaaf

Housekeeping

f4064c3

ssokolow requested changes Sep 21, 2017

View reviewed changes

harindu95 added 2 commits September 25, 2017 15:17

naming convensions and docstring update

8d0d0e9

changine check_command back into call

76e97dc

ssokolow requested changes Sep 29, 2017

View reviewed changes

harindu95 added 2 commits October 22, 2017 23:01

Resolving type issue

cdf7ea0

Resolving misplaced type annotation

f4894f2

ssokolow reviewed Nov 1, 2017

View reviewed changes

harindu95 added 4 commits December 21, 2017 16:19

Correcting type annotations.

7d4400a

Revert "Resolving misplaced type annotation"

f90de3d

This reverts commit f4892f262eb984cad9add54602c4f3c71c0dbc82.

Revert "Resolving type issue"

4d47975

This reverts commit cdf6ea0ac272443e53ed8e23c48039459e14f755.

Merge commit '7d4400a'

418185a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

command chaining functionality in config #89

command chaining functionality in config #89

harindu95 commented Sep 16, 2017

coveralls commented Sep 16, 2017

coveralls commented Sep 16, 2017

ssokolow commented Sep 18, 2017

ssokolow left a comment

ssokolow Sep 21, 2017

harindu95 Sep 25, 2017

ssokolow Sep 25, 2017

harindu95 Oct 23, 2017

ssokolow Sep 21, 2017

ssokolow Sep 21, 2017

ssokolow Sep 21, 2017

coveralls commented Sep 25, 2017

coveralls commented Sep 25, 2017

ssokolow left a comment

ssokolow Sep 29, 2017

harindu95 Oct 23, 2017

ssokolow commented Sep 29, 2017

ssokolow commented Oct 29, 2017

ssokolow Nov 1, 2017

ssokolow commented Nov 1, 2017

ssokolow commented Nov 14, 2017

coveralls commented Dec 21, 2017

harindu95 commented Dec 21, 2017

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

command chaining functionality in config #89

Are you sure you want to change the base?

command chaining functionality in config #89

Conversation

harindu95 commented Sep 16, 2017

coveralls commented Sep 16, 2017

coveralls commented Sep 16, 2017

ssokolow commented Sep 18, 2017

ssokolow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 25, 2017

coveralls commented Sep 25, 2017

ssokolow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssokolow commented Sep 29, 2017

ssokolow commented Oct 29, 2017

Choose a reason for hiding this comment

ssokolow commented Nov 1, 2017

ssokolow commented Nov 14, 2017

coveralls commented Dec 21, 2017

harindu95 commented Dec 21, 2017