Skip to content

Commit

Permalink
Fix crash when certain symbol pages are open in style dock, eg
Browse files Browse the repository at this point in the history 
categorized class symbol editors, and QGIS is closed or a new project
opened

The symbol ownership of QgsSymbolSelectorWidget is very messy, and
we can't fix till 4.0. Workaround this by introducing a temporary
API to transfer symbol ownership to the widget.
  • Loading branch information
@nyalldawson
nyalldawson committed Sep 29, 2023
1 parent 3de6757 commit 784c844
Show file tree
Hide file tree
Showing 11 changed files with 68 additions and 100 deletions.
2 changes: 2 additions & 0 deletions python/gui/auto_generated/symbology/qgssymbolselectordialog.sip.in
View file
Expand Up @@ -43,6 +43,8 @@ Symbol selector widget that can be used to select and build a symbol
The ownership of the symbol is not transferred and must exist for the lifetime of the widget.
%End



QMenu *advancedMenu();
%Docstring
Returns menu for "advanced" button - create it if doesn't exist and show the advanced button
Expand Down
43 changes: 14 additions & 29 deletions src/gui/qgssymbolbutton.cpp
View file
Expand Up @@ -139,23 +139,23 @@ void QgsSymbolButton::showSettingsDialog()
context.appendScopes( QgsExpressionContextUtils::globalProjectLayerScopes( mLayer.data() ) );
}

