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.

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

Fix some bugs in Aenea #64

Closed
wants to merge 1 commit into from
Closed

Fix some bugs in Aenea #64

wants to merge 1 commit into from

Conversation

djr7C4
Copy link
Contributor

@djr7C4 djr7C4 commented Sep 14, 2014

  1. dragonfly uses lessthan and greaterthan but Aenea uses less and greater; this makes it impossible to use Aenea Key actions involving '<' or '>'
  2. Fixed broken Mouse actions. Previously, args and kw args were not passed up to base classes resulting in a missing self._spec
  3. Fixed the linux server to handle negative arguments for mouse actions correctly; previously, it did not pass the proper commands to xdotool which caused errors
  4. Fixed _make_mouse_parser to allow negative numbers
  5. Added missing self._str values in various Aenea context classes. This caused errors when attempting to combine contexts in various ways before

1. dragonfly uses lessthan and greaterthan but Aenea uses less and greater; this makes it impossible to use Aenea Key actions involving '<' or '>'
2. Fixed broken Mouse actions.  Previously, args and kw args were not passed up to base classes resulting in a missing._spec
3. Fixed the linux server to handle negative arguments for mouse actions correctly; previously, it did not pass the proper commands to xdotool which caused errors
4. Fixed _make_mouse_parser to allow negative numbers
5. Added missing self._str values in various Aenea context classes.  This caused errors when attempting to combine contexts in various ways before
@djr7C4 djr7C4 changed the title Fix the following bugs in Aenea: Fix some bugs in Aenea Sep 14, 2014
@mzizzi
Copy link
Contributor

mzizzi commented Sep 14, 2014

Looking forward to seeing this reviewed. I had previously noticed issue 1 and always attributed it to a fault of my own in a grammar somewhere.

proxy = _spec(aenea.proxy_actions.ProxyText, a, kw)
local = _spec(dragonfly.Text, a, kw)
proxy = _spec(aenea.proxy_actions.ProxyText, *a, **kw)
local = _spec(dragonfly.Text, *a, **kw)
Copy link
Member

Choose a reason for hiding this comment

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

Is this what you mean in point 2? Are you sure this is correct? Reading the signature for _spec, it expects three arguments -- a callable, a list, and a dict, not unpacked arguments. By my reading this introduces a bug. Am I missing something?

Assuming my understanding is correct, perhaps we should change spec to take _a, *_kw by the principle of least surprise.

@calmofthestorm
Copy link
Member

Splitting this into one pull request per bug or feature would be the fastest way forward. I agree completely with some of these but have reservations about others, and it's difficult to isolate each issue, reproduce the old bug, and verify the fix works and breaks nothing when they're all together.

Replies to your points:

  1. We can merge this for now, but I think we need a broader conversation about key translation going forward, now that we have three different server implementations on three different platforms. See Key translations requires a rethink #65 for further discussion.

  2. I assume you're referring to the _a and *_kw in strict? You're correct in strict.py but I believe the analogous changes in lax.py are incorrect since spec takes a, kw not _a, *_kw. Double check and let me know if I'm missing something.

  3. Straightforward bug fix; happy to merge.

  4. Straightforward bug fix; happy to merge.

  5. Happy to merge, but could you give me an example (or better yet, the grammar in question) to reproduce this? I want to make sure there aren't any related issues with the way some of the actions/contexts are factory functions rather than classes that return a different class than their name.

The way I would split this up is 1, 2, and 5 should each be separate. You can put 3 and 4 together if you want, since it's basically one feature and I plan to test it as a unit.

@djr7C4
Copy link
Contributor Author

djr7C4 commented Sep 19, 2014

Yes, you are right about 2. I think I did it first in strict.py and then tried to copy the fix over to lax.py

@calmofthestorm
Copy link
Member

The real lesson here is that the project has grown (and changed) enough that I need to completely rethink the testing strategy, and ideally set up CI. I'm not sure how much testing we can do at the higher levels (stuff that touches Dragonfly), but the core client (and server) should at the very least be thoroughly tested to handle all sensible inputs and produce the correct calls/objects. That would catch things like the negative mouse issue.

Overall since this is a project targeted at software engineers for interactive use, "find bugs in production" is a far better strategy than usual (pretty low bar!), but I'd aim to do better.

@calmofthestorm calmofthestorm mentioned this pull request Sep 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants