Define the registry merging procedure...

... before we populate the registry. This could apply to menu items, or more generally to other registries. A registry is a tree of items identified by path names. Various code, that need not coordinate, can specify items to attach to the tree, and the merging procedure collects them into a single tree that can be visited. Pathnames imply only an unordered tree. Some visitation ordering must be imposed on the nodes, and can be remembered in preferences for stability between runs, independently of accidents of the unspecified sequence of initialization of file-scope static objects in the various plug-ins. It can be arbitrary -- not constrained to some fixed intrinsic criterion like alphabetical order. Merging consults the preferences, and also updates them if previously unknown items are found and inserted. For now, such unknowns just go to the end of the sequence of siblings, sorted by their path component names.
2025-07-30 15:39:27 +02:00 · 2019-01-02 01:34:48 -05:00 · 2019-01-02 01:34:48 -05:00 · 564a3ac708
commit 564a3ac708
parent 9d7f151cf6
2 changed files with 382 additions and 23 deletions
--- a/src/Menus.cpp
+++ b/src/Menus.cpp
@ -40,8 +40,11 @@
 #include "commands/CommandManager.h"
 #include "prefs/TracksPrefs.h"
 #include "toolbars/ToolManager.h"
+#include "widgets/AudacityMessageBox.h"
 #include "widgets/ErrorDialog.h"

+#include <unordered_set>
+
 #include <wx/menu.h>
 #include <wx/windowptr.h>

