Release stale native menu command items on rebuild - #4587
Conversation
fbd301e to
dc00219
Compare
|
CI note: the linux-wayland-qt testbed job failed only in the existing layout-sensitive tests/widgets/test_selection.py::test_flex_horizontal_widget_size (expected width at least 350; observed 73). The new command_rebuild_replaces_native_items test passed in that job. All other completed backend testbeds are green; the remaining jobs are still running. |
dc00219 to
2af2346
Compare
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
As noted inline, the additional test isn't actually verifying anything; but the core of the implementation is on the right track. While the mechanics are correct, it's missing the "spirit" of the suggested change.
Cocoa is highlighted as a backend that doesn't have the problem - it has a matching pair of create_menu_item and remove_menu_item methods, and remove_menu_item does the full removal process. This means there's symmetry in the interface, and the abstracted create/remove method does the whole job.
However, in your PR, you've added remove_native methods... which are nothing more than a proxy to calling native.remove() (or the equivalent). We either need to work out how to abstract a full create_menu_item()/remove_menu_item() pair; or simplify the implementation to call the remove method directly.
|
|
||
|
|
||
| async def test_command_rebuild_replaces_native_items(app, app_probe): | ||
| """Rebuilding menus must release obsolete native command items.""" |
There was a problem hiding this comment.
How is this a test of the release behavior? The only thing it asserts is that a newly created menu item generates a new native item.
There was a problem hiding this comment.
The revised regression captures every pre-rebuild native item for an existing command, triggers a real application menu rebuild by adding another command, then asserts that none of the captured items remain registered on the original command. A second Qt/WinForms regression creates a temporary window with menu and toolbar items, closes it, and verifies those exact native objects are removed.
|
@lntutor It's great that you've made some progress on the issue. There is also the issue of deleting the menu items when the MainWindow is removed. Here's an example to highlight the problem: import gc
import toga
class MinimalApp(toga.App):
def __init__(self):
super().__init__(
formal_name="MinimalApp", app_id="com.example.minimal.app"
)
def startup(self):
def on_running(app=self):
all_commands = [cmd for cmd in app.commands._commands.values()]
cmd = all_commands[0]
for i in range(10):
temp_window = toga.MainWindow(title="Temp Window")
temp_window.show()
temp_window.close()
gc.collect()
print(f"The number of Toga windows is {len(self.windows)}.")
print(f"Native instances of `cmd`: {len(cmd._impl.native)}")
self.on_running = on_running
self.main_window = toga.MainWindow(title=self.formal_name)
self.main_window.show()
def main():
return MinimalApp()
if __name__ == "__main__":
app = main()
app.main_loop() |
|
Follow-up to the review: the commits after 2af2346 now take the direct-removal option. Qt and WinForms filter each window-owned item out of the associated command native list during window cleanup; no remove_native proxy interface was retained. The accompanying test selects only the items owned by the command being checked. |
|
The refreshed full CI matrix is now green, including both GTK3 Wayland and X11 testbeds. The requested temporary-window lifecycle scenario is covered by |
Oliver-Leigh
left a comment
Oliver-Leigh
left a comment
There was a problem hiding this comment.
@lntutor Thanks for making changes to address the issue that I raised. This is getting a lot closer!
I want to raise a point in the spirit of something @freakboy3742 mentioned: We would like there to be as much consistency between the platforms as possible. It looks like you're getting good consistency between GTK, Qt and Windows, but not with macOS. For example, Command.native is a set on macOS, but a list elsewhere. Also, on macOS purge_toolbar uses a very different method to what you're using here.
I've highlighted a few other things in-line that need to be fixed too.
| temporary_window.toolbar.add(command) | ||
| await app_probe.redraw("Temporary window created") | ||
|
|
||
| if toga.backend == "toga_gtk": |
There was a problem hiding this comment.
We try not to add backend specifics in this way. If there are backend specific methodologies, then we prefer to use a probe and add the implementation specific code there. For example, in this test, you could create a window probe.
There was a problem hiding this comment.
Addressed in 74f1f1e. Backend-specific command-item discovery now lives in the Cocoa, GTK, Qt, and WinForms WindowProbe implementations; the shared test only performs backend-agnostic lifecycle assertions.
| or (toga.backend == "toga_gtk" and os.environ.get("TOGA_GTK") == "4"), | ||
| reason=("Window-local command items are only created by GTK3, Qt, and WinForms."), | ||
| ) | ||
| async def test_closed_window_releases_native_command_items(app, app_probe): |
There was a problem hiding this comment.
Since this is a window test, it's better to move it to test_window.py.
There was a problem hiding this comment.
Addressed in 74f1f1e. The closed-window lifecycle test now lives in testbed/tests/window/test_window.py.
| @pytest.mark.skipif( | ||
| toga.backend not in {"toga_gtk", "toga_qt", "toga_winforms"} | ||
| or (toga.backend == "toga_gtk" and os.environ.get("TOGA_GTK") == "4"), | ||
| reason=("Window-local command items are only created by GTK3, Qt, and WinForms."), | ||
| ) |
There was a problem hiding this comment.
We don't use this method to skip tests - if skipping is needed, you can find the functions in conftest.py. However, I don't think skipping is needed here. You write "Window-local command items are only created by GTK3, Qt, and WinForms", but the toolbar items are created on the other desktop backends.
There was a problem hiding this comment.
Addressed in 74f1f1e. The backend-specific skipif decorator was removed, so the test now covers Cocoa, GTK3, Qt, and WinForms. The unsupported GTK4 case is represented as a probe capability and routed through the existing skip_on_backends helper.
|
Following up on the current head What changed on this head:
The shared test no longer depends on the concrete container type of CI on this exact head is fully green, including the full testbed matrix. Please take another look when convenient. |
|
@Oliver-Leigh Current head |
|
@lntutor I’ve looked through the changes and I can’t see where you have addressed my concerns about the backend consistency. Can you have another look and let me know when these are addressed? Something else to keep in mind: It is highlighted in our contribution guide that the reviewer determines if an issue is resolved. If you mark an issue as resolved yourself, it makes the review process take considerably longer. |
Assisted-by: OpenAI Codex GPT-5 <noreply@openai.com>
74f1f1e to
6a5ba7d
Compare
|
@Oliver-Leigh I reworked the implementation on current Cocoa, GTK, Qt, and WinForms now use the same lifecycle model:
The backend probes now expose the same ownership mapping to the shared window lifecycle test. I also strengthened the app-menu and status-icon tests so they assert the old native objects are actually replaced or removed, avoiding the earlier vacuous-test problem. Verification on this exact commit:
I also reopened the four discussions I had previously marked resolved, so resolution remains with the reviewers as requested. |
Assisted-by: OpenAI Codex GPT-5 <noreply@openai.com>
Assisted-by: OpenAI Codex GPT-5 <noreply@openai.com>
Oliver-Leigh
left a comment
Oliver-Leigh
left a comment
There was a problem hiding this comment.
Apologies for the delay in getting back to you on this. I've now had a chance to look have a deeper look. This is definitely heading in the right direction, but there are some changes that need to be made.
In regards to the testing, there shouldn't be any need for backend specific flags. It's possible to test the desired properties in a backend-agnostic way. I've detailed how to do this in my suggestions below.
Also, don't forget to update the Command.native type in the other backends.
| self.toolbar_items = {} | ||
| self._toolbar_items = {} |
There was a problem hiding this comment.
I see what you've done here, but this naming is confusing. I would suggest using the following across all the backends:
self._toolbar_native_items = {}self._menu_native_items = {}
| cmd._impl.native.append(item_impl) | ||
| self.toolbar_items[cmd] = item_impl | ||
| cmd._impl.native.add(item_impl) | ||
| self.toolbar_items[item_impl] = cmd |
There was a problem hiding this comment.
We want to be able to implement this new structure across existing and future backends. The native implementations for some backends may not be hashable (e.g. the upcoming WinUI 3 backend). I can't see where it's needed that this is a dict. It might just be easier to have a set of pairs (item_native, cmd).
| async def test_command_rebuild_replaces_native_items(app, app_probe): | ||
| """Rebuilding menus must release obsolete native command items.""" | ||
| if not app_probe.supports_application_menu_command_native_items: | ||
| pytest.skip("Application menu commands don't expose native items.") | ||
|
|
||
| command = app.cmd1 | ||
| old_items = set(command._impl.native) | ||
| assert old_items | ||
|
|
||
| app.commands.add( | ||
| toga.Command( | ||
| action=None, | ||
| text="Rebuild command", | ||
| group=toga.Group.FILE, | ||
| ) | ||
| ) | ||
| await app_probe.redraw("Application menus rebuilt") | ||
|
|
||
| assert old_items.isdisjoint(command._impl.native) | ||
|
|
||
|
|
There was a problem hiding this comment.
As far as I know, there is no requirement that the native items are destroyed and recreated. We're more interested in testing that there is no increase in the number of native items for each Command instance. Your current test doesn't actually test this property.
I suggest that you take the code from #4580 as an example, and build a test for the number of native items. There are some changes that are needed, for example you should make a new Group and test the number of native items for all existing Command instances stays the same.
This test can be run on all existing and future backends, so won't require your supports_application_menu_command_native_items flag.
| @pytest.mark.parametrize( | ||
| "second_window_class, second_window_kwargs", | ||
| [(toga.MainWindow, {"title": "Temporary window"})], | ||
| ) |
There was a problem hiding this comment.
There is no need for this because your list of values only has length 1. You can just define second_window directly in the test.
There was a problem hiding this comment.
This is following the pattern of a lot of other tests that use the second_window fixture; even if there's only one test case, the process of creating the second_window_probe is just complex enough that it's worth parameterising.
| async def test_closed_window_releases_native_command_items( | ||
| app, second_window, second_window_probe | ||
| ): | ||
| """Closing a window releases its native command menu and toolbar items.""" | ||
| if not second_window_probe.supports_command_items: | ||
| skip_on_backends( | ||
| toga.backend, | ||
| reason="Window command items are not implemented on this backend.", | ||
| ) | ||
|
|
||
| command = app.cmd1 | ||
| second_window.toolbar.add(command) | ||
| second_window.show() | ||
| await second_window_probe.wait_for_window("Temporary window created") | ||
|
|
||
| window_items = second_window_probe.command_items(command) | ||
| assert window_items | ||
| assert window_items <= command._impl.native | ||
|
|
||
| second_window.close() | ||
| await second_window_probe.wait_for_window("Temporary window closed") | ||
|
|
||
| assert window_items.isdisjoint(command._impl.native) |
There was a problem hiding this comment.
The issue which I raise in #4587 (comment) was that the act of creating a new window added native items to existing Commands. I don't see how this test is capturing that information.
The test test_secondary_window_cleanup already exists, and would be a good place to add the new testing code. You can look at the code from #4587 (comment) to see what needs to be added.
The flags should not ne needed with the new test that I suggested.
| if app_probe.supports_status_icon_command_native_items: | ||
| assert not new_cmd1._impl.native |
There was a problem hiding this comment.
I think this supports_status_icon_command_native_items narrows the test scope too much. We should have a test that works on all backends. I think that the testing method that I described in https://github.com/beeware/toga/pull/4587/changes#r3819636164 can be used here too.
| # from the toolbar | ||
| for cmd, item_impl in self.toolbar_items.items(): | ||
| def purge_toolbar(self): | ||
| if GTK_VERSION < (4, 0, 0): # pragma: no-cover-if-gtk4 # pragma: no branch |
There was a problem hiding this comment.
When combined with the no-cover-if-gtk4, the addition of no branch is highly suspicious. I suspect there's a simplification required here - probably a placeholder no-op else clause with a no-cover-if-gtk3
| def create(self): | ||
| super().create() | ||
| self.menu_items = {} | ||
| self.toolbar_items = {} |
There was a problem hiding this comment.
These properties feels like they should be defined in __init__(), not create().
| @pytest.mark.parametrize( | ||
| "second_window_class, second_window_kwargs", | ||
| [(toga.MainWindow, {"title": "Temporary window"})], | ||
| ) |
There was a problem hiding this comment.
This is following the pattern of a lot of other tests that use the second_window fixture; even if there's only one test case, the process of creating the second_window_probe is just complex enough that it's worth parameterising.
3 participants
Fixes #4580
Rebuilding application menus, window menus and toolbars, and status-icon menus now deregisters obsolete native command items on Cocoa, GTK, Qt, and WinForms.
The desktop backends use the same ownership model:
Command.nativeis a set of active native representations.The testbed covers application-menu rebuilds, status-icon menu rebuilds, and closing a temporary MainWindow with command items. Backend-specific discovery remains in WindowProbe implementations.
PR Checklist:
Assisted-by: OpenAI Codex