Merge pull request #2237 from freakboy3742/separator-fix

Correct handling of separators.

android/src/toga_android/app.py

@@ -7,7 +7,7 @@
 from java import dynamic_proxy
 from org.beeware.android import IPythonApp, MainActivity
 
-from toga.command import GROUP_BREAK, SECTION_BREAK, Command, Group
+from toga.command import Command, Group, Separator
 
 from .libs import events
 from .window import Window
@@ -96,25 +96,27 @@ def onPrepareOptionsMenu(self, menu):
 
         # create option menu
         for cmd in self._impl.interface.commands:
-            if cmd == SECTION_BREAK or cmd == GROUP_BREAK:
+            if isinstance(cmd, Separator):
                 groupid += 1
                 continue
 
             # Toolbar commands are added below.
             if cmd in self._impl.interface.main_window.toolbar:
                 continue
 
-            if cmd.group.key in menulist:
+            try:
+                # Find the menu representing the group for this command
                 menugroup = menulist[cmd.group.key]
-            else:
-                # create all missing submenus
+            except KeyError:
+                # Menu doesn't exist yet; create it.
                 parentmenu = menu
                 groupkey = ()
+                # Iterate over the full key, creating submenus as needed
                 for section, order, text in cmd.group.key:
                     groupkey += ((section, order, text),)
-                    if groupkey in menulist:
+                    try:
                         menugroup = menulist[groupkey]
-                    else:
+                    except KeyError:
                         if len(groupkey) == 1 and text == Group.COMMANDS.text:
                             # Add this group directly to the top-level menu
                             menulist[groupkey] = menu
@@ -127,20 +129,30 @@ def onPrepareOptionsMenu(self, menu):
                             menulist[groupkey] = menugroup
                     parentmenu = menugroup
 
-            # create menu item
+            # Create menu item
             menuitem = menugroup.add(groupid, itemid, Menu.NONE, cmd.text)
             menuitem.setShowAsActionFlags(MenuItem.SHOW_AS_ACTION_NEVER)
             menuitem.setEnabled(cmd.enabled)
             self.menuitem_mapping[itemid] = cmd
             itemid += 1
 
-        # create toolbar actions
+        # Create toolbar actions
         if self._impl.interface.main_window:  # pragma: no branch
+            prev_group = None
             for cmd in self._impl.interface.main_window.toolbar:
-                if cmd == SECTION_BREAK or cmd == GROUP_BREAK:
+                if isinstance(cmd, Separator):
                     groupid += 1
+                    prev_group = None
                     continue
 
+                # A change in group requires adding a toolbar separator
+                if prev_group is not None and cmd.group != prev_group:
+                    groupid += 1
+                    prev_group = None
+                else:
+                    prev_group = cmd.group
+
+                # Add a menu item for the toolbar command
                 menuitem = menu.add(groupid, itemid, Menu.NONE, cmd.text)
                 # SHOW_AS_ACTION_IF_ROOM is too conservative, showing only 2 items on
                 # a medium-size screen in portrait.

changes/2193.bugfix.rst

@@ -0,0 +1 @@
+Separators before and after command sub-groups are now included in menus.

cocoa/src/toga_cocoa/app.py

@@ -8,7 +8,7 @@
 from rubicon.objc.eventloop import CocoaLifecycle, EventLoopPolicy
 
 import toga
-from toga.command import GROUP_BREAK, SECTION_BREAK
+from toga.command import Separator
 from toga.handlers import NativeHandler
 
 from .keys import cocoa_key
@@ -333,13 +333,10 @@ def create_menus(self):
         self._menu_items = {}
 
         for cmd in self.interface.commands:
-            if cmd == GROUP_BREAK:
-                submenu = None
-            elif cmd == SECTION_BREAK:
+            submenu = self._submenu(cmd.group, menubar)
+            if isinstance(cmd, Separator):
                 submenu.addItem(NSMenuItem.separatorItem())
             else:
-                submenu = self._submenu(cmd.group, menubar)
-
                 if cmd.shortcut:
                     key, modifier = cocoa_key(cmd.shortcut)
                 else:

cocoa/src/toga_cocoa/window.py

