Code review comment for lp:~elopio/autopilot/fix1257055-slow_drag

Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

Hi,

I'm not sure if this MP is still relevant. If it's not, please delete the proposal to merge. If it is, then:

39 - def drag(self, x1, y1, x2, y2):
40 + def drag(self, x1, y1, x2, y2, rate=10):

If you're adding (or removing, or changing) a parameter to a public method, please document it in the docstrings. What exactly is 'rate' measured in anyway?

82 - def drag(self, x1, y1, x2, y2):
83 + def drag(self, x1, y1, x2, y2, rate=10):

You introduce a new parameter, and then don't use it at all in the method.

In general this MP makes me feel rather uneasy. You're introducing a new parameter, and some code changes, and I can't see any justification for it.

If this is really needed, I think it might be worth moving the algorithm to a common location, and making both the X11 and uinput backends be able to use it. That way, you can test it without needing to mock out anything.

Anyway, please talk to me on IRC about this. We need to either clean it up and merge it, or remove it from the review queue.

review: Needs Fixing

« Back to merge proposal