QgsSymbol *newSymbol = nullptr;
std::unique_ptr< QgsSymbol > newSymbol;
if ( mSymbol )
{
newSymbol = mSymbol->clone();
newSymbol.reset( mSymbol->clone() );
}
else
{
switch ( mType )
{
case Qgis::SymbolType::Marker:
newSymbol = QgsSymbol::defaultSymbol( Qgis::GeometryType::Point );
newSymbol.reset( QgsSymbol::defaultSymbol( Qgis::GeometryType::Point ) );
break;
case Qgis::SymbolType::Line:
newSymbol = QgsSymbol::defaultSymbol( Qgis::GeometryType::Line );
newSymbol.reset( QgsSymbol::defaultSymbol( Qgis::GeometryType::Line ) );
break;
case Qgis::SymbolType::Fill:
newSymbol = QgsSymbol::defaultSymbol( Qgis::GeometryType::Polygon );
newSymbol.reset( QgsSymbol::defaultSymbol( Qgis::GeometryType::Polygon ) );
break;
case Qgis::SymbolType::Hybrid:
break;
Expand All @@ -170,45 +170,30 @@ void QgsSymbolButton::showSettingsDialog()
QgsPanelWidget *panel = QgsPanelWidget::findParentPanel( this );
if ( panel && panel->dockMode() )
{
QgsSymbolSelectorWidget *d = new QgsSymbolSelectorWidget( newSymbol, QgsStyle::defaultStyle(), mLayer, panel );
d->setPanelTitle( mDialogTitle );
d->setContext( symbolContext );
connect( d, &QgsPanelWidget::widgetChanged, this, &QgsSymbolButton::updateSymbolFromWidget );
connect( d, &QgsPanelWidget::panelAccepted, this, &QgsSymbolButton::cleanUpSymbolSelector );
panel->openPanel( d );
QgsSymbolSelectorWidget *widget = QgsSymbolSelectorWidget::createWidgetWithSymbolOwnership( std::move( newSymbol ), QgsStyle::defaultStyle(), mLayer, panel );
widget->setPanelTitle( mDialogTitle );
widget->setContext( symbolContext );
connect( widget, &QgsPanelWidget::widgetChanged, this, [ = ] { updateSymbolFromWidget( widget ); } );
panel->openPanel( widget );
}
else
{
QgsSymbolSelectorDialog dialog( newSymbol, QgsStyle::defaultStyle(), mLayer, this );
QgsSymbolSelectorDialog dialog( newSymbol.get(), QgsStyle::defaultStyle(), mLayer, this );
dialog.setWindowTitle( mDialogTitle );
dialog.setContext( symbolContext );
if ( dialog.exec() )
{
setSymbol( newSymbol );
}
else
{
delete newSymbol;
setSymbol( newSymbol.release() );
}

// reactivate button's window
activateWindow();
}
}

void QgsSymbolButton::updateSymbolFromWidget()
{
if ( QgsSymbolSelectorWidget *w = qobject_cast<QgsSymbolSelectorWidget *>( sender() ) )
setSymbol( w->symbol()->clone() );
}

void QgsSymbolButton::cleanUpSymbolSelector( QgsPanelWidget *container )
void QgsSymbolButton::updateSymbolFromWidget( QgsSymbolSelectorWidget *widget )
{
QgsSymbolSelectorWidget *w = qobject_cast<QgsSymbolSelectorWidget *>( container );
if ( !w )
return;

delete w->symbol();
setSymbol( widget->symbol()->clone() );
}

QgsMapCanvas *QgsSymbolButton::mapCanvas() const
Expand Down
4 changes: 2 additions & 2 deletions src/gui/qgssymbolbutton.h
View file
Expand Up @@ -30,6 +30,7 @@ class QgsPanelWidget;
class QgsMessageBar;
class QMimeData;
class QgsSymbol;
class QgsSymbolSelectorWidget;

/**
* \ingroup gui
Expand Down Expand Up @@ -304,8 +305,7 @@ class GUI_EXPORT QgsSymbolButton : public QToolButton
private slots:

void showSettingsDialog();
void updateSymbolFromWidget();
void cleanUpSymbolSelector( QgsPanelWidget *container );
void updateSymbolFromWidget( QgsSymbolSelectorWidget *widget );

/**
* Creates the drop-down menu entries
Expand Down
36 changes: 11 additions & 25 deletions src/gui/symbology/qgscategorizedsymbolrendererwidget.cpp
View file
Expand Up @@ -816,13 +816,10 @@ void QgsCategorizedSymbolRendererWidget::changeCategorizedSymbol()
std::unique_ptr<QgsSymbol> newSymbol( mCategorizedSymbol->clone() );
if ( panel && panel->dockMode() )
{
// bit tricky here - the widget doesn't take ownership of the symbol. So we need it to last for the duration of the
// panel's existence. Accordingly, just kinda give it ownership here, and clean up in cleanUpSymbolSelector
QgsSymbolSelectorWidget *dlg = new QgsSymbolSelectorWidget( newSymbol.release(), mStyle, mLayer, panel );
dlg->setContext( mContext );
connect( dlg, &QgsPanelWidget::widgetChanged, this, &QgsCategorizedSymbolRendererWidget::updateSymbolsFromWidget );
connect( dlg, &QgsPanelWidget::panelAccepted, this, &QgsCategorizedSymbolRendererWidget::cleanUpSymbolSelector );
openPanel( dlg );
QgsSymbolSelectorWidget *widget = QgsSymbolSelectorWidget::createWidgetWithSymbolOwnership( std::move( newSymbol ), mStyle, mLayer, panel );
widget->setContext( mContext );
connect( widget, &QgsPanelWidget::widgetChanged, this, [ = ] { updateSymbolsFromWidget( widget ); } );
openPanel( widget );
}
else
{
Expand Down Expand Up @@ -873,12 +870,11 @@ void QgsCategorizedSymbolRendererWidget::changeCategorySymbol()
QgsPanelWidget *panel = QgsPanelWidget::findParentPanel( this );
if ( panel && panel->dockMode() )
{
QgsSymbolSelectorWidget *dlg = new QgsSymbolSelectorWidget( symbol.release(), mStyle, mLayer, panel );
dlg->setContext( mContext );
dlg->setPanelTitle( category.label() );
connect( dlg, &QgsPanelWidget::widgetChanged, this, &QgsCategorizedSymbolRendererWidget::updateSymbolsFromWidget );
connect( dlg, &QgsPanelWidget::panelAccepted, this, &QgsCategorizedSymbolRendererWidget::cleanUpSymbolSelector );
openPanel( dlg );
QgsSymbolSelectorWidget *widget = QgsSymbolSelectorWidget::createWidgetWithSymbolOwnership( std::move( symbol ), mStyle, mLayer, panel );
widget->setContext( mContext );
widget->setPanelTitle( category.label() );
connect( widget, &QgsPanelWidget::widgetChanged, this, [ = ] { updateSymbolsFromWidget( widget ); } );
openPanel( widget );
}
else
{
Expand Down Expand Up @@ -1274,19 +1270,9 @@ void QgsCategorizedSymbolRendererWidget::pasteSymbolToSelection()
}
}

void QgsCategorizedSymbolRendererWidget::cleanUpSymbolSelector( QgsPanelWidget *container )
void QgsCategorizedSymbolRendererWidget::updateSymbolsFromWidget( QgsSymbolSelectorWidget *widget )
{
QgsSymbolSelectorWidget *dlg = qobject_cast<QgsSymbolSelectorWidget *>( container );
if ( !dlg )
return;

delete dlg->symbol();
}

void QgsCategorizedSymbolRendererWidget::updateSymbolsFromWidget()
{
QgsSymbolSelectorWidget *dlg = qobject_cast<QgsSymbolSelectorWidget *>( sender() );
mCategorizedSymbol.reset( dlg->symbol()->clone() );
mCategorizedSymbol.reset( widget->symbol()->clone() );

applyChangeToSymbol();
}
Expand Down
4 changes: 2 additions & 2 deletions src/gui/symbology/qgscategorizedsymbolrendererwidget.h
View file
Expand Up @@ -25,6 +25,7 @@

class QgsCategorizedSymbolRenderer;
class QgsRendererCategory;
class QgsSymbolSelectorWidget;

#include "ui_qgscategorizedsymbolrendererwidget.h"
#include "qgis_gui.h"
Expand Down Expand Up @@ -191,8 +192,7 @@ class GUI_EXPORT QgsCategorizedSymbolRendererWidget : public QgsRendererWidget,

private slots:

void cleanUpSymbolSelector( QgsPanelWidget *container );
void updateSymbolsFromWidget();
void updateSymbolsFromWidget( QgsSymbolSelectorWidget *widget );
void updateSymbolsFromButton();
void dataDefinedSizeLegend();

Expand Down
27 changes: 7 additions & 20 deletions src/gui/symbology/qgsgraduatedsymbolrendererwidget.cpp
View file
Expand Up @@ -962,19 +962,9 @@ void QgsGraduatedSymbolRendererWidget::setSymbolLevels( const QgsLegendSymbolLis
emit widgetChanged();
}

void QgsGraduatedSymbolRendererWidget::cleanUpSymbolSelector( QgsPanelWidget *container )
void QgsGraduatedSymbolRendererWidget::updateSymbolsFromWidget( QgsSymbolSelectorWidget *widget )
{
QgsSymbolSelectorWidget *dlg = qobject_cast<QgsSymbolSelectorWidget *>( container );
if ( !dlg )
return;

delete dlg->symbol();
}

void QgsGraduatedSymbolRendererWidget::updateSymbolsFromWidget()
{
QgsSymbolSelectorWidget *dlg = qobject_cast<QgsSymbolSelectorWidget *>( sender() );
mGraduatedSymbol.reset( dlg->symbol()->clone() );
mGraduatedSymbol.reset( widget->symbol()->clone() );

applyChangeToSymbol();
}
Expand Down Expand Up @@ -1210,14 +1200,11 @@ void QgsGraduatedSymbolRendererWidget::changeRangeSymbol( int rangeIdx )
QgsPanelWidget *panel = QgsPanelWidget::findParentPanel( this );
if ( panel && panel->dockMode() )
{
// bit tricky here - the widget doesn't take ownership of the symbol. So we need it to last for the duration of the
// panel's existence. Accordingly, just kinda give it ownership here, and clean up in cleanUpSymbolSelector
QgsSymbolSelectorWidget *dlg = new QgsSymbolSelectorWidget( newSymbol.release(), mStyle, mLayer, panel );
dlg->setContext( mContext );
dlg->setPanelTitle( range.label() );
connect( dlg, &QgsPanelWidget::widgetChanged, this, &QgsGraduatedSymbolRendererWidget::updateSymbolsFromWidget );
connect( dlg, &QgsPanelWidget::panelAccepted, this, &QgsGraduatedSymbolRendererWidget::cleanUpSymbolSelector );
openPanel( dlg );
QgsSymbolSelectorWidget *widget = QgsSymbolSelectorWidget::createWidgetWithSymbolOwnership( std::move( newSymbol ), mStyle, mLayer, panel );
widget->setContext( mContext );
widget->setPanelTitle( range.label() );
connect( widget, &QgsPanelWidget::widgetChanged, this, [ = ] { updateSymbolsFromWidget( widget ); } );
openPanel( widget );
}
else
{
Expand Down
4 changes: 2 additions & 2 deletions src/gui/symbology/qgsgraduatedsymbolrendererwidget.h
View file
Expand Up @@ -31,6 +31,7 @@

#include "qgis_gui.h"

class QgsSymbolSelectorWidget;

#ifndef SIP_RUN
/// @cond PRIVATE
Expand Down Expand Up @@ -144,8 +145,7 @@ class GUI_EXPORT QgsGraduatedSymbolRendererWidget : public QgsRendererWidget, pr
void mSizeUnitWidget_changed();
void methodComboBox_currentIndexChanged( int );
void updateMethodParameters();
void cleanUpSymbolSelector( QgsPanelWidget *container );
void updateSymbolsFromWidget();
void updateSymbolsFromWidget( QgsSymbolSelectorWidget *widget );
void dataDefinedSizeLegend();
void changeGraduatedSymbol();
void selectionChanged( const QItemSelection &selected, const QItemSelection &deselected );
Expand Down
8 changes: 8 additions & 0 deletions src/gui/symbology/qgssymbolselectordialog.cpp
View file
Expand Up @@ -386,6 +386,14 @@ QgsSymbolSelectorWidget::QgsSymbolSelectorWidget( QgsSymbol *symbol, QgsStyle *s
connect( QgsProject::instance(), static_cast < void ( QgsProject::* )( const QList<QgsMapLayer *>& layers ) > ( &QgsProject::layersWillBeRemoved ), this, &QgsSymbolSelectorWidget::layersAboutToBeRemoved );
}

QgsSymbolSelectorWidget *QgsSymbolSelectorWidget::createWidgetWithSymbolOwnership( std::unique_ptr<QgsSymbol> symbol, QgsStyle *style, QgsVectorLayer *vl, QWidget *parent )
{
QgsSymbolSelectorWidget *widget = new QgsSymbolSelectorWidget( symbol.get(), style, vl, parent );
// transfer ownership of symbol to widget, so that we are guaranteed it will last for the duration of the widget
widget->mOwnedSymbol = std::move( symbol );
return widget;
}

QMenu *QgsSymbolSelectorWidget::advancedMenu()
{
if ( !mAdvancedMenu )
Expand Down
11 changes: 11 additions & 0 deletions src/gui/symbology/qgssymbolselectordialog.h
View file
Expand Up @@ -105,6 +105,16 @@ class GUI_EXPORT QgsSymbolSelectorWidget: public QgsPanelWidget, private Ui::Qgs
*/
QgsSymbolSelectorWidget( QgsSymbol *symbol, QgsStyle *style, QgsVectorLayer *vl, QWidget *parent SIP_TRANSFERTHIS = nullptr );

// TODO QGIS 4.0 -- remove when normal constructor takes ownership

/**
* Creates a QgsSymbolSelectorWidget which takes ownership of a symbol and maintains
* the ownership for the life of the widget.
*
* \note Not available in Python bindings.
*/
static QgsSymbolSelectorWidget *createWidgetWithSymbolOwnership( std::unique_ptr< QgsSymbol > symbol, QgsStyle *style, QgsVectorLayer *vl, QWidget *parent SIP_TRANSFERTHIS = nullptr ) SIP_SKIP;

//! Returns menu for "advanced" button - create it if doesn't exist and show the advanced button
QMenu *advancedMenu();

Expand Down Expand Up @@ -258,6 +268,7 @@ class GUI_EXPORT QgsSymbolSelectorWidget: public QgsPanelWidget, private Ui::Qgs

QgsStyle *mStyle = nullptr;
QgsSymbol *mSymbol = nullptr;
std::unique_ptr< QgsSymbol > mOwnedSymbol;
QMenu *mAdvancedMenu = nullptr;
QAction *mLockColorAction = nullptr;
QAction *mLockSelectionColorAction = nullptr;
Expand Down
25 changes: 7 additions & 18 deletions src/gui/vectortile/qgsvectortilebasicrendererwidget.cpp
View file
Expand Up @@ -504,12 +504,11 @@ void QgsVectorTileBasicRendererWidget::editStyleAtIndex( const QModelIndex &prox
QgsPanelWidget *panel = QgsPanelWidget::findParentPanel( this );
if ( panel && panel->dockMode() )
{
QgsSymbolSelectorWidget *dlg = new QgsSymbolSelectorWidget( symbol.release(), QgsStyle::defaultStyle(), vectorLayer, panel );
dlg->setContext( context );
dlg->setPanelTitle( style.styleName() );
connect( dlg, &QgsPanelWidget::widgetChanged, this, &QgsVectorTileBasicRendererWidget::updateSymbolsFromWidget );
connect( dlg, &QgsPanelWidget::panelAccepted, this, &QgsVectorTileBasicRendererWidget::cleanUpSymbolSelector );
openPanel( dlg );
QgsSymbolSelectorWidget *widget = QgsSymbolSelectorWidget::createWidgetWithSymbolOwnership( std::move( symbol ), QgsStyle::defaultStyle(), vectorLayer, panel );
widget->setContext( context );
widget->setPanelTitle( style.styleName() );
connect( widget, &QgsPanelWidget::widgetChanged, this, [ = ] { updateSymbolsFromWidget( widget ); } );
openPanel( widget );
}
else
{
Expand All @@ -526,30 +525,20 @@ void QgsVectorTileBasicRendererWidget::editStyleAtIndex( const QModelIndex &prox
}
}

void QgsVectorTileBasicRendererWidget::updateSymbolsFromWidget()
void QgsVectorTileBasicRendererWidget::updateSymbolsFromWidget( QgsSymbolSelectorWidget *widget )
{
const int index = mProxyModel->mapToSource( viewStyles->selectionModel()->currentIndex() ).row();
if ( index < 0 )
return;

QgsVectorTileBasicRendererStyle style = mRenderer->style( index );

QgsSymbolSelectorWidget *dlg = qobject_cast<QgsSymbolSelectorWidget *>( sender() );
style.setSymbol( dlg->symbol()->clone() );
style.setSymbol( widget->symbol()->clone() );

mRenderer->setStyle( index, style );
emit widgetChanged();
}

void QgsVectorTileBasicRendererWidget::cleanUpSymbolSelector( QgsPanelWidget *container )
{
QgsSymbolSelectorWidget *dlg = qobject_cast<QgsSymbolSelectorWidget *>( container );
if ( !dlg )
return;

delete dlg->symbol();
}

void QgsVectorTileBasicRendererWidget::removeStyle()
{
const QModelIndexList sel = viewStyles->selectionModel()->selectedIndexes();
Expand Down
4 changes: 2 additions & 2 deletions src/gui/vectortile/qgsvectortilebasicrendererwidget.h
View file
Expand Up @@ -33,6 +33,7 @@ class QgsVectorTileLayer;
class QgsMapCanvas;
class QgsMessageBar;
class QgsVectorTileBasicRendererProxyModel;
class QgsSymbolSelectorWidget;

/**
* \ingroup gui
Expand All @@ -59,8 +60,7 @@ class GUI_EXPORT QgsVectorTileBasicRendererWidget : public QgsMapLayerConfigWidg
void editStyleAtIndex( const QModelIndex &index );
void removeStyle();

void updateSymbolsFromWidget();
void cleanUpSymbolSelector( QgsPanelWidget *container );
void updateSymbolsFromWidget( QgsSymbolSelectorWidget *widget );

private:
QPointer< QgsVectorTileLayer > mVTLayer;
Expand Down

0 comments on commit 784c844

Please sign in to comment.