From bbccce43863c029192cfed467c43e776d6c00984 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sun, 26 Jan 2020 13:47:52 -0500 Subject: [PATCH 1/3] More rework of the class hierarchy of Registry items --- src/Menus.cpp | 24 +++++----------- src/commands/CommandManager.h | 53 ++++++++++++++++++++++++++--------- 2 files changed, 46 insertions(+), 31 deletions(-) diff --git a/src/Menus.cpp b/src/Menus.cpp index e0cb00994..795e70518 100644 --- a/src/Menus.cpp +++ b/src/Menus.cpp @@ -110,18 +110,8 @@ ComputedItem::~ComputedItem() {} SingleItem::~SingleItem() {} -GroupItem::GroupItem( const wxString &internalName, BaseItemPtrs &&items_ ) -: BaseItem{ internalName }, items{ std::move( items_ ) } -{ -} -void GroupItem::AppendOne( BaseItemPtr&& ptr ) -{ - items.push_back( std::move( ptr ) ); -} GroupItem::~GroupItem() {} -TransparentGroupItem::~TransparentGroupItem() {} - Visitor::~Visitor(){} void Visitor::BeginGroup(GroupItem &, const Path &) {} void Visitor::EndGroup(GroupItem &, const Path &) {} @@ -133,7 +123,7 @@ namespace MenuTable { MenuItem::MenuItem( const wxString &internalName, const TranslatableString &title_, BaseItemPtrs &&items_ ) -: GroupItem{ internalName, std::move( items_ ) }, title{ title_ } +: ConcreteGroupItem< false >{ internalName, std::move( items_ ) }, title{ title_ } { wxASSERT( !title.empty() ); } @@ -141,7 +131,7 @@ MenuItem::~MenuItem() {} ConditionalGroupItem::ConditionalGroupItem( const wxString &internalName, Condition condition_, BaseItemPtrs &&items_ ) -: GroupItem{ internalName, std::move( items_ ) }, condition{ condition_ } +: ConcreteGroupItem< false >{ internalName, std::move( items_ ) }, condition{ condition_ } { } ConditionalGroupItem::~ConditionalGroupItem() {} @@ -237,7 +227,7 @@ void CollectItem( AudacityProject &project, } else if (auto pGroup = dynamic_cast(pItem)) { - if (dynamic_cast(pItem) && pItem->name.empty()) + if (pGroup->Transparent() && pItem->name.empty()) // nameless grouping item is transparent to path calculations // recursion CollectItems( project, collection, pGroup->items ); @@ -384,8 +374,8 @@ struct MenuItemVisitor : Registry::Visitor flags.push_back(flag); } else - if (const auto pGroup = - dynamic_cast( pItem )) { + if (const auto pGroup = dynamic_cast( pItem )) { + wxASSERT( pGroup->Transparent() ); } else wxASSERT( false ); @@ -407,8 +397,8 @@ struct MenuItemVisitor : Registry::Visitor flags.pop_back(); } else - if (const auto pGroup = - dynamic_cast( pItem )) { + if (const auto pGroup = dynamic_cast( pItem )) { + wxASSERT( pGroup->Transparent() ); } else wxASSERT( false ); diff --git a/src/commands/CommandManager.h b/src/commands/CommandManager.h index 6fb6adf37..1ab03785c 100644 --- a/src/commands/CommandManager.h +++ b/src/commands/CommandManager.h @@ -473,17 +473,30 @@ namespace Registry { // Common abstract base class for items that group other items struct GroupItem : BaseItem { + using BaseItem::BaseItem; + // Construction from an internal name and a previously built-up // vector of pointers - GroupItem( const wxString &internalName, BaseItemPtrs &&items_ ); - // In-line, variadic constructor that doesn't require building a vector - template< typename... Args > - GroupItem( const wxString &internalName, Args&&... args ) - : BaseItem( internalName ) - { Append( std::forward< Args >( args )... ); } + GroupItem( const wxString &internalName, BaseItemPtrs &&items_ ) + : BaseItem{ internalName }, items{ std::move( items_ ) } + {} ~GroupItem() override = 0; + // Whether the item is non-significant for path naming + // when it also has an empty name + virtual bool Transparent() const = 0; + BaseItemPtrs items; + }; + + // GroupItem adding variadic constructor conveniences + struct InlineGroupItem : GroupItem { + using GroupItem::GroupItem; + // In-line, variadic constructor that doesn't require building a vector + template< typename... Args > + InlineGroupItem( const wxString &internalName, Args&&... args ) + : GroupItem( internalName ) + { Append( std::forward< Args >( args )... ); } private: // nullary overload grounds the recursion @@ -502,7 +515,10 @@ namespace Registry { }; // Move one unique_ptr to an item into our array - void AppendOne( BaseItemPtr&& ptr ); + void AppendOne( BaseItemPtr&& ptr ) + { + items.push_back( std::move( ptr ) ); + } // This overload allows a lambda or function pointer in the variadic // argument lists without any other syntactic wrapping, and also // allows implicit conversions to type Factory. @@ -517,12 +533,21 @@ namespace Registry { { AppendOne( std::make_unique(ptr) ); } }; + // Inline group item also specifying transparency + template< bool transparent > + struct ConcreteGroupItem : InlineGroupItem + { + using InlineGroupItem::InlineGroupItem; + ~ConcreteGroupItem() {} + bool Transparent() const override { return transparent; } + }; + // Concrete subclass of GroupItem that adds nothing else // TransparentGroupItem with an empty name is transparent to item path calculations - struct TransparentGroupItem final : GroupItem + struct TransparentGroupItem final : ConcreteGroupItem< true > { - using GroupItem::GroupItem; - ~TransparentGroupItem() override; + using ConcreteGroupItem< true >::ConcreteGroupItem; + ~TransparentGroupItem() override {} }; // Define actions to be done in Visit. @@ -548,7 +573,7 @@ namespace MenuTable { using namespace Registry; // Describes a main menu in the toolbar, or a sub-menu - struct MenuItem final : GroupItem { + struct MenuItem final : ConcreteGroupItem< false > { // Construction from an internal name and a previously built-up // vector of pointers MenuItem( const wxString &internalName, @@ -557,7 +582,7 @@ namespace MenuTable { template< typename... Args > MenuItem( const wxString &internalName, const TranslatableString &title_, Args&&... args ) - : GroupItem{ internalName, std::forward(args)... } + : ConcreteGroupItem< false >{ internalName, std::forward(args)... } , title{ title_ } {} ~MenuItem() override; @@ -567,7 +592,7 @@ namespace MenuTable { // Collects other items that are conditionally shown or hidden, but are // always available to macro programming - struct ConditionalGroupItem final : GroupItem { + struct ConditionalGroupItem final : ConcreteGroupItem< false > { using Condition = std::function< bool() >; // Construction from an internal name and a previously built-up @@ -578,7 +603,7 @@ namespace MenuTable { template< typename... Args > ConditionalGroupItem( const wxString &internalName, Condition condition_, Args&&... args ) - : GroupItem{ internalName, std::forward(args)... } + : ConcreteGroupItem< false >{ internalName, std::forward(args)... } , condition{ condition_ } {} ~ConditionalGroupItem() override; From c7bfa7a2a663fa95510893257b7a364715335756 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sun, 26 Jan 2020 15:43:55 -0500 Subject: [PATCH 2/3] Parametrize ComputedItem::Factory by the type of argument it expects --- src/Menus.cpp | 6 +++-- src/commands/CommandManager.h | 46 ++++++++++++++++++++++------------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/Menus.cpp b/src/Menus.cpp index 795e70518..43bebb97e 100644 --- a/src/Menus.cpp +++ b/src/Menus.cpp @@ -123,7 +123,8 @@ namespace MenuTable { MenuItem::MenuItem( const wxString &internalName, const TranslatableString &title_, BaseItemPtrs &&items_ ) -: ConcreteGroupItem< false >{ internalName, std::move( items_ ) }, title{ title_ } +: ConcreteGroupItem< false, MenuVisitor >{ + internalName, std::move( items_ ) }, title{ title_ } { wxASSERT( !title.empty() ); } @@ -131,7 +132,8 @@ MenuItem::~MenuItem() {} ConditionalGroupItem::ConditionalGroupItem( const wxString &internalName, Condition condition_, BaseItemPtrs &&items_ ) -: ConcreteGroupItem< false >{ internalName, std::move( items_ ) }, condition{ condition_ } +: ConcreteGroupItem< false, MenuVisitor >{ + internalName, std::move( items_ ) }, condition{ condition_ } { } ConditionalGroupItem::~ConditionalGroupItem() {} diff --git a/src/commands/CommandManager.h b/src/commands/CommandManager.h index 1ab03785c..7122e1085 100644 --- a/src/commands/CommandManager.h +++ b/src/commands/CommandManager.h @@ -454,15 +454,18 @@ namespace Registry { // Return type is a shared_ptr to let the function decide whether to // recycle the object or rebuild it on demand each time. // Return value from the factory may be null - using Factory = std::function< BaseItemSharedPtr( AudacityProject & ) >; + template< typename VisitorType > + using Factory = std::function< BaseItemSharedPtr( VisitorType & ) >; - explicit ComputedItem( const Factory &factory_ ) + using DefaultVisitor = AudacityProject; + + explicit ComputedItem( const Factory< DefaultVisitor > &factory_ ) : BaseItem( wxEmptyString ) , factory{ factory_ } {} ~ComputedItem() override; - Factory factory; + Factory< DefaultVisitor > factory; }; // Common abstract base class for items that are not groups @@ -490,6 +493,7 @@ namespace Registry { }; // GroupItem adding variadic constructor conveniences + template< typename VisitorType = ComputedItem::DefaultVisitor > struct InlineGroupItem : GroupItem { using GroupItem::GroupItem; // In-line, variadic constructor that doesn't require building a vector @@ -525,8 +529,10 @@ namespace Registry { // (Thus, a lambda can return a unique_ptr rvalue even though // Factory's return type is shared_ptr, and the needed conversion is // appled implicitly.) - void AppendOne( const ComputedItem::Factory &factory ) - { AppendOne( std::make_unique( factory ) ); } + void AppendOne( const ComputedItem::Factory &factory ) + { + AppendOne( std::make_unique( factory ) ); + } // This overload lets you supply a shared pointer to an item, directly template void AppendOne( const std::shared_ptr &ptr ) @@ -534,19 +540,21 @@ namespace Registry { }; // Inline group item also specifying transparency - template< bool transparent > - struct ConcreteGroupItem : InlineGroupItem + template< bool transparent, + typename VisitorType = ComputedItem::DefaultVisitor > + struct ConcreteGroupItem : InlineGroupItem< VisitorType > { - using InlineGroupItem::InlineGroupItem; + using InlineGroupItem< VisitorType >::InlineGroupItem; ~ConcreteGroupItem() {} bool Transparent() const override { return transparent; } }; // Concrete subclass of GroupItem that adds nothing else // TransparentGroupItem with an empty name is transparent to item path calculations - struct TransparentGroupItem final : ConcreteGroupItem< true > + template< typename VisitorType = ComputedItem::DefaultVisitor > + struct TransparentGroupItem final : ConcreteGroupItem< true, VisitorType > { - using ConcreteGroupItem< true >::ConcreteGroupItem; + using ConcreteGroupItem< true, VisitorType >::ConcreteGroupItem; ~TransparentGroupItem() override {} }; @@ -568,12 +576,14 @@ namespace Registry { Visitor &visitor, AudacityProject &project, BaseItem *pTopItem ); } +using MenuVisitor = Registry::ComputedItem::DefaultVisitor; + // Define items that populate tables that specifically describe menu trees namespace MenuTable { using namespace Registry; // Describes a main menu in the toolbar, or a sub-menu - struct MenuItem final : ConcreteGroupItem< false > { + struct MenuItem final : ConcreteGroupItem< false, MenuVisitor > { // Construction from an internal name and a previously built-up // vector of pointers MenuItem( const wxString &internalName, @@ -582,7 +592,8 @@ namespace MenuTable { template< typename... Args > MenuItem( const wxString &internalName, const TranslatableString &title_, Args&&... args ) - : ConcreteGroupItem< false >{ internalName, std::forward(args)... } + : ConcreteGroupItem< false, MenuVisitor >{ + internalName, std::forward(args)... } , title{ title_ } {} ~MenuItem() override; @@ -592,7 +603,7 @@ namespace MenuTable { // Collects other items that are conditionally shown or hidden, but are // always available to macro programming - struct ConditionalGroupItem final : ConcreteGroupItem< false > { + struct ConditionalGroupItem final : ConcreteGroupItem< false, MenuVisitor > { using Condition = std::function< bool() >; // Construction from an internal name and a previously built-up @@ -603,7 +614,8 @@ namespace MenuTable { template< typename... Args > ConditionalGroupItem( const wxString &internalName, Condition condition_, Args&&... args ) - : ConcreteGroupItem< false >{ internalName, std::forward(args)... } + : ConcreteGroupItem< false, MenuVisitor >{ + internalName, std::forward(args)... } , condition{ condition_ } {} ~ConditionalGroupItem() override; @@ -738,10 +750,10 @@ namespace MenuTable { // in identification of items by path. Otherwise try to keep the name // stable across Audacity versions. template< typename... Args > - inline std::unique_ptr Items( + inline std::unique_ptr > Items( const wxString &internalName, Args&&... args ) - { return std::make_unique( internalName, - std::forward(args)... ); } + { return std::make_unique >( + internalName, std::forward(args)... ); } // Menu items can be constructed two ways, as for group items // Items will appear in a main toolbar menu or in a sub-menu. From 103a6050a024ced2fb0b50eec06b8e5a7f0826a5 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sun, 26 Jan 2020 16:10:26 -0500 Subject: [PATCH 3/3] Registry::Visit doesn't require an AudacityProject --- src/Menus.cpp | 48 +++++++++++++++-------------------- src/Menus.h | 5 ++-- src/commands/CommandManager.h | 20 +++++++++++---- src/menus/HelpMenus.cpp | 8 +++--- 4 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/Menus.cpp b/src/Menus.cpp index 43bebb97e..a7ef2524d 100644 --- a/src/Menus.cpp +++ b/src/Menus.cpp @@ -195,15 +195,15 @@ struct CollectedItems // alternate as the entire tree is recursively visited. // forward declaration for mutually recursive functions -void CollectItem( AudacityProject &project, +void CollectItem( Registry::Visitor &visitor, CollectedItems &collection, BaseItem *Item ); -void CollectItems( AudacityProject &project, +void CollectItems( Registry::Visitor &visitor, CollectedItems &collection, const BaseItemPtrs &items ) { for ( auto &item : items ) - CollectItem( project, collection, item.get() ); + CollectItem( visitor, collection, item.get() ); } -void CollectItem( AudacityProject &project, +void CollectItem( Registry::Visitor &visitor, CollectedItems &collection, BaseItem *pItem ) { if (!pItem) @@ -214,17 +214,17 @@ void CollectItem( AudacityProject &project, dynamic_cast( pItem )) { auto &delegate = pShared->ptr; // recursion - CollectItem( project, collection, delegate.get() ); + CollectItem( visitor, collection, delegate.get() ); } else if (const auto pComputed = dynamic_cast( pItem )) { - auto result = pComputed->factory( project ); + auto result = pComputed->factory( visitor ); if (result) { // Guarantee long enough lifetime of the result collection.computedItems.push_back( result ); // recursion - CollectItem( project, collection, result.get() ); + CollectItem( visitor, collection, result.get() ); } } else @@ -232,7 +232,7 @@ void CollectItem( AudacityProject &project, if (pGroup->Transparent() && pItem->name.empty()) // nameless grouping item is transparent to path calculations // recursion - CollectItems( project, collection, pGroup->items ); + CollectItems( visitor, collection, pGroup->items ); else // all other group items collection.items.push_back( pItem ); @@ -248,29 +248,26 @@ using Path = std::vector< Identifier >; // forward declaration for mutually recursive functions void VisitItem( - Registry::Visitor &visitor, - AudacityProject &project, CollectedItems &collection, + Registry::Visitor &visitor, CollectedItems &collection, Path &path, BaseItem *pItem ); void VisitItems( - Registry::Visitor &visitor, - AudacityProject &project, CollectedItems &collection, + Registry::Visitor &visitor, CollectedItems &collection, Path &path, GroupItem *pGroup ) { // Make a new collection for this subtree, sharing the memo cache CollectedItems newCollection{ {}, collection.computedItems }; // Gather items at this level - CollectItems( project, newCollection, pGroup->items ); + CollectItems( visitor, newCollection, pGroup->items ); // Now visit them path.push_back( pGroup->name ); for ( const auto &pSubItem : newCollection.items ) - VisitItem( visitor, project, collection, path, pSubItem ); + VisitItem( visitor, collection, path, pSubItem ); path.pop_back(); } void VisitItem( - Registry::Visitor &visitor, - AudacityProject &project, CollectedItems &collection, + Registry::Visitor &visitor, CollectedItems &collection, Path &path, BaseItem *pItem ) { if (!pItem) @@ -285,7 +282,7 @@ void VisitItem( dynamic_cast( pItem )) { visitor.BeginGroup( *pGroup, path ); // recursion - VisitItems( visitor, project, collection, path, pGroup ); + VisitItems( visitor, collection, path, pGroup ); visitor.EndGroup( *pGroup, path ); } else @@ -296,14 +293,12 @@ void VisitItem( namespace Registry { -void Visit( - Visitor &visitor, AudacityProject &project, - BaseItem *pTopItem ) +void Visit( Visitor &visitor, BaseItem *pTopItem ) { std::vector< BaseItemSharedPtr > computedItems; CollectedItems collection{ {}, computedItems }; Path emptyPath; - VisitItem( visitor, project, collection, emptyPath, pTopItem ); + VisitItem( visitor, collection, emptyPath, pTopItem ); } } @@ -354,10 +349,10 @@ static const auto menuTree = MenuTable::Items( MenuPathStart ); namespace { -struct MenuItemVisitor : Registry::Visitor +struct MenuItemVisitor : MenuVisitor { MenuItemVisitor( AudacityProject &proj, CommandManager &man ) - : project(proj), manager( man ) {} + : MenuVisitor(proj), manager( man ) {} void BeginGroup( GroupItem &item, const Path& ) override { @@ -440,7 +435,6 @@ struct MenuItemVisitor : Registry::Visitor wxASSERT( false ); } - AudacityProject &project; CommandManager &manager; std::vector flags; }; @@ -458,7 +452,7 @@ void MenuCreator::CreateMenusAndCommands(AudacityProject &project) wxASSERT(menubar); MenuItemVisitor visitor{ project, commandManager }; - MenuManager::Visit( visitor, project ); + MenuManager::Visit( visitor ); GetProjectFrame( project ).SetMenuBar(menubar.release()); @@ -469,9 +463,9 @@ void MenuCreator::CreateMenusAndCommands(AudacityProject &project) #endif } -void MenuManager::Visit( Registry::Visitor &visitor, AudacityProject &project ) +void MenuManager::Visit( MenuVisitor &visitor ) { - Registry::Visit( visitor, project, menuTree.get() ); + Registry::Visit( visitor, menuTree.get() ); } // TODO: This surely belongs in CommandManager? diff --git a/src/Menus.h b/src/Menus.h index 65d637ab3..232757659 100644 --- a/src/Menus.h +++ b/src/Menus.h @@ -54,6 +54,8 @@ public: PluginID mLastEffect{}; }; +struct MenuVisitor; + class MenuManager final : public MenuCreator , public ClientData::Base @@ -70,8 +72,7 @@ public: MenuManager &operator=( const MenuManager & ) PROHIBITED; ~MenuManager(); - static void Visit( - Registry::Visitor &visitor, AudacityProject &project ); + static void Visit( MenuVisitor &visitor ); static void ModifyUndoMenuItems(AudacityProject &project); static void ModifyToolbarMenus(AudacityProject &project); diff --git a/src/commands/CommandManager.h b/src/commands/CommandManager.h index 7122e1085..c080d3bc2 100644 --- a/src/commands/CommandManager.h +++ b/src/commands/CommandManager.h @@ -427,6 +427,8 @@ namespace Registry { using BaseItemPtr = std::unique_ptr; using BaseItemSharedPtr = std::shared_ptr; using BaseItemPtrs = std::vector; + + struct Visitor; // An item that delegates to another held in a shared pointer; this allows @@ -457,7 +459,7 @@ namespace Registry { template< typename VisitorType > using Factory = std::function< BaseItemSharedPtr( VisitorType & ) >; - using DefaultVisitor = AudacityProject; + using DefaultVisitor = Visitor; explicit ComputedItem( const Factory< DefaultVisitor > &factory_ ) : BaseItem( wxEmptyString ) @@ -531,7 +533,10 @@ namespace Registry { // appled implicitly.) void AppendOne( const ComputedItem::Factory &factory ) { - AppendOne( std::make_unique( factory ) ); + auto adaptedFactory = [factory]( Registry::Visitor &visitor ){ + return factory( dynamic_cast< VisitorType& >( visitor ) ); + }; + AppendOne( std::make_unique( adaptedFactory ) ); } // This overload lets you supply a shared pointer to an item, directly template @@ -572,11 +577,16 @@ namespace Registry { }; // Top-down visitation of all items and groups in a tree - void Visit( - Visitor &visitor, AudacityProject &project, BaseItem *pTopItem ); + void Visit( Visitor &visitor, BaseItem *pTopItem ); } -using MenuVisitor = Registry::ComputedItem::DefaultVisitor; +struct MenuVisitor : Registry::Visitor +{ + explicit MenuVisitor( AudacityProject &p ) : project{ p } {} + operator AudacityProject & () const { return project; } + + AudacityProject &project; +}; // Define items that populate tables that specifically describe menu trees namespace MenuTable { diff --git a/src/menus/HelpMenus.cpp b/src/menus/HelpMenus.cpp index c9a682862..e8831d682 100644 --- a/src/menus/HelpMenus.cpp +++ b/src/menus/HelpMenus.cpp @@ -380,8 +380,10 @@ void OnMenuTree(const CommandContext &context) auto &project = context.project; using namespace MenuTable; - struct MyVisitor : Visitor + struct MyVisitor : MenuVisitor { + using MenuVisitor::MenuVisitor; + enum : unsigned { TAB = 3 }; void BeginGroup( GroupItem &item, const Path& ) override { @@ -415,9 +417,9 @@ void OnMenuTree(const CommandContext &context) unsigned level{}; wxString indentation; wxString info; - } visitor; + } visitor{ project }; - MenuManager::Visit( visitor, project ); + MenuManager::Visit( visitor ); ShowDiagnostics( project, visitor.info, XO("Menu Tree"), wxT("menutree.txt"), true );