From 4d99112056410762c720e8dfe2c859ff84c4a8b3 Mon Sep 17 00:00:00 2001 From: Guillaume Munch Date: Wed, 3 Aug 2016 22:06:20 +0100 Subject: [PATCH] Action.cpp: replace a reference with a shared_ptr Replace the member reference to FuncRequest in Action.cpp with a shared_ptr. Compared to copying the FuncRequest, the shared_ptr has two advantages: * Recreating the menu each time creates a lot of new actions, so we avoid a lot of copies. * FuncRequest can remain forward-declared in Action.h. --- src/frontends/qt4/Action.cpp | 28 +++++++++--- src/frontends/qt4/Action.h | 15 +++++-- src/frontends/qt4/GuiToolbar.cpp | 14 +++--- src/frontends/qt4/Menus.cpp | 74 +++++++++++++++++--------------- src/frontends/qt4/Toolbars.cpp | 12 +++--- src/frontends/qt4/Toolbars.h | 3 +- 6 files changed, 90 insertions(+), 56 deletions(-) diff --git a/src/frontends/qt4/Action.cpp b/src/frontends/qt4/Action.cpp index 71e41a4431..bd102e6b73 100644 --- a/src/frontends/qt4/Action.cpp +++ b/src/frontends/qt4/Action.cpp @@ -24,14 +24,32 @@ #include "support/debug.h" #include "support/lstrings.h" +using namespace std; + + namespace lyx { namespace frontend { -Action::Action(QIcon const & icon, - QString const & text, FuncRequest const & func, - QString const & tooltip, QObject * parent) +Action::Action(FuncRequest func, QIcon const & icon, QString const & text, + QString const & tooltip, QObject * parent) + : QAction(parent), func_(make_shared(move(func))) +{ + init(icon, text, tooltip); +} + + +Action::Action(shared_ptr func, + QIcon const & icon, QString const & text, + QString const & tooltip, QObject * parent) : QAction(parent), func_(func) +{ + init(icon, text, tooltip); +} + + +void Action::init(QIcon const & icon, QString const & text, + QString const & tooltip) { // only Qt/Mac handles that setMenuRole(NoRole); @@ -46,7 +64,7 @@ Action::Action(QIcon const & icon, void Action::update() { - FuncStatus const status = getStatus(func_); + FuncStatus const status = getStatus(*func_); if (status.onOff(true)) { setCheckable(true); @@ -66,7 +84,7 @@ void Action::action() { //LYXERR(Debug::ACTION, "calling lyx::dispatch: func_: "); - lyx::dispatch(func_); + lyx::dispatch(*func_); triggered(this); } diff --git a/src/frontends/qt4/Action.h b/src/frontends/qt4/Action.h index 54954c6900..8bc4a71f24 100644 --- a/src/frontends/qt4/Action.h +++ b/src/frontends/qt4/Action.h @@ -13,6 +13,7 @@ #define ACTION_H #include +#include class QIcon; @@ -32,8 +33,15 @@ class Action : public QAction Q_OBJECT public: - Action(QIcon const & icon, QString const & text, - FuncRequest const & func, QString const & tooltip, QObject * parent); + // Makes a copy of func + Action(FuncRequest func, QIcon const & icon, QString const & text, + QString const & tooltip, QObject * parent); + + // Takes shared ownership of func. + // Use for perf-sensitive code such as populating menus. + Action(std::shared_ptr func, + QIcon const & icon, QString const & text, + QString const & tooltip, QObject * parent); void update(); @@ -45,7 +53,8 @@ private Q_SLOTS: void action(); private: - FuncRequest const & func_ ; + void init(QIcon const & icon, QString const & text, QString const & tooltip); + std::shared_ptr func_; }; diff --git a/src/frontends/qt4/GuiToolbar.cpp b/src/frontends/qt4/GuiToolbar.cpp index 2f0b510991..36a7326a5e 100644 --- a/src/frontends/qt4/GuiToolbar.cpp +++ b/src/frontends/qt4/GuiToolbar.cpp @@ -116,12 +116,12 @@ Action * GuiToolbar::addItem(ToolbarItem const & item) QString text = toqstr(item.label_); // Get the keys bound to this action, but keep only the // first one later - KeyMap::Bindings bindings = theTopLevelKeymap().findBindings(item.func_); + KeyMap::Bindings bindings = theTopLevelKeymap().findBindings(*item.func_); if (!bindings.empty()) text += " [" + toqstr(bindings.begin()->print(KeySequence::ForGui)) + "]"; - Action * act = new Action(getIcon(item.func_, false), - text, item.func_, text, this); + Action * act = new Action(item.func_, getIcon(*item.func_, false), text, + text, this); actions_.append(act); return act; } @@ -148,7 +148,7 @@ public: ToolbarInfo const * tbinfo = guiApp->toolbars().info(tbitem_.name_); if (tbinfo) // use the icon of first action for the toolbar button - setIcon(getIcon(tbinfo->items.begin()->func_, true)); + setIcon(getIcon(*tbinfo->items.begin()->func_, true)); } void mousePressEvent(QMouseEvent * e) @@ -173,7 +173,7 @@ public: ToolbarInfo::item_iterator it = tbinfo->items.begin(); ToolbarInfo::item_iterator const end = tbinfo->items.end(); for (; it != end; ++it) - if (!getStatus(it->func_).unknown()) + if (!getStatus(*it->func_).unknown()) panel->addButton(bar_->addItem(*it)); QToolButton::mousePressEvent(e); @@ -228,7 +228,7 @@ void MenuButton::initialize() ToolbarInfo::item_iterator it = tbinfo->items.begin(); ToolbarInfo::item_iterator const end = tbinfo->items.end(); for (; it != end; ++it) - if (!getStatus(it->func_).unknown()) + if (!getStatus(*it->func_).unknown()) m->add(bar_->addItem(*it)); setMenu(m); } @@ -311,7 +311,7 @@ void GuiToolbar::add(ToolbarItem const & item) break; } case ToolbarItem::COMMAND: { - if (!getStatus(item.func_).unknown()) + if (!getStatus(*item.func_).unknown()) addAction(addItem(item)); break; } diff --git a/src/frontends/qt4/Menus.cpp b/src/frontends/qt4/Menus.cpp index dce5c8cc0d..11ea996a26 100644 --- a/src/frontends/qt4/Menus.cpp +++ b/src/frontends/qt4/Menus.cpp @@ -198,8 +198,8 @@ public: QString const & submenu = QString(), QString const & tooltip = QString(), bool optional = false) - : kind_(kind), label_(label), submenuname_(submenu), - tooltip_(tooltip), optional_(optional) + : kind_(kind), label_(label), func_(make_shared()), + submenuname_(submenu), tooltip_(tooltip), optional_(optional) { LATTEST(kind == Submenu || kind == Help || kind == Info); } @@ -210,10 +210,10 @@ public: QString const & tooltip = QString(), bool optional = false, FuncRequest::Origin origin = FuncRequest::MENU) - : kind_(kind), label_(label), func_(func), + : kind_(kind), label_(label), func_(make_shared(func)), tooltip_(tooltip), optional_(optional) { - func_.setOrigin(origin); + func_->setOrigin(origin); } /// The label of a given menuitem @@ -234,7 +234,7 @@ public: /// The kind of entry Kind kind() const { return kind_; } /// the action (if relevant) - FuncRequest const & func() const { return func_; } + shared_ptr func() const { return func_; } /// the tooltip QString const & tooltip() const { return tooltip_; } /// returns true if the entry should be omitted when disabled @@ -253,13 +253,13 @@ public: return QString(); // Get the keys bound to this action, but keep only the // first one later - KeyMap::Bindings bindings = theTopLevelKeymap().findBindings(func_); + KeyMap::Bindings bindings = theTopLevelKeymap().findBindings(*func_); if (!bindings.empty()) return toqstr(bindings.begin()->print(KeySequence::ForGui)); LYXERR(Debug::KBMAP, "No binding for " - << lyxaction.getActionName(func_.action()) - << '(' << func_.argument() << ')'); + << lyxaction.getActionName(func_->action()) + << '(' << func_->argument() << ')'); return QString(); } @@ -285,7 +285,7 @@ private: /// QString label_; /// - FuncRequest func_; + shared_ptr func_;// non-null /// QString submenuname_; /// @@ -398,7 +398,7 @@ void MenuDefinition::addWithStatusCheck(MenuItem const & i) switch (i.kind()) { case MenuItem::Command: { - FuncStatus status = lyx::getStatus(i.func()); + FuncStatus status = lyx::getStatus(*i.func()); if (status.unknown() || (!status.enabled() && i.optional())) break; items_.push_back(i); @@ -691,7 +691,7 @@ MenuItem const & MenuDefinition::operator[](size_type i) const bool MenuDefinition::hasFunc(FuncRequest const & func) const { for (const_iterator it = begin(), et = end(); it != et; ++it) - if (it->func() == func) + if (*it->func() == func) return true; return false; } @@ -741,7 +741,7 @@ bool MenuDefinition::searchMenu(FuncRequest const & func, docstring_list & names const_iterator m = begin(); const_iterator m_end = end(); for (; m != m_end; ++m) { - if (m->kind() == MenuItem::Command && m->func() == func) { + if (m->kind() == MenuItem::Command && *m->func() == func) { names.push_back(qstring_to_ucs4(m->label())); return true; } @@ -1706,7 +1706,7 @@ struct Menu::Impl { /// populates the menu or one of its submenu /// This is used as a recursive function - void populate(QMenu & qMenu, MenuDefinition const & menu); + void populate(QMenu * qMenu, MenuDefinition const & menu); /// Only needed for top level menus. MenuDefinition * top_level_menu; @@ -1739,7 +1739,7 @@ static QString label(MenuItem const & mi) return label; } -void Menu::Impl::populate(QMenu & qMenu, MenuDefinition const & menu) +void Menu::Impl::populate(QMenu * qMenu, MenuDefinition const & menu) { LYXERR(Debug::GUI, "populating menu " << menu.name()); if (menu.empty()) { @@ -1747,21 +1747,26 @@ void Menu::Impl::populate(QMenu & qMenu, MenuDefinition const & menu) return; } LYXERR(Debug::GUI, " ***** menu entries " << menu.size()); - MenuDefinition::const_iterator m = menu.begin(); - MenuDefinition::const_iterator end = menu.end(); - for (; m != end; ++m) { - if (m->kind() == MenuItem::Separator) - qMenu.addSeparator(); - else if (m->kind() == MenuItem::Submenu) { - QMenu * subMenu = qMenu.addMenu(label(*m)); - populate(*subMenu, m->submenu()); + for (MenuItem const & m : menu) + switch (m.kind()) { + case MenuItem::Separator: + qMenu->addSeparator(); + break; + case MenuItem::Submenu: { + QMenu * subMenu = qMenu->addMenu(label(m)); + populate(subMenu, m.submenu()); subMenu->setEnabled(!subMenu->isEmpty()); - } else { - // we have a MenuItem::Command - qMenu.addAction(new Action(QIcon(), label(*m), - m->func(), m->tooltip(), &qMenu)); + break; + } + case MenuItem::Command: + default: + // FIXME: A previous comment assured that MenuItem::Command was the + // only possible case in practice, but this is wrong. It would be + // good to document which cases are actually treated here. + qMenu->addAction(new Action(m.func(), QIcon(), label(m), + m.tooltip(), qMenu)); + break; } - } } #if (defined(Q_OS_WIN) || defined(Q_CYGWIN_WIN)) && (QT_VERSION >= 0x040600) @@ -1919,7 +1924,7 @@ void Menus::Impl::macxMenuBarInit(QMenuBar * qmb) QAction::MenuRole role; }; - static MacMenuEntry entries[] = { + static const MacMenuEntry entries[] = { {LFUN_DIALOG_SHOW, "aboutlyx", "About LyX", QAction::AboutRole}, {LFUN_DIALOG_SHOW, "prefs", "Preferences", @@ -1948,11 +1953,10 @@ void Menus::Impl::macxMenuBarInit(QMenuBar * qmb) // add the entries to a QMenu that will eventually be empty // and therefore invisible. QMenu * qMenu = qmb->addMenu("special"); - MenuDefinition::const_iterator cit = mac_special_menu_.begin(); - MenuDefinition::const_iterator end = mac_special_menu_.end(); - for (size_t i = 0 ; cit != end ; ++cit, ++i) { - Action * action = new Action(QIcon(), cit->label(), - cit->func(), QString(), qMenu); + size_t i = 0; + for (MenuItem const & m : mac_special_menu_) { + Action * action = new Action(m.func(), QIcon(), m.label(), + QString(), qMenu); action->setMenuRole(entries[i].role); qMenu->addAction(action); } @@ -2091,7 +2095,7 @@ void Menus::Impl::expand(MenuDefinition const & frommenu, break; case MenuItem::Command: - if (!mac_special_menu_.hasFunc(cit->func())) + if (!mac_special_menu_.hasFunc(*cit->func())) tomenu.addWithStatusCheck(*cit); } } @@ -2330,7 +2334,7 @@ void Menus::updateMenu(Menu * qmenu) if (qmenu->d->view) bv = qmenu->d->view->currentBufferView(); d->expand(fromLyxMenu, *qmenu->d->top_level_menu, bv); - qmenu->d->populate(*qmenu, *qmenu->d->top_level_menu); + qmenu->d->populate(qmenu, *qmenu->d->top_level_menu); } diff --git a/src/frontends/qt4/Toolbars.cpp b/src/frontends/qt4/Toolbars.cpp index a9f90d4a5a..b2a9c37138 100644 --- a/src/frontends/qt4/Toolbars.cpp +++ b/src/frontends/qt4/Toolbars.cpp @@ -38,14 +38,16 @@ namespace frontend { // ///////////////////////////////////////////////////////////////////////// -ToolbarItem::ToolbarItem(Type type, FuncRequest const & func, docstring const & label) - : type_(type), func_(func), label_(label) +ToolbarItem::ToolbarItem(Type type, FuncRequest const & func, + docstring const & label) + : type_(type), func_(make_shared(func)), label_(label) { } -ToolbarItem::ToolbarItem(Type type, string const & name, docstring const & label) - : type_(type), label_(label), name_(name) +ToolbarItem::ToolbarItem(Type type, string const & name, + docstring const & label) + : type_(type), func_(make_shared()), label_(label), name_(name) { } @@ -53,7 +55,7 @@ ToolbarItem::ToolbarItem(Type type, string const & name, docstring const & label void ToolbarInfo::add(ToolbarItem const & item) { items.push_back(item); - items.back().func_.setOrigin(FuncRequest::TOOLBAR); + items.back().func_->setOrigin(FuncRequest::TOOLBAR); } diff --git a/src/frontends/qt4/Toolbars.h b/src/frontends/qt4/Toolbars.h index dbaf3aaa21..a65d05ebec 100644 --- a/src/frontends/qt4/Toolbars.h +++ b/src/frontends/qt4/Toolbars.h @@ -17,6 +17,7 @@ #include #include +#include namespace lyx { @@ -57,7 +58,7 @@ public: /// item type Type type_; /// action - FuncRequest func_; + std::shared_ptr func_; // non-null /// label/tooltip docstring label_; /// name -- 2.39.2