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

Add dclick option to test assistant #444

Merged
merged 3 commits into from
Nov 17, 2020
Merged

Conversation

notmatthancock
Copy link
Contributor

  • Adds dclick option to EnableTestAssistant.mouse_down
  • Fixes import failure in associated test module
  • Adds tests for EnableTestAssistant.mouse_down

Closes #438

test_assistant = EnableTestAssistant()
component = Component(bounds=[100, 200])
component.normal_left_dclick = mock.Mock()
test_assistant.mouse_down(component, x=0, y=0, dclick=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding a flag for mouse_down, what do you think about having a separate method altogether?

Part of that is because Mouse Down != Mouse Click. Mouse Click includes both pressing the mouse down and releasing it at the same place. With mouse down and mouse release separated, one can, for example, test pressing the mouse somewhere, hold it, and then release it elsewhere. This would be what happens in a drag-and-drop action.
Double clicking a mouse key is a very different command: It involves system settings e.g. how quick two successive clicks should be considered as one double click event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, that makes sense.

@codecov-io
Copy link

codecov-io commented Nov 11, 2020

Codecov Report

Merging #444 (a997bea) into master (854778d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
+ Coverage   28.92%   28.95%   +0.02%     
==========================================
  Files         206      206              
  Lines       18246    18252       +6     
  Branches     2466     2466              
==========================================
+ Hits         5278     5284       +6     
  Misses      12641    12641              
  Partials      327      327              
Impacted Files Coverage Δ
enable/testing.py 90.35% <100.00%> (+0.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 854778d...a997bea. Read the comment docs.

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the addition!
(I am not an enable expert so it might be good to have a second pair of eyes on this.)

'alt_down': alt_down,
'control_down': control_down,
'shift_down': shift_down,
'{0}_down'.format(button): True,
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a while to understand if this "left_down" or "right_down" was necessary... I think it could potentially be handled by the window to change focus, so it makes sense to keep.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whether or not it necessary, what matters is whether the generated event has the same features of the events which are generated by the backends, eg.

left_down=bool(buttons & QtCore.Qt.LeftButton),
middle_down=bool(buttons & QtCore.Qt.MidButton),
right_down=bool(buttons & QtCore.Qt.RightButton),

This doesn't allow generation of chorded double-click events, but that's probably for the best 😄

component.normal_left_dclick.assert_called_once()


@skipIf(ETSConfig.toolkit == "null", "Skipping null tookit")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not happy about this change.

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

Approved.

But can we please have a public base-level testing module which has standard implementations for toolkit skip checks. I am unhappy about the amount of code replication that occurs when we don't have that.

@kitchoi
Copy link
Contributor

kitchoi commented Nov 13, 2020

But can we please have a public base-level testing module which has standard implementations for toolkit skip checks. I am unhappy about the amount of code replication that occurs when we don't have that.

I'd propose handling this separately - it is orthogonal to this PR.

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.

Double click testing not possible with EnableTestAssistant
4 participants