Skip to content

Commit

Permalink
Improve safety in code (#588)
Browse files Browse the repository at this point in the history
Delete areas later so that that they can be accessed by
(inherited) dock widgets in dtor. Add some QPointer to
prevent crashes.

Hence allow users to do more while dock widgets etc
are being destroyed.
  • Loading branch information
tmartsum authored Dec 8, 2023
1 parent 6c98c29 commit f848df7
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 37 deletions.
7 changes: 3 additions & 4 deletions src/DockAreaWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,7 @@ int CDockAreaWidget::openDockWidgetsCount() const
int Count = 0;
for (int i = 0; i < d->ContentsLayout->count(); ++i)
{
if (!dockWidget(i)->isClosed())
if (dockWidget(i) && !dockWidget(i)->isClosed())
{
++Count;
}
Expand All @@ -772,7 +772,7 @@ QList<CDockWidget*> CDockAreaWidget::openedDockWidgets() const
for (int i = 0; i < d->ContentsLayout->count(); ++i)
{
CDockWidget* DockWidget = dockWidget(i);
if (!DockWidget->isClosed())
if (DockWidget && !DockWidget->isClosed())
{
DockWidgetList.append(dockWidget(i));
}
Expand All @@ -786,7 +786,7 @@ int CDockAreaWidget::indexOfFirstOpenDockWidget() const
{
for (int i = 0; i < d->ContentsLayout->count(); ++i)
{
if (!dockWidget(i)->isClosed())
if (dockWidget(i) && !dockWidget(i)->isClosed())
{
return i;
}
Expand All @@ -809,7 +809,6 @@ CDockWidget* CDockAreaWidget::dockWidget(int Index) const
return qobject_cast<CDockWidget*>(d->ContentsLayout->widget(Index));
}


//============================================================================
void CDockAreaWidget::reorderDockWidget(int fromIndex, int toIndex)
{
Expand Down
33 changes: 24 additions & 9 deletions src/DockContainerWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ class DockContainerWidgetPrivate
CDockContainerWidget* _this;
QPointer<CDockManager> DockManager;
unsigned int zOrderIndex = 0;
QList<CDockAreaWidget*> DockAreas;
QList<QPointer<CDockAreaWidget>> DockAreas;
QList<CAutoHideDockContainer*> AutoHideWidgets;
QMap<SideBarLocation, CAutoHideSideBar*> SideTabBarWidgets;
QGridLayout* Layout = nullptr;
Expand Down Expand Up @@ -299,7 +299,11 @@ class DockContainerWidgetPrivate
VisibleDockAreaCount = 0;
for (auto DockArea : DockAreas)
{
VisibleDockAreaCount += DockArea->isHidden() ? 0 : 1;
if (!DockArea)
{
continue;
}
VisibleDockAreaCount += (DockArea->isHidden() ? 0 : 1);
}
}

Expand Down Expand Up @@ -924,7 +928,10 @@ void DockContainerWidgetPrivate::addDockAreasToList(const QList<CDockAreaWidget*
//============================================================================
void DockContainerWidgetPrivate::appendDockAreas(const QList<CDockAreaWidget*> NewDockAreas)
{
DockAreas.append(NewDockAreas);
for (auto *newDockArea : NewDockAreas)
{
DockAreas.append(newDockArea);
}
for (auto DockArea : NewDockAreas)
{
QObject::connect(DockArea,
Expand Down Expand Up @@ -1641,7 +1648,7 @@ CDockAreaWidget* CDockContainerWidget::dockAreaAt(const QPoint& GlobalPos) const
{
for (const auto& DockArea : d->DockAreas)
{
if (DockArea->isVisible() && DockArea->rect().contains(DockArea->mapFromGlobal(GlobalPos)))
if (DockArea && DockArea->isVisible() && DockArea->rect().contains(DockArea->mapFromGlobal(GlobalPos)))
{
return DockArea;
}
Expand Down Expand Up @@ -1678,7 +1685,7 @@ int CDockContainerWidget::visibleDockAreaCount() const
int Result = 0;
for (auto DockArea : d->DockAreas)
{
Result += DockArea->isHidden() ? 0 : 1;
Result += (!DockArea || DockArea->isHidden()) ? 0 : 1;
}

return Result;
Expand Down Expand Up @@ -1802,7 +1809,7 @@ QList<CDockAreaWidget*> CDockContainerWidget::openedDockAreas() const
QList<CDockAreaWidget*> Result;
for (auto DockArea : d->DockAreas)
{
if (!DockArea->isHidden())
if (DockArea && !DockArea->isHidden())
{
Result.append(DockArea);
}
Expand All @@ -1818,7 +1825,7 @@ QList<CDockWidget*> CDockContainerWidget::openedDockWidgets() const
QList<CDockWidget*> DockWidgetList;
for (auto DockArea : d->DockAreas)
{
if (!DockArea->isHidden())
if (DockArea && !DockArea->isHidden())
{
DockWidgetList.append(DockArea->openedDockWidgets());
}
Expand All @@ -1833,7 +1840,7 @@ bool CDockContainerWidget::hasOpenDockAreas() const
{
for (auto DockArea : d->DockAreas)
{
if (!DockArea->isHidden())
if (DockArea && !DockArea->isHidden())
{
return true;
}
Expand Down Expand Up @@ -2058,6 +2065,10 @@ QList<CDockWidget*> CDockContainerWidget::dockWidgets() const
QList<CDockWidget*> Result;
for (const auto DockArea : d->DockAreas)
{
if (!DockArea)
{
continue;
}
Result.append(DockArea->dockWidgets());
}

Expand Down Expand Up @@ -2090,6 +2101,10 @@ CDockWidget::DockWidgetFeatures CDockContainerWidget::features() const
CDockWidget::DockWidgetFeatures Features(CDockWidget::AllDockWidgetFeatures);
for (const auto DockArea : d->DockAreas)
{
if (!DockArea)
{
continue;
}
Features &= DockArea->features();
}

Expand All @@ -2109,7 +2124,7 @@ void CDockContainerWidget::closeOtherAreas(CDockAreaWidget* KeepOpenArea)
{
for (const auto DockArea : d->DockAreas)
{
if (DockArea == KeepOpenArea)
if (!DockArea || DockArea == KeepOpenArea)
{
continue;
}
Expand Down
1 change: 1 addition & 0 deletions src/DockFocusController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ DockFocusControllerPrivate::DockFocusControllerPrivate(
//============================================================================
void DockFocusControllerPrivate::updateDockWidgetFocus(CDockWidget* DockWidget)
{
if (!DockWidget) return;
if (!DockWidget->features().testFlag(CDockWidget::DockWidgetFocusable))
{
return;
Expand Down
63 changes: 40 additions & 23 deletions src/DockManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ static QString FloatingContainersTitle;
struct DockManagerPrivate
{
CDockManager* _this;
QList<CFloatingDockContainer*> FloatingWidgets;
QList<CFloatingDockContainer*> HiddenFloatingWidgets;
QList<QPointer<CFloatingDockContainer>> FloatingWidgets;
QList<QPointer<CFloatingDockContainer>> HiddenFloatingWidgets;
QList<CDockContainerWidget*> Containers;
CDockOverlay* ContainerOverlay;
CDockOverlay* DockAreaOverlay;
Expand Down Expand Up @@ -153,7 +153,10 @@ struct DockManagerPrivate
// Hide updates of floating widgets from user
for (auto FloatingWidget : FloatingWidgets)
{
FloatingWidget->hide();
if (FloatingWidget)
{
FloatingWidget->hide();
}
}
}

Expand Down Expand Up @@ -333,7 +336,8 @@ bool DockManagerPrivate::restoreStateFromXml(const QByteArray &state, int versi
int FloatingWidgetIndex = DockContainerCount - 1;
for (int i = FloatingWidgetIndex; i < FloatingWidgets.count(); ++i)
{
auto* floatingWidget = FloatingWidgets[i];
CFloatingDockContainer* floatingWidget = FloatingWidgets[i];
if (!floatingWidget) continue;
_this->removeDockContainer(floatingWidget->dockContainer());
floatingWidget->deleteLater();
}
Expand Down Expand Up @@ -536,32 +540,40 @@ CDockManager::CDockManager(QWidget *parent) :
CDockManager::~CDockManager()
{
// fix memory leaks, see https://github.com/githubuser0xFFFF/Qt-Advanced-Docking-System/issues/307
std::vector<ads::CDockAreaWidget*> areas;
for ( int i = 0; i != dockAreaCount(); ++i )
{
auto area = dockArea(i);
if ( area->dockManager() == this )
areas.push_back( area );
// else, this is surprising, looks like this CDockAreaWidget is child of two different CDockManager
// this is reproductible by https://github.com/githubuser0xFFFF/Qt-Advanced-Docking-System/issues/585 testcase
// then, when a CDockManager deletes itself, it deletes the CDockAreaWidget, then, the other
// CDockManager will try to delete the CDockAreaWidget again and this will crash
// So let's just delete CDockAreaWidget we are the parent of!
}
for ( auto area : areas )
{
for ( auto widget : area->dockWidgets() )
delete widget;
std::vector<QPointer<ads::CDockAreaWidget>> areas;
for (int i = 0; i != dockAreaCount(); ++i)
{
areas.push_back( dockArea(i) );
}
for ( auto area : areas )
{
if (!area || area->dockManager() != this) continue;

delete area;
}
// QPointer delete safety - just in case some dock wigdet in destruction
// deletes another related/twin or child dock widget.
std::vector<QPointer<QWidget>> deleteWidgets;
for ( auto widget : area->dockWidgets() )
{
deleteWidgets.push_back(widget);
}
for ( auto ptrWdg : deleteWidgets)
{
delete ptrWdg;
}
}

auto FloatingWidgets = d->FloatingWidgets;
for (auto FloatingWidget : FloatingWidgets)
{
delete FloatingWidget;
}

// Delete Dock Widgets before Areas so widgets can access them late (like dtor)
for ( auto area : areas )
{
delete area;
}

delete d;
}

Expand Down Expand Up @@ -732,7 +744,12 @@ const QList<CDockContainerWidget*> CDockManager::dockContainers() const
//============================================================================
const QList<CFloatingDockContainer*> CDockManager::floatingWidgets() const
{
return d->FloatingWidgets;
QList<CFloatingDockContainer*> res;
for (auto &fl : d->FloatingWidgets)
{
if (fl) res.append(fl);
}
return res;
}


Expand Down
2 changes: 1 addition & 1 deletion src/DockWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ struct DockWidgetPrivate
CDockWidgetTab* TabWidget = nullptr;
CDockWidget::DockWidgetFeatures Features = CDockWidget::DefaultDockWidgetFeatures;
CDockManager* DockManager = nullptr;
CDockAreaWidget* DockArea = nullptr;
QPointer<CDockAreaWidget> DockArea;
QAction* ToggleViewAction = nullptr;
bool Closed = false;
QScrollArea* ScrollArea = nullptr;
Expand Down

0 comments on commit f848df7

Please sign in to comment.