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

Remove 'parent' argument from wx StatusBarManager #1192

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

greschd
Copy link
Contributor

@greschd greschd commented Jan 10, 2023

Another small fix:

Remove the 'parent' argument from 'StatusBarManager.destroy' in the wx backend. The argument is unused, and doesn't match the interface definition.

If you prefer keeping the argument (to not break applications which may pass it explicitly?), I'd propose adding a default =None, and emitting a deprecation warning.

Remove the 'parent' argument from 'StatusBarManager.destroy' in the
WX implementation. The argument is unused, and doesn't match the
interface definition.
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.

Once again, thanks for the PR. This probably needs a corresponding change to the call here:

old.destroy(self.control)

@greschd
Copy link
Contributor Author

greschd commented Jan 11, 2023

Good catch, I've adapted the call site now.

I wonder if we should add some kind of test? Originally, I stumbled upon this because our code triggered the following exception:

Traceback (most recent call last):
  File "d:\ansysdev\thirdparty_sources\pyface\pyface\ui\wx\window.py", line 189, in _wx_on_close
    self.close()
  File "d:\ansysdev\thirdparty_sources\pyface\pyface\workbench\workbench_window.py", line 202, in close
    self.destroy()
  File "d:\ansysdev\thirdparty_sources\pyface\pyface\ui\wx\application_window.py", line 154, in destroy
    super().destroy()
  File "d:\ansysdev\thirdparty_sources\pyface\pyface\i_application_window.py", line 133, in destroy
    self.status_bar_manager.destroy()
TypeError: StatusBarManager.destroy() missing 1 required positional argument: 'parent'

@corranwebster
Copy link
Contributor

I wonder if we should add some kind of test?

There are tests for the create/destroy aspects of the StatusBarManager API that would have detected the problem here:

def test_statusbar(self):
# test that status bar gets created as expected
self.window.status_bar_manager = StatusBarManager(
message="hello world"
)
with self.event_loop():
self.window._create()
with self.event_loop():
self.window.show(True)
with self.event_loop():
self.window.show(False)
with self.event_loop():
self.window.close()
def test_statusbar_changed(self):
# test that status bar gets changed as expected
self.window.status_bar_manager = StatusBarManager(
message="hello world"
)
with self.event_loop():
self.window._create()
with self.event_loop():
self.window.show(True)
with self.event_loop():
self.window.status_bar_manager = StatusBarManager(
message="goodbye world"
)
with self.event_loop():
self.window.show(False)
with self.event_loop():
self.window.close()

Unfortunately those tests need the GuiTestAssistant class which we haven't yet integrated for WxPython (see #614 and #266 - the primary obstacle is not writing the class, it's getting everything running correctly once you have it).

So the long and short of it is, no, no need to write a test, and this looks good to go.

@corranwebster corranwebster merged commit c737dcd into enthought:main Jan 11, 2023
@greschd
Copy link
Contributor Author

greschd commented Jan 11, 2023

Thanks for the review, and the context on testing!

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

2 participants