Type checking when building registry trees, except...

... GroupItemBase::push_back is still a hole in the defenses

libraries/lib-registries/Registry.h

@@ -80,9 +80,9 @@ namespace detail {
 
       OrderingHint orderingHint;
    };
-}
 
    using BaseItemPtr = std::unique_ptr<detail::BaseItem>;
+}
 
    class Visitor;
 
@@ -236,7 +236,7 @@ namespace detail {
 
    private:
       friend REGISTRIES_API void detail::RegisterItem(GroupItemBase &registry,
-         const Placement &placement, BaseItemPtr pItem);
+         const Placement &placement, detail::BaseItemPtr pItem);
    };
 
    // Forward declarations necessary before customizing Composite::Traits
@@ -252,11 +252,12 @@ template<typename RegistryTraits> struct Composite::Traits<
    static constexpr auto ItemBuilder =
    Registry::detail::Builder<RegistryTraits>{};
    //! Prohibit pushing-back of type-unchecked BaseItemPtr directly
-   template<typename T> static constexpr auto enables_item_type_v = true;
+   template<typename T> static constexpr auto enables_item_type_v =
+      !std::is_same_v<T, Registry::detail::BaseItemPtr>;
 };
 
 namespace Registry {
-   //! Extends GroupItemBase with a variadic constructor
+   //! Extends GroupItemBase with a variadic constructor that checks types
    /*!
     @tparam RegistryTraits defines associated types
     */
@@ -313,6 +314,7 @@ namespace detail {
 
       template<typename ItemType>
       auto operator () (std::unique_ptr<ItemType> ptr) const {
+         static_assert(AcceptableType_v<RegistryTraits, ItemType>);
          return move(ptr);
       }
       //! This overload allows a lambda or function pointer in the variadic
@@ -322,11 +324,13 @@ namespace detail {
          typename std::invoke_result_t<Factory, Context&>::element_type
       >
       auto operator () (const Factory &factory) const {
+         static_assert(AcceptableType_v<RegistryTraits, ItemType>);
          return std::make_unique<ComputedItem<Context, ItemType>>(factory);
       }
       //! This overload lets you supply a shared pointer to an item, directly
       template<typename ItemType>
       auto operator () (const std::shared_ptr<ItemType> &ptr) const {
+         static_assert(AcceptableType_v<RegistryTraits, ItemType>);
          return std::make_unique<IndirectItem<ItemType>>(ptr);
       }
    };

libraries/lib-snapping/SnapUtils.cpp

@@ -336,15 +336,15 @@ class ProjectDependentMultiplierSnapItem final : public SnapRegistryItem
 
 }
 
-Registry::BaseItemPtr TimeInvariantSnapFunction(
+std::unique_ptr<SnapRegistryItem> TimeInvariantSnapFunction(
    const Identifier& functionId, const TranslatableString& label,
    MultiplierFunctor functor)
 {
    return std::make_unique<ProjectDependentMultiplierSnapItem>(
       functionId, label, std::move(functor));
 }
 
-Registry::BaseItemPtr TimeInvariantSnapFunction(
+std::unique_ptr<SnapRegistryItem> TimeInvariantSnapFunction(
    const Identifier& functionId, const TranslatableString& label,
    double multiplier)
 {

libraries/lib-snapping/SnapUtils.h

@@ -91,13 +91,13 @@ struct SNAPPING_API SnapRegistryItem : public Registry::SingleItem
    virtual SnapResult SingleStep(const AudacityProject& project, double time, bool upwards) const = 0;
 };
 
-SNAPPING_API Registry::BaseItemPtr TimeInvariantSnapFunction(
+std::unique_ptr<SnapRegistryItem> TimeInvariantSnapFunction(
    const Identifier& functionId, const TranslatableString& label,
    double multiplier);
 
 using MultiplierFunctor = std::function<double(const AudacityProject&)>;
 
-SNAPPING_API Registry::BaseItemPtr TimeInvariantSnapFunction(
+std::unique_ptr<SnapRegistryItem> TimeInvariantSnapFunction(
    const Identifier& functionId, const TranslatableString& label,
    MultiplierFunctor functor);
 

src/commands/CommandManager.h

@@ -427,14 +427,17 @@ namespace MenuTable {
 
    struct Traits;
    struct CommandItem;
+   struct CommandGroupItem;
    struct ConditionalGroupItem;
    struct MenuItem;
    struct MenuItems;
+   struct SpecialItem;
    struct MenuPart;
 
    struct Traits : Registry::DefaultTraits {
       using ComputedItemContextType = AudacityProject;
-      using LeafTypes = List<CommandItem>;
+      using LeafTypes = List<
+         CommandItem, CommandGroupItem, SpecialItem>;
       using NodeTypes = List<
          ConditionalGroupItem, MenuItem, MenuItems, MenuPart>;
    };

src/menus/MenuHelper.cpp

@@ -254,16 +254,6 @@ void AddSortedEffectMenuItems(
    }
 }
 
-auto MenuOrItems(const TranslatableString &label)
-   -> std::unique_ptr<MenuHelper::Group>
-{
-   using namespace MenuTable;
-   if (label.empty())
-      return Items("");
-   else
-      return Menu("", label);
-}
-
 auto MakeAddGroupItems(
    const EffectsMenuGroups& list,
    CommandFlag batchflags,
@@ -301,11 +291,21 @@ auto MakeAddGroupItems(
 
          if (!groupNames.empty())
          {
-            auto temp = MenuOrItems(p.first);
-            AddEffectMenuItemGroup(*temp,
-               groupNames, groupPlugs, groupFlags,
-               onMenuCommand);
-            items.push_back(move(temp));
+            using namespace MenuTable;
+            if (p.first.empty()) {
+               auto temp = Items("");
+               AddEffectMenuItemGroup(*temp,
+                  groupNames, groupPlugs, groupFlags,
+                  onMenuCommand);
+               items.push_back(move(temp));
+            }
+            else {
+               auto temp = Menu("", p.first);
+               AddEffectMenuItemGroup(*temp,
+                  groupNames, groupPlugs, groupFlags,
+                  onMenuCommand);
+               items.push_back(move(temp));
+            }
          }
       }
    };
@@ -332,13 +332,22 @@ void AddGroupedEffectMenuItems(
 
    auto doAddGroup = [&]
    {
+      using namespace MenuTable;
       if(names.empty())
          return;
 
       const auto inSubmenu = !path.empty() && (names.size() > 1);
-      auto items = MenuOrItems(inSubmenu ? path.back() : TranslatableString{});
-      AddEffectMenuItemGroup(*items, names, group, flags, onMenuCommand);
-      parentTable->push_back(move(items));
+      const auto label = inSubmenu ? path.back() : TranslatableString{};
+      if (label.empty()) {
+         auto items = Items("");
+         AddEffectMenuItemGroup(*items, names, group, flags, onMenuCommand);
+         parentTable->push_back(move(items));
+      }
+      else {
+         auto items = Menu("", label);
+         AddEffectMenuItemGroup(*items, names, group, flags, onMenuCommand);
+         parentTable->push_back(move(items));
+      }
 
       names.clear();
       group.clear();
@@ -753,16 +762,19 @@ void MenuHelper::PopulateEffectsMenu(
       if(section.compare != nullptr)
          std::sort(section.plugins.begin(), section.plugins.end(), section.compare);
 
-      std::unique_ptr<Group> group;
-      if (menuItems.empty())
-         group = MenuTable::Items("");
-      else
-         group = MenuTable::Section("");
-      section.add(*group, section.plugins);
-
-      if (group->empty())
-         continue;
-
-      menuItems.push_back(move(group));
+      if (menuItems.empty()) {
+         auto group = MenuTable::Items("");
+         section.add(*group, section.plugins);
+         if (group->empty())
+            continue;
+         menuItems.push_back(move(group));
+      }
+      else {
+         auto group = MenuTable::Section("");
+         section.add(*group, section.plugins);
+         if (group->empty())
+            continue;
+         menuItems.push_back(move(group));
+      }
    }
 }

src/widgets/PopupMenuTable.h

@@ -227,7 +227,7 @@ class AUDACITY_DLL_API PopupMenuTable : public PopupMenuHandler
    void BeginSection( const Identifier &name );
    void EndSection();
 
-   std::shared_ptr<PopupMenuGroupItem> mTop;
+   std::shared_ptr<PopupSubMenu> mTop;
    std::vector<PopupMenuGroupItem*> mStack;
    Identifier mId;
    TranslatableString mCaption;