Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix potential crash in symbol list widget
Possible fix for a crash obtained with incomplete crash report
trace... but it doesn't hurt to be safe anyway and guard against
nullptr symbols.

Also safer memory management via unique_ptrs, which fixes a couple
of leaks in symbol handling.

(cherry-picked from 7973408)
  • Loading branch information
nyalldawson committed Jul 19, 2018
1 parent b166e57 commit 850e70b
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 59 deletions.
22 changes: 12 additions & 10 deletions src/app/qgsprojectproperties.cpp
Expand Up @@ -1761,7 +1761,10 @@ void QgsProjectProperties::populateStyles()
for ( int i = 0; i < symbolNames.count(); ++i )
{
QString name = symbolNames[i];
QgsSymbol *symbol = mStyle->symbol( name );
std::unique_ptr< QgsSymbol > symbol( mStyle->symbol( name ) );
if ( !symbol )
continue;

QComboBox *cbo = nullptr;
switch ( symbol->type() )
{
Expand All @@ -1780,10 +1783,9 @@ void QgsProjectProperties::populateStyles()
}
if ( cbo )
{
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( symbol, cbo->iconSize() );
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( symbol.get(), cbo->iconSize() );
cbo->addItem( icon, name );
}
delete symbol;
}

// populate color ramps
Expand Down Expand Up @@ -1857,27 +1859,27 @@ void QgsProjectProperties::editSymbol( QComboBox *cbo )
QMessageBox::information( this, QLatin1String( "" ), tr( "Select a valid symbol" ) );
return;
}
QgsSymbol *symbol = mStyle->symbol( symbolName );
std::unique_ptr< QgsSymbol > symbol( mStyle->symbol( symbolName ) );
if ( ! symbol )
{
QMessageBox::warning( this, QLatin1String( "" ), tr( "Invalid symbol : " ) + symbolName );
return;
}

// let the user edit the symbol and update list when done
QgsSymbolSelectorDialog dlg( symbol, mStyle, nullptr, this );
QgsSymbolSelectorDialog dlg( symbol.get(), mStyle, nullptr, this );
if ( dlg.exec() == 0 )
{
delete symbol;
return;
}

// by adding symbol to style with the same name the old effectively gets overwritten
mStyle->addSymbol( symbolName, symbol );

// update icon
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( symbol, cbo->iconSize() );
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( symbol.get(), cbo->iconSize() );
cbo->setItemIcon( cbo->currentIndex(), icon );

// by adding symbol to style with the same name the old effectively gets overwritten
mStyle->addSymbol( symbolName, symbol.release() );

}

void QgsProjectProperties::resetPythonMacros()
Expand Down
20 changes: 10 additions & 10 deletions src/core/symbology/qgssymbol.cpp
Expand Up @@ -266,40 +266,40 @@ void QgsSymbol::setMapUnitScale( const QgsMapUnitScale &scale )