@@ -1,4 +1,4 @@
-from toga.command import Command
+from toga.command import Command, Separator
 from toga_cocoa.container import Container
 from toga_cocoa.images import nsdata_to_bytes
 from toga_cocoa.libs import (
@@ -20,10 +20,7 @@
 
 
 def toolbar_identifier(cmd):
-    if isinstance(cmd, Command):
-        return "ToolbarItem-%s" % id(cmd)
-    else:
-        return "ToolbarSeparator-%s" % id(cmd)
+    return f"Toolbar-{type(cmd).__name__}-{id(cmd)}"
 
 
 class TogaWindow(NSWindow):
@@ -59,8 +56,16 @@ def toolbarAllowedItemIdentifiers_(self, toolbar):  # pragma: no cover
     def toolbarDefaultItemIdentifiers_(self, toolbar):
         """Determine the list of toolbar items that will display by default."""
         default = NSMutableArray.alloc().init()
+        prev_group = None
         for item in self.interface.toolbar:
+            # If there's been a group change, and this item isn't a separator,
+            # add a separator between groups.
+            if prev_group is not None:
+                if item.group != prev_group and not isinstance(item, Separator):
+                    default.addObject_(toolbar_identifier(prev_group))
             default.addObject_(toolbar_identifier(item))
+            prev_group = item.group
+
         return default
 
     @objc_method

cocoa/tests_backend/window.py

@@ -240,7 +240,9 @@ def has_toolbar(self):
 
     def assert_is_toolbar_separator(self, index, section=False):
         item = self.native.toolbar.items[index]
-        assert str(item.itemIdentifier).startswith("ToolbarSeparator-")
+        assert str(item.itemIdentifier).startswith(
+            f"Toolbar-{'Separator' if section else 'Group'}"
+        )
 
     def assert_toolbar_item(self, index, label, tooltip, has_icon, enabled):
         item = self.native.toolbar.items[index]

core/src/toga/command.py

@@ -263,21 +263,21 @@ def __repr__(self) -> bool:
         )
 
 
-class Break:
-    def __init__(self, name: str):
-        """A representation of a separator between Command Groups, or between sections
-        in a Group.
+class Separator:
+    def __init__(self, group: Group = None):
+        """A representation of a separator between sections in a Group.
 
