mifr/subsurface
1
0
Fork 0
mirror of https://github.com/subsurface/subsurface.git synced 2024-11-28 05:00:20 +00:00
Code Issues Projects Releases Packages Wiki Activity

desktop: do own memory management of quadrant widgets

Browse source 
The memory management of the quadrant widgets is a total mess:
When setting the widget, the QSplitters take ownership, which
means that they will delete the widget in their destructor.

This is inherently incompatible with singletons, which must
not be deleted.

To avoid all these troubles, remove the widgets from the
QSplitters in the desctructor of the MainWindow. This of
course means that we now have to take care about deletion
of the widgets.

For local widgets use std::unique_ptr, for singletons use
a static variable that is deleted on application exit.

Sadly, for the map widget we can't use a normal singleton,
because the QML MapWidget's memory management is buggy.
Add a comment in the source code explaining this.

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>
This commit is contained in:
Berthold Stoeger 2020-12-18 12:01:36 +01:00 committed by Dirk Hohndel
parent 8a36a100ce
commit 64dae43bdd
3 changed files with 59 additions and 43 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

80
desktop-widgets/mainwindow.cpp
View file

@ -135,16 +135,17 @@ MainWindow::MainWindow() : QMainWindow(),
// Define the States of the Application Here, Currently the states are situations where the different // Define the States of the Application Here, Currently the states are situations where the different
// widgets will change on the mainwindow. // widgets will change on the mainwindow.
topSplitter = new QSplitter(Qt::Horizontal); topSplitter.reset(new QSplitter(Qt::Horizontal));
bottomSplitter = new QSplitter(Qt::Horizontal); bottomSplitter.reset(new QSplitter(Qt::Horizontal));
// for the "default" mode // for the "default" mode
mainTab.reset(new MainTab); mainTab.reset(new MainTab);
diveList = new DiveListView(this); diveList.reset(new DiveListView);
graphics = new ProfileWidget2(this); graphics = new ProfileWidget2(this);
MapWidget *mapWidget = MapWidget::instance(); mapWidget.reset(MapWidget::instance()); // Yes, this is ominous see comment in mapwidget.cpp.
plannerWidgets.reset(new PlannerWidgets); plannerWidgets.reset(new PlannerWidgets);
StatsWidget *statistics = new StatsWidget(this); statistics.reset(new StatsWidget);
profileContainer.reset(new QWidget);
// what is a sane order for those icons? we should have the ones the user is // what is a sane order for those icons? we should have the ones the user is
// most likely to want towards the top so they are always visible // most likely to want towards the top so they are always visible
@ -164,7 +165,6 @@ MainWindow::MainWindow() : QMainWindow(),
toolBar->addAction(a); toolBar->addAction(a);
toolBar->setOrientation(Qt::Vertical); toolBar->setOrientation(Qt::Vertical);
toolBar->setIconSize(QSize(24,24)); toolBar->setIconSize(QSize(24,24));
QWidget *profileContainer = new QWidget();
QHBoxLayout *profLayout = new QHBoxLayout(); QHBoxLayout *profLayout = new QHBoxLayout();
profLayout->setSpacing(0); profLayout->setSpacing(0);
profLayout->setMargin(0); profLayout->setMargin(0);
@ -175,26 +175,26 @@ MainWindow::MainWindow() : QMainWindow(),
diveSiteEdit.reset(new LocationInformationWidget); diveSiteEdit.reset(new LocationInformationWidget);
registerApplicationState(ApplicationState::Default, { true, { mainTab.get(), FLAG_NONE }, { profileContainer, FLAG_NONE }, registerApplicationState(ApplicationState::Default, { true, { mainTab.get(), FLAG_NONE }, { profileContainer.get(), FLAG_NONE },
{ diveList, FLAG_NONE }, { mapWidget, FLAG_NONE } }); { diveList.get(), FLAG_NONE }, { mapWidget.get(), FLAG_NONE } });
registerApplicationState(ApplicationState::EditDive, { false, { mainTab.get(), FLAG_NONE }, { profileContainer, FLAG_NONE }, registerApplicationState(ApplicationState::EditDive, { false, { mainTab.get(), FLAG_NONE }, { profileContainer.get(), FLAG_NONE },
{ diveList, FLAG_NONE }, { mapWidget, FLAG_NONE } }); { diveList.get(), FLAG_NONE }, { mapWidget.get(), FLAG_NONE } });
registerApplicationState(ApplicationState::PlanDive, { false, { &plannerWidgets->plannerWidget, FLAG_NONE }, { profileContainer, FLAG_NONE }, registerApplicationState(ApplicationState::PlanDive, { false, { &plannerWidgets->plannerWidget, FLAG_NONE }, { profileContainer.get(), FLAG_NONE },
{ &plannerWidgets->plannerSettingsWidget, FLAG_NONE }, { &plannerWidgets->plannerDetails, FLAG_NONE } }); { &plannerWidgets->plannerSettingsWidget, FLAG_NONE }, { &plannerWidgets->plannerDetails, FLAG_NONE } });
registerApplicationState(ApplicationState::EditPlannedDive, { true, { &plannerWidgets->plannerWidget, FLAG_NONE }, { profileContainer, FLAG_NONE }, registerApplicationState(ApplicationState::EditPlannedDive, { true, { &plannerWidgets->plannerWidget, FLAG_NONE }, { profileContainer.get(), FLAG_NONE },
{ diveList, FLAG_NONE }, { mapWidget, FLAG_NONE } }); { diveList.get(), FLAG_NONE }, { mapWidget.get(), FLAG_NONE } });
registerApplicationState(ApplicationState::EditDiveSite, { false, { diveSiteEdit.get(), FLAG_NONE }, { profileContainer, FLAG_DISABLED }, registerApplicationState(ApplicationState::EditDiveSite, { false, { diveSiteEdit.get(), FLAG_NONE }, { profileContainer.get(), FLAG_DISABLED },
{ diveList, FLAG_DISABLED }, { mapWidget, FLAG_NONE } }); { diveList.get(), FLAG_DISABLED }, { mapWidget.get(), FLAG_NONE } });
registerApplicationState(ApplicationState::FilterDive, { true, { mainTab.get(), FLAG_NONE }, { profileContainer, FLAG_NONE }, registerApplicationState(ApplicationState::FilterDive, { true, { mainTab.get(), FLAG_NONE }, { profileContainer.get(), FLAG_NONE },
{ diveList, FLAG_NONE }, { &filterWidget, FLAG_NONE } }); { diveList.get(), FLAG_NONE }, { &filterWidget, FLAG_NONE } });
registerApplicationState(ApplicationState::Statistics, { true, { statistics, FLAG_NONE }, { profileContainer, FLAG_NONE }, registerApplicationState(ApplicationState::Statistics, { true, { statistics.get(), FLAG_NONE }, { nullptr, FLAG_NONE },
{ diveList, FLAG_DISABLED }, { &filterWidget, FLAG_NONE } }); { diveList.get(), FLAG_DISABLED }, { &filterWidget, FLAG_NONE } });
registerApplicationState(ApplicationState::MapMaximized, { true, { nullptr, FLAG_NONE }, { nullptr, FLAG_NONE }, registerApplicationState(ApplicationState::MapMaximized, { true, { nullptr, FLAG_NONE }, { nullptr, FLAG_NONE },
{ nullptr, FLAG_NONE }, { mapWidget, FLAG_NONE } }); { nullptr, FLAG_NONE }, { mapWidget.get(), FLAG_NONE } });
registerApplicationState(ApplicationState::ProfileMaximized, { true, { nullptr, FLAG_NONE }, { profileContainer, FLAG_NONE }, registerApplicationState(ApplicationState::ProfileMaximized, { true, { nullptr, FLAG_NONE }, { profileContainer.get(), FLAG_NONE },
{ nullptr, FLAG_NONE }, { nullptr, FLAG_NONE } }); { nullptr, FLAG_NONE }, { nullptr, FLAG_NONE } });
registerApplicationState(ApplicationState::ListMaximized, { true, { nullptr, FLAG_NONE }, { nullptr, FLAG_NONE }, registerApplicationState(ApplicationState::ListMaximized, { true, { nullptr, FLAG_NONE }, { nullptr, FLAG_NONE },
{ diveList, FLAG_NONE }, { nullptr, FLAG_NONE } }); { diveList.get(), FLAG_NONE }, { nullptr, FLAG_NONE } });
registerApplicationState(ApplicationState::InfoMaximized, { true, { mainTab.get(), FLAG_NONE }, { nullptr, FLAG_NONE }, registerApplicationState(ApplicationState::InfoMaximized, { true, { mainTab.get(), FLAG_NONE }, { nullptr, FLAG_NONE },
{ nullptr, FLAG_NONE }, { nullptr, FLAG_NONE } }); { nullptr, FLAG_NONE }, { nullptr, FLAG_NONE } });
restoreSplitterSizes(); restoreSplitterSizes();
@ -204,7 +204,7 @@ MainWindow::MainWindow() : QMainWindow(),
if (!QIcon::hasThemeIcon("window-close")) { if (!QIcon::hasThemeIcon("window-close")) {
QIcon::setThemeName("subsurface"); QIcon::setThemeName("subsurface");
} }
connect(diveList, &DiveListView::divesSelected, this, &MainWindow::selectionChanged); connect(diveList.get(), &DiveListView::divesSelected, this, &MainWindow::selectionChanged);
connect(&diveListNotifier, &DiveListNotifier::settingsChanged, this, &MainWindow::readSettings); connect(&diveListNotifier, &DiveListNotifier::settingsChanged, this, &MainWindow::readSettings);
for (int i = 0; i < NUM_RECENT_FILES; i++) { for (int i = 0; i < NUM_RECENT_FILES; i++) {
actionsRecent[i] = new QAction(this); actionsRecent[i] = new QAction(this);
@ -340,6 +340,8 @@ MainWindow::MainWindow() : QMainWindow(),
MainWindow::~MainWindow() MainWindow::~MainWindow()
{ {
// Remove widgets from the splitters so that they don't delete singletons.
clearSplitters();
write_hashes(); write_hashes();
m_Instance = nullptr; m_Instance = nullptr;
} }
@ -1573,29 +1575,29 @@ void MainWindow::registerApplicationState(ApplicationState state, Quadrants q)
applicationState[(int)state] = q; applicationState[(int)state] = q;
} }
void MainWindow::setQuadrantWidget(const Quadrant &q, QSplitter *splitter) void MainWindow::setQuadrantWidget(const Quadrant &q, QSplitter &splitter)
{ {
if (q.widget) { if (q.widget) {
splitter->addWidget(q.widget); splitter.addWidget(q.widget);
splitter->setCollapsible(splitter->count() - 1, false); splitter.setCollapsible(splitter.count() - 1, false);
q.widget->setEnabled(!(q.flags & FLAG_DISABLED)); q.widget->setEnabled(!(q.flags & FLAG_DISABLED));
} }
} }
static void clearSplitter(QSplitter *splitter, QWidget *parent) static void clearSplitter(QSplitter &splitter)
{ {
// Qt's ownership model is absolutely hare-brained. // Qt's ownership model is absolutely hare-brained.
// To remove a widget from a splitter, you reparent it, which // To remove a widget from a splitter, you reparent it, which
// informs the splitter via a signal. Wow. // informs the splitter via a signal. Wow.
while (splitter->count() > 0) while (splitter.count() > 0)
splitter->widget(0)->setParent(parent); splitter.widget(0)->setParent(nullptr);
} }
void MainWindow::clearSplitters() void MainWindow::clearSplitters()
{ {
clearSplitter(topSplitter, this); clearSplitter(*topSplitter);
clearSplitter(bottomSplitter, this); clearSplitter(*bottomSplitter);
clearSplitter(ui.mainSplitter, this); clearSplitter(*ui.mainSplitter);
} }
bool MainWindow::userMayChangeAppState() const bool MainWindow::userMayChangeAppState() const
@ -1614,16 +1616,16 @@ void MainWindow::setApplicationState(ApplicationState state)
clearSplitters(); clearSplitters();
const Quadrants &quadrants = applicationState[(int)state]; const Quadrants &quadrants = applicationState[(int)state];
setQuadrantWidget(quadrants.topLeft, topSplitter); setQuadrantWidget(quadrants.topLeft, *topSplitter);
setQuadrantWidget(quadrants.topRight, topSplitter); setQuadrantWidget(quadrants.topRight, *topSplitter);
setQuadrantWidget(quadrants.bottomLeft, bottomSplitter); setQuadrantWidget(quadrants.bottomLeft, *bottomSplitter);
setQuadrantWidget(quadrants.bottomRight, bottomSplitter); setQuadrantWidget(quadrants.bottomRight, *bottomSplitter);
if (topSplitter->count() >= 1) { if (topSplitter->count() >= 1) {
ui.mainSplitter->addWidget(topSplitter); ui.mainSplitter->addWidget(topSplitter.get());
ui.mainSplitter->setCollapsible(ui.mainSplitter->count() - 1, false); ui.mainSplitter->setCollapsible(ui.mainSplitter->count() - 1, false);
} }
if (bottomSplitter->count() >= 1) { if (bottomSplitter->count() >= 1) {
ui.mainSplitter->addWidget(bottomSplitter); ui.mainSplitter->addWidget(bottomSplitter.get());
ui.mainSplitter->setCollapsible(ui.mainSplitter->count() - 1, false); ui.mainSplitter->setCollapsible(ui.mainSplitter->count() - 1, false);
} }