QgsSymbol *QgsSymbol::defaultSymbol( QgsWkbTypes::GeometryType geomType )
{
QgsSymbol *s = nullptr;
std::unique_ptr< QgsSymbol > s;

// override global default if project has a default for this type
QString defaultSymbol;
switch ( geomType )
{
case QgsWkbTypes::PointGeometry :
defaultSymbol = QgsProject::instance()->readEntry( QStringLiteral( "DefaultStyles" ), QStringLiteral( "/Marker" ), QLatin1String( "" ) );
defaultSymbol = QgsProject::instance()->readEntry( QStringLiteral( "DefaultStyles" ), QStringLiteral( "/Marker" ) );
break;
case QgsWkbTypes::LineGeometry :
defaultSymbol = QgsProject::instance()->readEntry( QStringLiteral( "DefaultStyles" ), QStringLiteral( "/Line" ), QLatin1String( "" ) );
defaultSymbol = QgsProject::instance()->readEntry( QStringLiteral( "DefaultStyles" ), QStringLiteral( "/Line" ) );
break;
case QgsWkbTypes::PolygonGeometry :
defaultSymbol = QgsProject::instance()->readEntry( QStringLiteral( "DefaultStyles" ), QStringLiteral( "/Fill" ), QLatin1String( "" ) );
defaultSymbol = QgsProject::instance()->readEntry( QStringLiteral( "DefaultStyles" ), QStringLiteral( "/Fill" ) );
break;
default:
break;
}
if ( !defaultSymbol.isEmpty() )
s = QgsStyle::defaultStyle()->symbol( defaultSymbol );
s.reset( QgsStyle::defaultStyle()->symbol( defaultSymbol ) );

// if no default found for this type, get global default (as previously)
if ( ! s )
if ( !s )
{
switch ( geomType )
{
case QgsWkbTypes::PointGeometry:
s = new QgsMarkerSymbol();
s = qgis::make_unique< QgsMarkerSymbol >();
break;
case QgsWkbTypes::LineGeometry:
s = new QgsLineSymbol();
s = qgis::make_unique< QgsLineSymbol >();
break;
case QgsWkbTypes::PolygonGeometry:
s = new QgsFillSymbol();
s = qgis::make_unique< QgsFillSymbol >();
break;
default:
QgsDebugMsg( "unknown layer's geometry type" );
Expand All @@ -326,7 +326,7 @@ QgsSymbol *QgsSymbol::defaultSymbol( QgsWkbTypes::GeometryType geomType )
s->setColor( QgsApplication::colorSchemeRegistry()->fetchRandomStyleColor() );
}

return s;
return s.release();
}

QgsSymbolLayer *QgsSymbol::symbolLayer( int layer )
Expand Down
4 changes: 2 additions & 2 deletions src/gui/symbology/qgscategorizedsymbolrendererwidget.cpp
Expand Up @@ -923,14 +923,14 @@ int QgsCategorizedSymbolRendererWidget::matchToSymbols( QgsStyle *style )
for ( int catIdx = 0; catIdx < mRenderer->categories().count(); ++catIdx )
{
QString val = mRenderer->categories().at( catIdx ).value().toString();
QgsSymbol *symbol = style->symbol( val );
std::unique_ptr< QgsSymbol > symbol( style->symbol( val ) );
if ( symbol &&
( ( symbol->type() == QgsSymbol::Marker && mLayer->geometryType() == QgsWkbTypes::PointGeometry )
|| ( symbol->type() == QgsSymbol::Line && mLayer->geometryType() == QgsWkbTypes::LineGeometry )
|| ( symbol->type() == QgsSymbol::Fill && mLayer->geometryType() == QgsWkbTypes::PolygonGeometry ) ) )
{
matched++;
mRenderer->updateCategorySymbol( catIdx, symbol->clone() );
mRenderer->updateCategorySymbol( catIdx, symbol.release() );
}
}
mModel->updateSymbology();
Expand Down
52 changes: 33 additions & 19 deletions src/gui/symbology/qgsstyleexportimportdialog.cpp
Expand Up @@ -205,17 +205,18 @@ bool QgsStyleExportImportDialog::populateStyles( QgsStyle *style )
{
name = styleNames[i];
QStringList tags = style->tagsOfSymbol( QgsStyle::SymbolEntity, name );
QgsSymbol *symbol = style->symbol( name );
std::unique_ptr< QgsSymbol > symbol( style->symbol( name ) );
if ( !symbol )
continue;
QStandardItem *item = new QStandardItem( name );
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( symbol, listItems->iconSize(), 15 );
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( symbol.get(), listItems->iconSize(), 15 );
item->setIcon( icon );
item->setToolTip( QStringLiteral( "<b>%1</b><br><i>%2</i>" ).arg( name, tags.count() > 0 ? tags.join( QStringLiteral( ", " ) ) : tr( "Not tagged" ) ) );
// Set font to 10points to show reasonable text
QFont itemFont = item->font();
itemFont.setPointSize( 10 );
item->setFont( itemFont );
model->appendRow( item );
delete symbol;
}

// and color ramps
Expand All @@ -237,10 +238,8 @@ bool QgsStyleExportImportDialog::populateStyles( QgsStyle *style )
void QgsStyleExportImportDialog::moveStyles( QModelIndexList *selection, QgsStyle *src, QgsStyle *dst )
{
QString symbolName;
QgsSymbol *symbol = nullptr;
QStringList symbolTags;
bool symbolFavorite;
QgsColorRamp *ramp = nullptr;
QModelIndex index;
bool isSymbol = true;
bool prompt = true;
Expand All @@ -255,7 +254,8 @@ void QgsStyleExportImportDialog::moveStyles( QModelIndexList *selection, QgsStyl
{
index = selection->at( i );
symbolName = index.model()->data( index, 0 ).toString();
symbol = src->symbol( symbolName );
std::unique_ptr< QgsSymbol > symbol( src->symbol( symbolName ) );
std::unique_ptr< QgsColorRamp > ramp;

if ( !mIgnoreXMLTags->isChecked() )
{
Expand All @@ -279,7 +279,7 @@ void QgsStyleExportImportDialog::moveStyles( QModelIndexList *selection, QgsStyl
if ( !symbol )
{
isSymbol = false;
ramp = src->colorRamp( symbolName );
ramp.reset( src->colorRamp( symbolName ) );
}

if ( isSymbol )
Expand All @@ -297,9 +297,12 @@ void QgsStyleExportImportDialog::moveStyles( QModelIndexList *selection, QgsStyl
case QMessageBox::No:
continue;
case QMessageBox::Yes:
dst->addSymbol( symbolName, symbol );
dst->saveSymbol( symbolName, symbol, symbolFavorite, symbolTags );
{
QgsSymbol *newSymbol = symbol.get();
dst->addSymbol( symbolName, symbol.release() );
dst->saveSymbol( symbolName, newSymbol, symbolFavorite, symbolTags );
continue;
}
case QMessageBox::YesToAll:
prompt = false;
overwrite = true;
Expand All @@ -313,17 +316,21 @@ void QgsStyleExportImportDialog::moveStyles( QModelIndexList *selection, QgsStyl

if ( dst->symbolNames().contains( symbolName ) && overwrite )
{
dst->addSymbol( symbolName, symbol );
dst->saveSymbol( symbolName, symbol, symbolFavorite, symbolTags );
QgsSymbol *newSymbol = symbol.get();
dst->addSymbol( symbolName, symbol.release() );
dst->saveSymbol( symbolName, newSymbol, symbolFavorite, symbolTags );
continue;
}
else if ( dst->symbolNames().contains( symbolName ) && !overwrite )
{
continue;
}
else
{
dst->addSymbol( symbolName, symbol );
dst->saveSymbol( symbolName, symbol, symbolFavorite, symbolTags );
QgsSymbol *newSymbol = symbol.get();
dst->addSymbol( symbolName, symbol.release() );
dst->saveSymbol( symbolName, newSymbol, symbolFavorite, symbolTags );
continue;
}
}
else
Expand All @@ -341,9 +348,12 @@ void QgsStyleExportImportDialog::moveStyles( QModelIndexList *selection, QgsStyl
case QMessageBox::No:
continue;
case QMessageBox::Yes:
dst->addColorRamp( symbolName, ramp );
dst->saveColorRamp( symbolName, ramp, symbolFavorite, symbolTags );
{
QgsColorRamp *newRamp = ramp.get();
dst->addColorRamp( symbolName, ramp.release() );
dst->saveColorRamp( symbolName, newRamp, symbolFavorite, symbolTags );
continue;
}
case QMessageBox::YesToAll:
prompt = false;
overwrite = true;
Expand All @@ -357,17 +367,21 @@ void QgsStyleExportImportDialog::moveStyles( QModelIndexList *selection, QgsStyl

if ( dst->colorRampNames().contains( symbolName ) && overwrite )
{
dst->addColorRamp( symbolName, ramp );
dst->saveColorRamp( symbolName, ramp, symbolFavorite, symbolTags );
QgsColorRamp *newRamp = ramp.get();
dst->addColorRamp( symbolName, ramp.release() );
dst->saveColorRamp( symbolName, newRamp, symbolFavorite, symbolTags );
continue;
}
else if ( dst->colorRampNames().contains( symbolName ) && !overwrite )
{
continue;
}
else
{
dst->addColorRamp( symbolName, ramp );
dst->saveColorRamp( symbolName, ramp, symbolFavorite, symbolTags );
QgsColorRamp *newRamp = ramp.get();
dst->addColorRamp( symbolName, ramp.release() );
dst->saveColorRamp( symbolName, newRamp, symbolFavorite, symbolTags );
continue;
}
}
}
Expand Down
17 changes: 8 additions & 9 deletions src/gui/symbology/qgsstylemanagerdialog.cpp
Expand Up @@ -279,20 +279,19 @@ void QgsStyleManagerDialog::populateSymbols( const QStringList &symbolNames, boo
for ( int i = 0; i < symbolNames.count(); ++i )
{
QString name = symbolNames[i];
QgsSymbol *symbol = mStyle->symbol( name );
std::unique_ptr< QgsSymbol > symbol( mStyle->symbol( name ) );
if ( symbol && symbol->type() == type )
{
QStringList tags = mStyle->tagsOfSymbol( QgsStyle::SymbolEntity, name );
QStandardItem *item = new QStandardItem( name );
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( symbol, listItems->iconSize(), 18 );
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( symbol.get(), listItems->iconSize(), 18 );
item->setIcon( icon );
item->setData( name ); // used to find out original name when user edited the name
item->setCheckable( check );
item->setToolTip( QStringLiteral( "<b>%1</b><br><i>%2</i>" ).arg( name, tags.count() > 0 ? tags.join( QStringLiteral( ", " ) ) : tr( "Not tagged" ) ) );
// add to model
model->appendRow( item );
}
delete symbol;
}
selectedSymbolsChanged( QItemSelection(), QItemSelection() );
symbolSelected( listItems->currentIndex() );
Expand Down Expand Up @@ -654,18 +653,17 @@ bool QgsStyleManagerDialog::editSymbol()
if ( symbolName.isEmpty() )
return false;

QgsSymbol *symbol = mStyle->symbol( symbolName );
std::unique_ptr< QgsSymbol > symbol( mStyle->symbol( symbolName ) );

// let the user edit the symbol and update list when done
QgsSymbolSelectorDialog dlg( symbol, mStyle, nullptr, this );
QgsSymbolSelectorDialog dlg( symbol.get(), mStyle, nullptr, this );
if ( dlg.exec() == 0 )
{
delete symbol;
return false;
}

// by adding symbol to style with the same name the old effectively gets overwritten
mStyle->addSymbol( symbolName, symbol, true );
mStyle->addSymbol( symbolName, symbol.release(), true );
mModified = true;
return true;
}
Expand Down Expand Up @@ -870,8 +868,9 @@ void QgsStyleManagerDialog::exportSelectedItemsImages( const QString &dir, const
{
QString name = index.data().toString();
QString path = dir + '/' + name + '.' + format;
QgsSymbol *sym = mStyle->symbol( name );
sym->exportImage( path, format, size );
std::unique_ptr< QgsSymbol > sym( mStyle->symbol( name ) );
if ( sym )
sym->exportImage( path, format, size );
}
}

Expand Down
16 changes: 7 additions & 9 deletions src/gui/symbology/qgssymbolslistwidget.cpp
Expand Up @@ -295,10 +295,9 @@ void QgsSymbolsListWidget::populateSymbols( const QStringList &names )

for ( int i = 0; i < names.count(); i++ )
{
QgsSymbol *s = mStyle->symbol( names[i] );
if ( s->type() != mSymbol->type() )
std::unique_ptr< QgsSymbol > s( mStyle->symbol( names[i] ) );
if ( !s || s->type() != mSymbol->type() )
{
delete s;
continue;
}
QStringList tags = mStyle->tagsOfSymbol( QgsStyle::SymbolEntity, names[i] );
Expand All @@ -312,11 +311,10 @@ void QgsSymbolsListWidget::populateSymbols( const QStringList &names )
itemFont.setPointSize( 10 );
item->setFont( itemFont );
// create preview icon
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( s, previewSize, 15 );
QIcon icon = QgsSymbolLayerUtils::symbolPreviewIcon( s.get(), previewSize, 15 );
item->setIcon( icon );
// add to model
model->appendRow( item );
delete s;
}
}

Expand Down Expand Up @@ -637,7 +635,10 @@ void QgsSymbolsListWidget::setSymbolFromStyle( const QModelIndex &index )
QString symbolName = index.data( Qt::UserRole ).toString();
lblSymbolName->setText( symbolName );
// get new instance of symbol from style
QgsSymbol *s = mStyle->symbol( symbolName );
std::unique_ptr< QgsSymbol > s( mStyle->symbol( symbolName ) );
if ( !s )
return;

// remove all symbol layers from original symbolgroupsCombo
while ( mSymbol->symbolLayerCount() )
mSymbol->deleteSymbolLayer( 0 );
Expand All @@ -649,9 +650,6 @@ void QgsSymbolsListWidget::setSymbolFromStyle( const QModelIndex &index )
}
mSymbol->setOpacity( s->opacity() );

// delete the temporary symbol
delete s;

updateSymbolInfo();
emit changed();
}
Expand Down

0 comments on commit 850e70b

Please sign in to comment.