Release stale native menu command items on rebuild

[lntutor]

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.native is a set of active native representations.
• Menu and toolbar owners track native-item-to-command mappings.
• Rebuild and close paths purge only the items owned by that container.
• No proxy removal API is added; cleanup directly removes the tracked native item.

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:

• I will abide by the BeeWare Code of Conduct
• I have read and have followed the CONTRIBUTING.md file
• This PR was generated or assisted using an AI tool

Assisted-by: OpenAI Codex

[lntutor]

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.

[freakboy3742]

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.

[freakboy3742]

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.

[lntutor]

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.

[Oliver-Leigh]

@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()

[lntutor]

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.

[lntutor]

The refreshed full CI matrix is now green, including both GTK3 Wayland and X11 testbeds. The requested temporary-window lifecycle scenario is covered by test_closed_window_releases_native_command_items; that test passed in the GTK3 jobs. The GTK4-only coverage branch is marked as no-branch to avoid counting an unreachable GTK3 path. Please re-review when convenient.

[Oliver-Leigh]

@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.

[Oliver-Leigh]

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.

[lntutor]

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.

[Oliver-Leigh]

Since this is a window test, it's better to move it to test_window.py.

[lntutor]

Addressed in 74f1f1e. The closed-window lifecycle test now lives in testbed/tests/window/test_window.py.

[Oliver-Leigh]

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.

[lntutor]

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.

[lntutor]

Following up on the current head 74f1f1e712b220e1721b7b2ed421ce08e67e357b with a concrete mapping to the latest review-body concerns.

What changed on this head:

• the closed-window lifecycle regression now lives in testbed/tests/window/test_window.py rather than the app test module;
• backend-specific item discovery moved into the per-backend WindowProbe.command_items() implementations, so the shared test only asserts backend-agnostic ownership/lifecycle behavior;
• the backend-specific skipif was removed; GTK4 capability differences are represented through probe support instead;
• cleanup now happens in the backend window implementations themselves: Cocoa purges window-owned NSToolbarItems for the closing window, and GTK/Qt/WinForms track window-owned toolbar/menu items and drop those native references on rebuild/close.

The shared test no longer depends on the concrete container type of Command.native; it only asks the probe for the native items owned by the temporary window and verifies that those exact items are gone from the command after close.

CI on this exact head is fully green, including the full testbed matrix. Please take another look when convenient.

[lntutor]

@Oliver-Leigh Current head 74f1f1e712b220e1721b7b2ed421ce08e67e357b is still the fully green branch described in my August 1 follow-up, and it incorporates the backend-consistency and lifecycle-test changes from your July 31 review. When you have time, could you take another look at this exact head?

[Oliver-Leigh]

@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.

[lntutor]

@Oliver-Leigh I reworked the implementation on current main in 6a5ba7d72e12416b74b2ecc4a5534b36ce3d5879 to address the backend-consistency concern directly.

Cocoa, GTK, Qt, and WinForms now use the same lifecycle model:

• each Command.native is a set of active native representations;
• app menus, window menus/toolbars, and status-icon menus track native-item-to-command ownership;
• rebuild and close paths remove only the native items owned by that container;
• Cocoa no longer uses a backend-specific scan by native type/target;
• no proxy remove_native API was introduced.

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:

• all pre-commit hooks pass;
• Cocoa testbed regressions pass for app-menu rebuild, MainWindow close, and status-icon rebuild;
• the full GTK, Qt, and WinForms testbeds are running in CI.

I also reopened the four discussions I had previously marked resolved, so resolution remains with the reviewers as requested.