-        :param name: A name of the break type.
+        :param group: The group that contains the separator.
         """
-        self.name = name
+        self.group = group
 
     def __repr__(self) -> str:
-        return f"<{self.name} break>"
-
+        return f"<Separator group={self.group.text}>"
 
-GROUP_BREAK = Break("Group")
-SECTION_BREAK = Break("Section")
+    def __eq__(self, other) -> bool:
+        if isinstance(other, Separator):
+            return self.group == other.group
+        return False
 
 
 class CommandSetChangeHandler(Protocol):
@@ -337,14 +337,81 @@ def app(self) -> App:
     def __len__(self) -> int:
         return len(self._commands)
 
-    def __iter__(self) -> Command | Group | Break:
-        prev_cmd = None
-        for cmd in sorted(self._commands):
-            if prev_cmd:
-                if cmd.group != prev_cmd.group:
-                    yield GROUP_BREAK
-                elif cmd.section != prev_cmd.section:
-                    yield SECTION_BREAK
-
-            yield cmd
-            prev_cmd = cmd
+    def __iter__(self) -> Command | Separator:
+        cmd_iter = iter(sorted(self._commands))
+
+        def descendant(group, ancestor):
+            # Return the immediate descendant of ancestor used by this group.
+            if group.parent == ancestor:
+                return group
+            if group.parent:
+                return descendant(group.parent, ancestor)
+            return None
+
+        # The iteration over commands tells us the exact order of commands, but doesn't
+        # tell us anything about menu and submenu structure. In order to insert section
+        # breaks in the right place (including before and after submenus), we need to
+        # iterate over commands inside each group, dealing with each subgroup as an
+        # independent iteration.
+        #
+        # The iterator over commands is maintained external to this recursive iteration,
+        # because we may need to inspect the command at multiple group levels, but we
+        # can't `peek` at the top element of an iterator, `push` an item back on after
+        # it has been consumed, or pass the consumed item as a return value in addition
+        # to the generator result.
+        def _iter_group(parent):
+            nonlocal command
+            nonlocal finished
+            section = None
+
+            def _section_break(obj):
+                # Utility method that will insert a section break, if required.
+                # A section break is needed if the section for the object we're
+                # processing (either a command, or a group acting as a submenu)
+                # has a section ID different to the previous object processed at
+                # this level, excluding the very first object (as there's no need
+                # for a section break before the first command/submenu).
+                nonlocal section
+                if section is not None:
+                    if section != obj.section:
+                        yield Separator(parent)
+                        section = obj.section
+                else:
+                    section = obj.section
+
+            while not finished:
+                if parent is None:
+                    # Handle root-level menus
+                    yield from _iter_group(command.group.root)
+                elif command.group == parent:
+                    # A normal command at this level of the group.
+                    yield from _section_break(command)
+                    yield command
+
+                    # Consume the next item on the iterator; if we run out, mark the
+                    # sentinel that says we've finished. We can't just raise
+                    # StopIteration, because that stops the *generator* we're creating.
+                    try:
+                        command = next(cmd_iter)
+                    except StopIteration:
+                        finished = True
+                else:
+                    # The command isn't in this group. If the command is in descendant
+                    # group, yield items from the group. If it's not a descendant, then
+                    # there are no more commands in this group; we can return to the
+                    # previous group for processing.
+                    subgroup = descendant(command.group, parent)
+                    if subgroup:
+                        yield from _section_break(subgroup)
+                        yield from _iter_group(subgroup)
+                    else:
+                        return
+
+        # Prime the initial command into the command iterator
+        try:
+            command = next(cmd_iter)
+        except StopIteration:
+            pass
+        else:
+            finished = False
+            yield from _iter_group(None)

core/tests/command/conftest.py

@@ -5,24 +5,34 @@
 
 @pytest.fixture
 def parent_group_1():
-    return toga.Group("P", order=1)
+    return toga.Group("P1", order=1)
 
 
 @pytest.fixture
 def child_group_1(parent_group_1):
-    return toga.Group("C", order=2, parent=parent_group_1)
+    return toga.Group("C1", order=2, parent=parent_group_1)
 
 
 @pytest.fixture
 def child_group_2(parent_group_1):
-    return toga.Group("B", order=4, parent=parent_group_1)
+    return toga.Group("C2", order=4, parent=parent_group_1)
 
 
 @pytest.fixture
 def parent_group_2():
-    return toga.Group("O", order=2)
+    return toga.Group("P2", order=2)
 
 
 @pytest.fixture
 def child_group_3(parent_group_2):
-    return toga.Group("A", order=2, parent=parent_group_2)
+    return toga.Group("C3", section=2, order=2, parent=parent_group_2)
+
+
+@pytest.fixture
+def child_group_4(parent_group_2):
+    return toga.Group("C4", section=2, order=1, parent=parent_group_2)
+
+
+@pytest.fixture
+def child_group_5(parent_group_2):
+    return toga.Group("C5", section=1, order=1, parent=parent_group_2)

core/tests/command/test_command.py

@@ -4,7 +4,7 @@
 import pytest
 
 import toga
-from toga.command import Break
+from toga.command import Separator
 from toga_dummy.utils import assert_action_performed_with
 
 
@@ -21,11 +21,28 @@ def assert_order(*items):
             assert not items[i] > 42
 
 
-def test_break():
-    """A break can be created"""
+def test_separator(parent_group_1):
+    """A separator can be created"""
 
-    example_break = Break("Example")
-    assert repr(example_break) == "<Example break>"
+    separator = Separator(group=parent_group_1)
+    assert repr(separator) == "<Separator group=P1>"
+
+
+def test_separator_eq(parent_group_1, parent_group_2):
+    """Separator objects can be compared for equality"""
+
+    separator_1a = Separator(parent_group_1)
+    separator_1b = Separator(parent_group_1)
+    separator_2 = Separator(parent_group_2)
+
+    # Separators are equal to breaks in the same section, but not to other
+    # sections
+    assert separator_1a == separator_1a
+    assert separator_1a == separator_1b
+    assert separator_1a != separator_2
+
+    # Separators aren't equal to non-separator objects
+    assert separator_1a != 3
 
 
 def test_create():