Le 26/06/2016 à 16:27, Richard Heck a écrit :
The problem here seems to trace to how FuncRequest objects are handled
by our Action class. Namely, when an Action is created, it retains a
const & to a FuncRequest. So one can't pass a temporary.
Well, you can always pass a temporary, but Action does not respect the
implicit contract not to access the reference after Action::Action() has
returned. I agree this is a weird design. You should not have to handle
the Action's resources on its behalf.
On the other hand, Actions are created pretty often in Menus.cpp so one
downside of copying might be performance. In the attached patch, I
propose to replace the reference with a shared_ptr to avoid copying
tens of FuncRequests every time one opens a menu.
But I worry that this is just hiding the real problem,
and that all these Action objects we're creating are leaking, or at
least being retained for no good reason.
Notice that you are giving the ownership of the new Actions to
InsetMenuButton (this):
+ Action * act =
+ new Action(getIcon(cfunc, false), loc_item, cfunc, loc_item,
this);
(This is not explained in Action.h, but this is consistent with the Qt
API.) Moreover the manual does not state that the QMenu takes ownership.
Therefore I see no reason for them to be deleted by QMenu::clear().
A small remark:
+InsetMenuButton::InsetMenuButton(GuiToolbar * bar)
+ : QToolButton(bar), bar_(bar), d(new Private())
you do not delete this Private object. The simplest is to just
replace the naked pointer with a unique_ptr so that you do not have to
implement a destructor by hand.
Guillaume
>From ec5409054e4ec402d687509f47681fde06856249 Mon Sep 17 00:00:00 2001
From: Guillaume Munch <g...@lyx.org>
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 | 21 +++++++++---
src/frontends/qt4/Action.h | 12 +++++--
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, 80 insertions(+), 56 deletions(-)
diff --git a/src/frontends/qt4/Action.cpp b/src/frontends/qt4/Action.cpp
index 71e41a4..ab6ca17 100644
--- a/src/frontends/qt4/Action.cpp
+++ b/src/frontends/qt4/Action.cpp
@@ -24,13 +24,24 @@
#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 const & func,
+ QIcon const & icon, QString const & text,
+ QString const & tooltip, QObject * parent)
+ : Action(make_shared<FuncRequest>(func),
+ icon, text, tooltip, parent)
+{}
+
+
+Action::Action(shared_ptr<FuncRequest const> func,
+ QIcon const & icon, QString const & text,
+ QString const & tooltip, QObject * parent)
: QAction(parent), func_(func)
{
// only Qt/Mac handles that
@@ -46,7 +57,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 +77,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 54954c6..40cfa86 100644
--- a/src/frontends/qt4/Action.h
+++ b/src/frontends/qt4/Action.h
@@ -13,6 +13,7 @@
#define ACTION_H
#include <QAction>
+#include <memory>
class QIcon;
@@ -32,8 +33,13 @@ class Action : public QAction
Q_OBJECT
public:
- Action(QIcon const & icon, QString const & text,
- FuncRequest const & func, QString const & tooltip, QObject * parent);
+ Action(FuncRequest const & func,
+ QIcon const & icon, QString const & text,
+ QString const & tooltip, QObject * parent);
+
+ Action(std::shared_ptr<FuncRequest const> func,
+ QIcon const & icon, QString const & text,
+ QString const & tooltip, QObject * parent);
void update();
@@ -45,7 +51,7 @@ private Q_SLOTS:
void action();
private:
- FuncRequest const & func_ ;
+ std::shared_ptr<FuncRequest const> func_;
};
diff --git a/src/frontends/qt4/GuiToolbar.cpp b/src/frontends/qt4/GuiToolbar.cpp
index 77471c9..c52fa8f 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 0d80b56..6984ba1 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<FuncRequest>()),
+ 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<FuncRequest>(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<FuncRequest const> 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<FuncRequest> 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;
}
@@ -1718,7 +1718,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;
@@ -1751,7 +1751,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()) {
@@ -1759,21 +1759,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)
@@ -1931,7 +1936,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",
@@ -1960,11 +1965,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);
}
@@ -2103,7 +2107,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);
}
}
@@ -2342,7 +2346,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 19b9560..2425bc8 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<FuncRequest>(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<FuncRequest>()), 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 02d0ebe..d22eda9 100644
--- a/src/frontends/qt4/Toolbars.h
+++ b/src/frontends/qt4/Toolbars.h
@@ -17,6 +17,7 @@
#include <vector>
#include <map>
+#include <memory>
namespace lyx {
@@ -57,7 +58,7 @@ public:
/// item type
Type type_;
/// action
- FuncRequest func_;
+ std::shared_ptr<FuncRequest> func_; // non-null
/// label/tooltip
docstring label_;
/// name
--
2.7.4
