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

Implement dispose for wx simple TextEditor (base class) #1161

Merged
merged 8 commits into from Sep 18, 2020

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented Aug 25, 2020

Closes #942
Also resolve the second item in #1073
Also part of #752

The cause of the issue is as explained in #942. This base class is used by both the text style FontEditor and the text style CompoundEditor.

I came across this while I was trying to resolve #1145.
(The solution I am going to propose for #1145 also "resolves the error" in the context when the UI wrapping this editor has its dispose method called. But I believe removing the handler hooks with Unbind is still the right thing to do.)

As a drive-by improvement to the test coverage, implementations are added for testing the Qt implementation of the same editor.

tester = UITester([LOCAL_TARGET_REGISTRY])
with tester.create_ui(ObjectWithFont(), dict(view=view)) as ui:
wrapper = tester.find_by_name(ui, "font_trait")
wrapper.perform(command.MouseClick())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record, I was able to get the attribute error shown in #942, but for some reasons the event is processed so late that the error occurrs after the context manager exits. Hence it is not captured for causing the test to fail. The error was printed to the console however. I did not try to dive any further into understanding why the exception could not be captured. The fix resolves issue such that the error is not printed to the console.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I observed the same

@kitchoi
Copy link
Contributor Author

kitchoi commented Sep 17, 2020

Updated to test the font editor for Qt as well. It is strictly speaking orthogonal to fixing #942, but is a good opportunity to expand test coverage and UITester support.

Copy link
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.

LGTM! just one minor comment for adding a docstring

@@ -116,6 +116,17 @@ def init(self, parent):
parent.Bind(wx.EVT_TEXT_ENTER, self.update_object, id=self.control.GetId())
self.set_tooltip()

def dispose(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add a docstring here?

@kitchoi kitchoi merged commit 734e103 into master Sep 18, 2020
@kitchoi kitchoi deleted the 942-dispose-wx-text-editor branch September 18, 2020 09:51
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.

wx text style FontEditor needs a dispose method
2 participants