Skip to content

Refactor registering interaction handlers for editable textboxes#1190

Merged
kitchoi merged 6 commits into
masterfrom
refactor-textbox-handlers
Sep 4, 2020
Merged

Refactor registering interaction handlers for editable textboxes#1190
kitchoi merged 6 commits into
masterfrom
refactor-textbox-handlers

Conversation

@kitchoi
Copy link
Copy Markdown
Contributor

@kitchoi kitchoi commented Sep 3, 2020

This PR refactors how we add interaction handlers for editable textboxes.

There are small changes to the tests to exercise more interactions in order to ensure the refactoring has not broken things.

With this refactoring, I can add interactions for things like the text-style FontEditor relatively easily (see #1161).

@kitchoi
Copy link
Copy Markdown
Contributor Author

kitchoi commented Sep 4, 2020

Ooooo that's interesting, this error is only seen on Windows:

======================================================================
FAIL: test_simple_auto_set_update_text (traitsui.tests.editors.test_text_editor.TestTextEditor)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\appveyor\.edm\envs\traitsui-test-3.6-wx\lib\site-packages\traitsui\tests\editors\test_text_editor.py", line 173, in test_simple_auto_set_update_text
    name_field.perform(command.KeySequence("NEW"))
  File "C:\Users\appveyor\.edm\envs\traitsui-test-3.6-wx\lib\site-packages\traits\testing\unittest_tools.py", line 119, in __exit__
    raise self.failureException(msg.format(*items))
AssertionError: Change event for name was fired 3 times instead of 4

Looks orthogonal. I will open a separate issue and revert the change to the test for now.

@kitchoi
Copy link
Copy Markdown
Contributor Author

kitchoi commented Sep 4, 2020

this error is only seen on Windows:

That's misleading: It is not seen on OSX but I don't know if the error should occur on Linux because our Linux+wx CI job has not been running.

@kitchoi
Copy link
Copy Markdown
Contributor Author

kitchoi commented Sep 4, 2020

Looks orthogonal. I will open a separate issue and revert the change to the test for now.

It is because on Windows + wx, the first input is always being activated and in the case of textbox, the entire text is selected. This makes the test setup different from Mac OS or with Qt. Therefore this is orthogonal to the refactoring.

Copy link
Copy Markdown
Contributor

@aaronayres35 aaronayres35 left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Just one question about where to place the new function

)


def register_editable_textbox_handlers(registry, target_class, widget_getter):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I very much like this refactor, pulling this logic into its own function is much cleaner. However, do you think we should move this into the helpers module, or somewhere else? It seems a little out of place in common_ui_targets IMO, what do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree this function does not sit well with this module.

But I am a bit hesitant about moving it to helpers. The only UITester/UIWrapper piece of knowledge in that module is the use of interaction object, and in the places where interaction is referred to, they could be replaced with more primitive data such as a string. Perhaps helpers should be renamed to something more meaningful to prevent it from becoming a catch-all-I-don't-know-where-this-goes kind of place.

What about... we move this register_editable_textbox_handlers to a new module called registry_helper? This will mirror the one on tester package and is responsible for holding functions that does some common handler registration.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That sounds good to me!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, I agree that helpers might be a bit too generic (all code we write should be helping with something!). Currently all the code in helpers is specific to some form of interaction (keyclicks, mouse clicks, key sequences, and DisplayedText queries) so maybe just something like interaction_helpers or something would be sufficient. We can leave that for another PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cool. I will move this function to a new module.
Agreed the renaming of helpers can be a separate PR.

Copy link
Copy Markdown
Contributor

@aaronayres35 aaronayres35 left a comment

Choose a reason for hiding this comment

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

Changes still LGTM!

@kitchoi kitchoi merged commit 8844145 into master Sep 4, 2020
@kitchoi kitchoi deleted the refactor-textbox-handlers branch September 4, 2020 16:57
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.

2 participants