13
desktop-widgets/mainwindow.h
View file

@ -30,12 +30,14 @@ class DiveTripModel;
class QItemSelection; class QItemSelection;
class DiveListView; class DiveListView;
class MainTab; class MainTab;
class MapWidget;
class QWebView; class QWebView;
class QSettings; class QSettings;
class UpdateManager; class UpdateManager;
class UserManual; class UserManual;
class PlannerWidgets; class PlannerWidgets;
class ProfileWidget2; class ProfileWidget2;
class StatsWidget;
class LocationInformationWidget; class LocationInformationWidget;
class MainWindow : public QMainWindow { class MainWindow : public QMainWindow {
@ -63,8 +65,11 @@ public:
std::unique_ptr<MainTab> mainTab; std::unique_ptr<MainTab> mainTab;
std::unique_ptr<PlannerWidgets> plannerWidgets; std::unique_ptr<PlannerWidgets> plannerWidgets;
std::unique_ptr<StatsWidget> statistics;
ProfileWidget2 *graphics; ProfileWidget2 *graphics;
DiveListView *diveList; std::unique_ptr<DiveListView> diveList;
std::unique_ptr<QWidget> profileContainer;
std::unique_ptr<MapWidget> mapWidget;
private private
slots: slots:
/* file menu action */ /* file menu action */
@ -152,8 +157,8 @@ slots:
private: private:
Ui::MainWindow ui; Ui::MainWindow ui;
FilterWidget filterWidget; FilterWidget filterWidget;
QSplitter *topSplitter; std::unique_ptr<QSplitter> topSplitter;
QSplitter *bottomSplitter; std::unique_ptr<QSplitter> bottomSplitter;
QAction *actionNextDive; QAction *actionNextDive;
QAction *actionPreviousDive; QAction *actionPreviousDive;
QAction *undoAction; QAction *undoAction;
@ -215,7 +220,7 @@ private:
Quadrants applicationState[(size_t)ApplicationState::Count]; Quadrants applicationState[(size_t)ApplicationState::Count];
static void addWidgets(const Quadrant &); static void addWidgets(const Quadrant &);
bool userMayChangeAppState() const; bool userMayChangeAppState() const;
void setQuadrantWidget(const Quadrant &q, QSplitter *splitter); void setQuadrantWidget(const Quadrant &q, QSplitter &splitter);
void registerApplicationState(ApplicationState state, Quadrants q); void registerApplicationState(ApplicationState state, Quadrants q);
QMenu *connections; QMenu *connections;

9
desktop-widgets/mapwidget.cpp
View file

@ -116,6 +116,15 @@ void MapWidget::divesChanged(const QVector<dive *> &, DiveField field)
reload(); reload();
} }
// Sadly, for reasons out of our control, we can't use a normal singleton for the
// map widget: In a standard singleton, the object is freed after main() exits.
// However, if there is an animation running (map zooming), the thread is
// terminated, when the QApplication object is destroyed, which is before main()
// exits. The thread has a QQmlAnimationTimer that is freed. However, the map widget
// then tries to free the object itself, leading to a crash. Clearly, a bug in
// the QML MapWidget / QtQuick ecosystem.
// To solve this, the mainwindow will destroy the map widget and in the destructor
// the reference is cleared. Sad.
MapWidget::~MapWidget() MapWidget::~MapWidget()
{ {
m_instance = NULL; m_instance = NULL;