@ -179,11 +182,48 @@ namespace {

 const auto MenuPathStart = wxT("MenuBar");

-using namespace MenuTable;
+struct ItemOrdering;
+
+struct OrderingHint{}; // to be defined in a later commit
+
+using namespace Registry;
 struct CollectedItems
 {
-   std::vector< BaseItem * > items;
+   struct Item{
+      // Predefined, or merged from registry already:
+      BaseItem *visitNow;
+      // Corresponding item from the registry, its sub-items to be merged:
+      GroupItem *mergeLater;
+   };
+   std::vector< Item > items;
   std::vector< BaseItemSharedPtr > &computedItems;
+
+   // A linear search.  Smarter search may not be worth the effort.
+   using Iterator = decltype( items )::iterator;
+   auto Find( const Identifier &name ) -> Iterator
+   {
+      auto end = items.end();
+      return name.empty()
+         ? end
+         : std::find_if( items.begin(), end,
+            [&]( const Item& item ){
+               return name == item.visitNow->name; } );
+   }
+
+   auto InsertNewItemUsingPreferences(
+      ItemOrdering &itemOrdering, BaseItem *pItem ) -> bool;
+
+   auto InsertNewItemUsingHint(
+      BaseItem *pItem, const OrderingHint &hint, bool force ) -> bool;
+
+   auto SubordinateSingleItem( Item &found, BaseItem *pItem ) -> void;
+
+   auto MergeItems(
+      Visitor &visitor, ItemOrdering &itemOrdering,
+      const BaseItemPtrs &toMerge ) -> void;
+
+   auto MergeItem(
+      Visitor &visitor, ItemOrdering &itemOrdering, BaseItem *pItem ) -> bool;
 };

 // "Collection" of items is the first pass of visitation, and resolves
@ -207,12 +247,13 @@ void CollectItem( Registry::Visitor &visitor,
   if (!pItem)
      return;

-   using namespace MenuTable;
+   using namespace Registry;
   if (const auto pShared =
       dynamic_cast<SharedItem*>( pItem )) {
-      auto &delegate = pShared->ptr;
-      // recursion
-      CollectItem( visitor, collection, delegate.get() );
+      auto delegate = pShared->ptr.get();
+      if ( delegate )
+         // recursion
+         CollectItem( visitor, collection, delegate );
   }
   else
   if (const auto pComputed =
@ -229,44 +270,337 @@ void CollectItem( Registry::Visitor &visitor,
   if (auto pGroup = dynamic_cast<GroupItem*>(pItem)) {
      if (pGroup->Transparent() && pItem->name.empty())
         // nameless grouping item is transparent to path calculations
+         // collect group members now
         // recursion
         CollectItems( visitor, collection, pGroup->items );
      else
         // all other group items
-         collection.items.push_back( pItem );
+         // defer collection of members until collecting at next lower level
+         collection.items.push_back( {pItem, nullptr} );
   }
   else {
      wxASSERT( dynamic_cast<SingleItem*>(pItem) );
      // common to all single items
-      collection.items.push_back( pItem );
+      collection.items.push_back( {pItem, nullptr} );
   }
 }

 using Path = std::vector< Identifier >;

+namespace {
+   std::unordered_set< wxString > sBadPaths;
+   void BadPath(
+     const TranslatableString &format, const wxString &key, const Identifier &name )
+   {
+     // Warn, but not more than once in a session for each bad path
+     auto badPath = key + '/' + name.GET();
+     if ( sBadPaths.insert( badPath ).second ) {
+        auto msg = TranslatableString{ format }.Format( badPath );
+        // debug message
+        wxLogDebug( msg.Translation() );
+#ifdef IS_ALPHA
+        // user-visible message
+        AudacityMessageBox( msg );
+#endif
+     }
+   }
+
+   void ReportGroupGroupCollision( const wxString &key, const Identifier &name )
+   {
+      BadPath(
+XO("Plug-in group at %s was merged with a previously defined group"),
+         key, name);
+   }
+
+   void ReportItemItemCollision( const wxString &key, const Identifier &name )
+   {
+      BadPath(
+XO("Plug-in item at %s conflicts with a previously defined item and was discarded"),
+         key, name);
+   }
+}
+
+struct ItemOrdering {
+   wxString key;
+
+   ItemOrdering( const Path &path )
+   {
+      // The set of path names determines only an unordered tree.
+      // We want an ordering of the tree that is stable across runs.
+      // The last used ordering for this node can be found in preferences at this
+      // key:
+      wxArrayString strings;
+      for (const auto &id : path)
+         strings.push_back( id.GET() );
+      key = '/' + ::wxJoin( strings, '/', '\0' );
+   }
+
+   // Retrieve the old ordering on demand, if needed to merge something.
+   bool gotOrdering = false;
+   wxString strValue;
+   wxArrayString ordering;
+
+   auto Get() -> wxArrayString & {
+      if ( !gotOrdering ) {
+         gPrefs->Read(key, &strValue);
+         ordering = ::wxSplit( strValue, ',' );
+         gotOrdering = true;
+      }
+      return ordering;
+   };
+};
+
+auto CollectedItems::InsertNewItemUsingPreferences(
+   ItemOrdering &itemOrdering, BaseItem *pItem )
+   -> bool
+{
+   // Note that if more than one plug-in registers items under the same
+   // node, then it is not specified which plug-in is handled first,
+   // the first time registration happens.  It might happen that you
+   // add a plug-in, run the program, then add another, then run again;
+   // registration order determined by those actions might not
+   // correspond to the order of re-loading of modules in later
+   // sessions.  But whatever ordering is chosen the first time some
+   // plug-in is seen -- that ordering gets remembered in preferences.
+
+   if ( !pItem->name.empty() ) {
+      // Check saved ordering first, and rebuild that as well as is possible
+      auto &ordering = itemOrdering.Get();
+      auto begin2 = ordering.begin(), end2 = ordering.end(),
+         found2 = std::find( begin2, end2, pItem->name );
+      if ( found2 != end2 ) {
+         auto insertPoint = items.end();
+         // Find the next name in the saved ordering that is known already
+         // in the collection.
+         while ( ++found2 != end2 ) {
+            auto known = Find( *found2 );
+            if ( known != insertPoint ) {
+               insertPoint = known;
+               break;
+            }
+         }
+         items.insert( insertPoint, {pItem, nullptr} );
+         return true;
+      }
+   }
+
+   return false;
+}
+
+auto CollectedItems::InsertNewItemUsingHint(
+   BaseItem *pItem, const OrderingHint &hint, bool force ) -> bool
+{
+   // To do, implement ordering hints
+   // For now, just put all at the end, sorted by name
+   auto insertPoint = items.end();
+   items.insert( insertPoint, {pItem, nullptr} );
+   return true;
+}
+
+auto CollectedItems::SubordinateSingleItem( Item &found, BaseItem *pItem )
+   -> void
+{
+   auto subGroup = std::make_shared<TransparentGroupItem<>>( pItem->name,
+      std::make_unique<SharedItem>(
+         // shared pointer with vacuous deleter
+         std::shared_ptr<BaseItem>( pItem, [](void*){} ) ) );
+   found.mergeLater = subGroup.get();
+   computedItems.push_back( subGroup );
+}
+
+auto CollectedItems::MergeItem(
+   Visitor &visitor, ItemOrdering &itemOrdering, BaseItem *pItem ) -> bool
+{
+   // Assume no null pointers in the registry
+   const auto &name = pItem->name;
+   auto found = Find( name );
+   if (found != items.end()) {
+      // Collision of names between collection and registry!
+      // There are 2 * 2 = 4 cases, as each of the two are group items or
+      // not.
+      auto pCollectionGroup = dynamic_cast< GroupItem * >( found->visitNow );
+      auto pRegistryGroup = dynamic_cast< GroupItem * >( pItem );
+      if (pCollectionGroup) {
+         if (pRegistryGroup) {
+            // This is the expected case of collision.
+            // Subordinate items from one of the groups will be merged in
+            // another call to MergeItems at a lower level of path.
+            // Note, however, that at most one of the two should be other
+            // than a plain grouping item; if not, we must lose the extra
+            // information carried by one of them.
+            bool pCollectionGrouping = pCollectionGroup->Transparent();
+            auto pRegistryGrouping = pRegistryGroup->Transparent();
+            if ( !(pCollectionGrouping || pRegistryGrouping) )
+               ReportGroupGroupCollision( itemOrdering.key, name );
+
+            if ( pCollectionGrouping && !pRegistryGrouping ) {
+               // Swap their roles
+               found->visitNow = pRegistryGroup;
+               found->mergeLater = pCollectionGroup;
+            }
+            else
+               found->mergeLater = pRegistryGroup;
+         }
+         else {
+            // Registered non-group item collides with a previously defined
+            // group.
+            // Resolve this by subordinating the non-group item below
+            // that group.
+            SubordinateSingleItem( *found, pItem );
+         }
+      }
+      else {
+         if (pRegistryGroup) {
+            // Subordinate the previously merged single item below the
+            // newly merged group.
+            // In case the name occurred in two different static registries,
+            // the final merge is the same, no matter which is treated first.
+            auto demoted = found->visitNow;
+            found->visitNow = pRegistryGroup;
+            SubordinateSingleItem( *found, demoted );
+         }
+         else
+            // Collision of non-group items is the worst case!
+            // The later-registered item is lost.
+            // Which one you lose might be unpredictable when both originate
+            // from static registries.
+            ReportItemItemCollision( itemOrdering.key, name );
+      }
+      return true;
+   }
+   else
+      // A name is registered that is not known in the collection.
+      return false;
+}
+
+auto CollectedItems::MergeItems(
+  Visitor &visitor, ItemOrdering &itemOrdering, const BaseItemPtrs &toMerge )
+   -> void
+{
+   // First do expansion of nameless groupings, and caching of computed
+   // items, just as for the previously defined menus.
+   CollectedItems newCollection{ {}, computedItems };
+   CollectItems( visitor, newCollection, toMerge );
+
+   // Try to merge each, resolving name collisions with items already in the
+   // tree, and collecting those with names that don't collide.
+   using NewItem = std::pair< BaseItem*, OrderingHint >;
+   std::vector< NewItem > newItems;
+   for ( const auto &item : newCollection.items )
+      if ( !MergeItem( visitor, itemOrdering, item.visitNow ) )
+          newItems.push_back( { item.visitNow, {} } );
+
+   // There may still be unresolved name collisions among the NEW items,
+   // so first find their sorted order.
+   static auto majorComp = [](const NewItem &a, const NewItem &b) {
+      // Descending sort!
+      return a.first->name > b.first->name;
+   };
+   std::sort( newItems.begin(), newItems.end(), majorComp );
+
+   // Choose placements for items with NEW names.
+   // Outer loop over trial passes.
+   int iPass = 0;
+   while( !newItems.empty() ) {
+      // Inner loop over ranges of like-named items.
+      // Do it right to left, to shrink array faster and avoid invalidating rend.
+      auto right = newItems.rbegin();
+      auto rend = newItems.rend();
+      bool forceNext = true;
+      while ( right != rend ) {
+         // Find the range
+         using namespace std::placeholders;
+         auto left = std::find_if(
+            right + 1, rend, std::bind( majorComp, _1, *right ) );
+
+         // Try to place the first item of the range.
+         // If such an item is a group, then we always retain the kind of
+         // grouping that was registered.  (Which doesn't always happen when
+         // there is name collision in MergeItem.)
+         auto iter = left.base();
+         auto &item = *iter;
+         auto pItem = item.first;
+         const auto &hint = item.second;
+         bool success = true;
+         if ( iPass == 0 )
+            // A first pass consults preferences.
+            success = InsertNewItemUsingPreferences( itemOrdering, pItem );
+         else {
+            // Later passes for choosing placements.
+            bool forceNow = iPass == -1;
+            success = InsertNewItemUsingHint( pItem, hint, forceNow );
+            wxASSERT( !forceNow || success );
+            // While some progress is made, don't force final placements.
+            if ( success )
+               forceNext = false;
+         }
+
+         if ( success ) {
+            // Resolve collisions among remaining like-named items.
+            ++iter;
+            while ( iter != right.base() )
+               // Re-invoke MergeItem for this item, which is known to have a name
+               // collision, so ignore the return value.
+               MergeItem( visitor, itemOrdering, iter++ -> first );
+            newItems.erase( left.base(), right.base() );
+         }
+
+         right = left;
+      }
+      iPass = forceNext ? -1 : 1;
+   }
+}
+
 // forward declaration for mutually recursive functions
 void VisitItem(
   Registry::Visitor &visitor, CollectedItems &collection,
-   Path &path, BaseItem *pItem );
+   Path &path, BaseItem *pItem, GroupItem *pToMerge, bool &doFlush );
 void VisitItems(
   Registry::Visitor &visitor, CollectedItems &collection,
-   Path &path, GroupItem *pGroup )
+   Path &path, GroupItem *pGroup, GroupItem *pToMerge, bool &doFlush )
 {
-   // Make a new collection for this subtree, sharing the memo cache
+   // Make a NEW collection for this subtree, sharing the memo cache
   CollectedItems newCollection{ {}, collection.computedItems };

   // Gather items at this level
   CollectItems( visitor, newCollection, pGroup->items );

+   path.push_back( pGroup->name.GET() );
+
+   // Merge with the registry
+   if ( pToMerge )
+   {
+      ItemOrdering itemOrdering{ path };
+      newCollection.MergeItems( visitor, itemOrdering, pToMerge->items );
+
+      // Remember the NEW ordering, if there was any need to use the old.
+      // This makes a side effect in preferences.
+      if ( itemOrdering.gotOrdering ) {
+         wxString newValue;
+         for ( const auto &item : newCollection.items ) {
+            const auto &name = item.visitNow->name;
+            if ( !name.empty() )
+               newValue += newValue.empty()
+                  ? name.GET()
+                  : ',' + name.GET();
+         }
+         if (newValue != itemOrdering.strValue) {
+            gPrefs->Write( itemOrdering.key, newValue );
+            doFlush = true;
+         }
+      }
+   }
+
   // Now visit them
-   path.push_back( pGroup->name );
-   for ( const auto &pSubItem : newCollection.items )
-      VisitItem( visitor, collection, path, pSubItem );
+   for ( const auto &item : newCollection.items )
+      VisitItem( visitor, collection, path, item.visitNow, item.mergeLater,
+         doFlush );
+
   path.pop_back();
 }
 void VisitItem(
   Registry::Visitor &visitor, CollectedItems &collection,
-   Path &path, BaseItem *pItem )
+   Path &path, BaseItem *pItem, GroupItem *pToMerge, bool &doFlush )
 {
   if (!pItem)
      return;
@ -280,7 +614,7 @@ void VisitItem(
       dynamic_cast<GroupItem*>( pItem )) {
      visitor.BeginGroup( *pGroup, path );
      // recursion
-      VisitItems( visitor, collection, path, pGroup );
+      VisitItems( visitor, collection, path, pGroup, pToMerge, doFlush );
      visitor.EndGroup( *pGroup, path );
   }
   else
@ -291,12 +625,17 @@ void VisitItem(

 namespace Registry {

-void Visit( Visitor &visitor,  BaseItem *pTopItem )
+void Visit( Visitor &visitor, BaseItem *pTopItem, GroupItem *pRegistry )
 {
   std::vector< BaseItemSharedPtr > computedItems;
+   bool doFlush = false;
   CollectedItems collection{ {}, computedItems };
   Path emptyPath;
-   VisitItem( visitor, collection, emptyPath, pTopItem );
+   VisitItem(
+      visitor, collection, emptyPath, pTopItem, pRegistry, doFlush );
+   // Flush any writes done by MergeItems()
+   if (doFlush)
+      gPrefs->Flush();
 }

 }
@ -328,6 +667,14 @@ MenuTable::BaseItemSharedPtr ExtraMenu();

 MenuTable::BaseItemSharedPtr HelpMenu();

+namespace {
+static Registry::GroupItem &sRegistry()
+{
+   static Registry::TransparentGroupItem<> registry{ MenuPathStart };
+   return registry;
+}
+}
+
 // Table of menu factories.
 // TODO:  devise a registration system instead.
 static const auto menuTree = MenuTable::Items( MenuPathStart
@ -347,6 +694,7 @@ static const auto menuTree = MenuTable::Items( MenuPathStart
 );

 namespace {
+using namespace MenuTable;
 struct MenuItemVisitor : MenuVisitor
 {
   MenuItemVisitor( AudacityProject &proj, CommandManager &man )
@ -465,7 +813,7 @@ void MenuCreator::CreateMenusAndCommands(AudacityProject &project)

 void MenuManager::Visit( MenuVisitor &visitor )
 {
-   Registry::Visit( visitor, menuTree.get() );
+   Registry::Visit( visitor, menuTree.get(), &sRegistry() );
 }

 // TODO: This surely belongs in CommandManager?
--- a/src/commands/CommandManager.h
+++ b/src/commands/CommandManager.h
@ -482,7 +482,7 @@ namespace Registry {

      // Construction from an internal name and a previously built-up
      // vector of pointers
-      GroupItem( const wxString &internalName, BaseItemPtrs &&items_ )
+      GroupItem( const Identifier &internalName, BaseItemPtrs &&items_ )
         : BaseItem{ internalName }, items{ std::move( items_ ) }
      {}
      ~GroupItem() override = 0;
@ -500,7 +500,7 @@ namespace Registry {
      using GroupItem::GroupItem;
      // In-line, variadic constructor that doesn't require building a vector
      template< typename... Args >
-         InlineGroupItem( const wxString &internalName, Args&&... args )
+         InlineGroupItem( const Identifier &internalName, Args&&... args )
         : GroupItem( internalName )
         { Append( std::forward< Args >( args )... ); }

@ -576,8 +576,19 @@ namespace Registry {
      virtual void Visit( SingleItem &item, const Path &path );
   };

-   // Top-down visitation of all items and groups in a tree
-   void Visit( Visitor &visitor, BaseItem *pTopItem );
+   // Top-down visitation of all items and groups in a tree rooted in
+   // pTopItem, as merged with pRegistry.
+   // The merger of the trees is recomputed in each call, not saved.
+   // So neither given tree is modified.
+   // But there may be a side effect on preferences to remember the ordering
+   // imposed on each node of the unordered tree of registered items; each item
+   // seen in the registry for the first time is placed somehere, and that
+   // ordering should be kept the same thereafter in later runs (which may add
+   // yet other previously unknown items).
+   void Visit(
+      Visitor &visitor,
+      BaseItem *pTopItem,
+      GroupItem *pRegistry = nullptr );
 }

 struct MenuVisitor : Registry::Visitor