Skip to content
Permalink
Browse files
Color ramp dialogs no longer edit ramps in place
Now the dialogs use a copy of the ramp, and the edited
ramp is retrieved by calling ramp() on the dialog after
it is executed.

Avoids pointer lifetime issues by storing and working
on a ramp pointer which the dialog does not have ownership
on.

Also fix a bunch of leaks relating to cloning color ramps.
  • Loading branch information
nyalldawson committed Aug 24, 2016
1 parent 7413db7 commit cf9292c
Show file tree
Hide file tree
Showing 25 changed files with 266 additions and 220 deletions.
@@ -295,6 +295,13 @@ variant instead.</li>
<li>GenericDataSourceURI has been renamed to GenericDataSourceUri</li>
</ul>

\subsection qgis_api_break_3_0_QgsColorBrewerColorRampDialog QgsColorBrewerColorRampDialog

<ul>
<li>The dialog no longer edits a color ramp in place. Instead, a copy of the ramp is edited
and the new ramp can be retrieved after executing the dialog by calling ramp().</li>
</ul>

\subsection qgis_api_break_3_0_QgsComposerLabel QgsComposerLabel

<ul>
@@ -389,6 +396,13 @@ plugins calling these methods will need to be updated.</li>
be returned in place of a null pointer.</li>
</ul>

\subsection qgis_api_break_3_0_QgsCptCityColorRampDialog QgsCptCityColorRampDialog

<ul>
<li>The dialog no longer edits a color ramp in place. Instead, a copy of the ramp is edited
and the new ramp can be retrieved after executing the dialog by calling ramp().</li>
</ul>

\subsection qgis_api_break_3_0_QgsCptCitySelectionItem QgsCptCitySelectionItem

<ul>
@@ -559,6 +573,13 @@ method to determine if a geometry is valid.</li>
a QgsGeometry value rather than a pointer.</li>
</ul>

\subsection qgis_api_break_3_0_QgsGradientColorRampDialog QgsGradientColorRampDialog

<ul>
<li>The dialog no longer edits a color ramp in place. Instead, a copy of the ramp is edited
and the new ramp can be retrieved after executing the dialog by calling ramp().</li>
</ul>

\subsection qgis_api_break_3_0_QgsGraphBuilderInterface QgsGraphBuilderInterface

<ul>
@@ -604,6 +625,13 @@ plugins calling this method will need to be updated.</li>
<li>writeCommonXML() has been renamed to writeCommonXml()</li>
</ul>

\subsection qgis_api_break_3_0_QgsLimitedRandomRampDialog QgsLimitedRandomColorRampDialog

<ul>
<li>The dialog no longer edits a color ramp in place. Instead, a copy of the ramp is edited
and the new ramp can be retrieved after executing the dialog by calling ramp().</li>
</ul>

\subsection qgis_api_break_3_0_QgsMapCanvas QgsMapCanvas

<ul>
@@ -16,11 +16,10 @@ class QgsStyle : QObject
*/
enum StyleEntity { SymbolEntity, GroupEntity, TagEntity, ColorrampEntity, SmartgroupEntity };

//! add color ramp to style. takes ramp's ownership
/*!
/** Adds a color ramp to the style. Calling this method takes the ramp's ownership.
* \note Adding a color ramp with the name of existing one replaces it.
* \param name is the name of the color ramp being added or updated
* \param colorRamp is the color ramp
* \param colorRamp is the color ramp. Ownership is transferred.
* \param update set to true when the style DB has to be updated, by default it is false
* \return success status of the operation
*/
@@ -71,8 +70,10 @@ class QgsStyle : QObject
//! remove all contents of the style
void clear();

//! return a NEW copy of color ramp
QgsColorRamp* colorRamp( const QString& name ) /Factory/;
/** Returns a new copy of the specified color ramp. The caller
* takes responsibility for deleting the returned object.
*/
QgsColorRamp* colorRamp( const QString& name ) const /Factory/;

//! return count of color ramps
int colorRampCount();
@@ -5,7 +5,9 @@ class QgsColorBrewerColorRampDialog : QDialog
%End

public:
QgsColorBrewerColorRampDialog( QgsColorBrewerColorRamp* ramp, QWidget* parent /TransferThis/ = NULL );
QgsColorBrewerColorRampDialog( const QgsColorBrewerColorRamp& ramp, QWidget* parent /TransferThis/ = nullptr );

QgsColorBrewerColorRamp ramp() const;

public slots:
void setSchemeName();
@@ -5,7 +5,10 @@ class QgsGradientColorRampDialog : QDialog
%End

public:
QgsGradientColorRampDialog( QgsGradientColorRamp* ramp, QWidget* parent /TransferThis/ = NULL );
QgsGradientColorRampDialog( const QgsGradientColorRamp& ramp, QWidget* parent /TransferThis/ = nullptr );
~QgsGradientColorRampDialog();

QgsGradientColorRamp ramp() const;

public slots:
void setColor1( const QColor& color );
@@ -5,7 +5,9 @@ class QgsLimitedRandomColorRampDialog : QDialog
%End

public:
QgsLimitedRandomColorRampDialog( QgsLimitedRandomColorRamp* ramp, QWidget* parent /TransferThis/ = NULL );
QgsLimitedRandomColorRampDialog( const QgsLimitedRandomColorRamp& ramp, QWidget* parent /TransferThis/ = nullptr );

QgsLimitedRandomColorRamp ramp() const;

public slots:
void setCount( int val );
@@ -11,11 +11,18 @@ class QgsColorRampComboBox : QComboBox
//! initialize the combo box with color ramps from the style
void populate( QgsStyle* style );

//! add/select color ramp which was used previously by the renderer
void setSourceColorRamp( QgsColorRamp* sourceRamp );
/** Adds or selects the current color ramp to show in the combo box. The ramp appears
* in the combo box as the "source" ramp.
* @param sourceRamp color ramp, ownership is transferred.
* @see currentColorRamp()
*/
void setSourceColorRamp( QgsColorRamp* sourceRamp /Transfer/ );

//! return new instance of the current color ramp or NULL if there is no active color ramp
QgsColorRamp* currentColorRamp();
/** Returns a new instance of the current color ramp or NULL if there is no active color ramp.
* The caller takes responsibility for deleting the returned value.
* @see setSourceColorRamp()
*/
QgsColorRamp* currentColorRamp() const /Factory/;

/** Returns true if the current selection in the combo box is the option for creating
* a new color ramp
@@ -5,9 +5,13 @@ class QgsCptCityColorRampDialog : QDialog
%End

public:
QgsCptCityColorRampDialog( QgsCptCityColorRamp* ramp, QWidget* parent /TransferThis/ = NULL );

QgsCptCityColorRampDialog( const QgsCptCityColorRamp& ramp, QWidget* parent /TransferThis/ = nullptr );
~QgsCptCityColorRampDialog();

QgsCptCityColorRamp ramp() const;


QString selectedName() const;

bool saveAsGradientRamp() const;
@@ -1713,10 +1713,9 @@ void QgsProjectProperties::populateStyles()
for ( int i = 0; i < colorRamps.count(); ++i )
{
QString name = colorRamps[i];
QgsColorRamp* ramp = mStyle->colorRamp( name );
QIcon icon = QgsSymbolLayerUtils::colorRampPreviewIcon( ramp, cboStyleColorRamp->iconSize() );
QScopedPointer< QgsColorRamp > ramp( mStyle->colorRamp( name ) );
QIcon icon = QgsSymbolLayerUtils::colorRampPreviewIcon( ramp.data(), cboStyleColorRamp->iconSize() );
cboStyleColorRamp->addItem( icon, name );
delete ramp;
}

// set current index if found
@@ -254,7 +254,7 @@ bool QgsStyle::removeColorRamp( const QString& name )
return true;
}

QgsColorRamp* QgsStyle::colorRamp( const QString& name )
QgsColorRamp* QgsStyle::colorRamp( const QString& name ) const
{
const QgsColorRamp *ramp = colorRampRef( name );
return ramp ? ramp->clone() : nullptr;
@@ -1536,7 +1536,8 @@ bool QgsStyle::updateSymbol( StyleEntity type, const QString& name )
return false;
}

symEl = QgsSymbolLayerUtils::saveColorRamp( name, colorRamp( name ), doc );
QScopedPointer< QgsColorRamp > ramp( colorRamp( name ) );
symEl = QgsSymbolLayerUtils::saveColorRamp( name, ramp.data(), doc );
if ( symEl.isNull() )
{
QgsDebugMsg( "Couldn't convert color ramp to valid XML!" );
@@ -82,11 +82,10 @@ class CORE_EXPORT QgsStyle : public QObject
*/
enum StyleEntity { SymbolEntity, GroupEntity, TagEntity, ColorrampEntity, SmartgroupEntity };

//! add color ramp to style. takes ramp's ownership
/*!
/** Adds a color ramp to the style. Calling this method takes the ramp's ownership.
* \note Adding a color ramp with the name of existing one replaces it.
* \param name is the name of the color ramp being added or updated
* \param colorRamp is the Vector color ramp
* \param colorRamp is the color ramp. Ownership is transferred.
* \param update set to true when the style DB has to be updated, by default it is false
* \return success status of the operation
*/
@@ -137,8 +136,10 @@ class CORE_EXPORT QgsStyle : public QObject
//! remove all contents of the style
void clear();

//! return a NEW copy of color ramp
QgsColorRamp* colorRamp( const QString& name );
/** Returns a new copy of the specified color ramp. The caller
* takes responsibility for deleting the returned object.
*/
QgsColorRamp* colorRamp( const QString& name ) const;

//! return count of color ramps
int colorRampCount();
@@ -31,7 +31,7 @@ static void updateColorButton( QAbstractButton* button, QColor color )
/////////


QgsColorBrewerColorRampDialog::QgsColorBrewerColorRampDialog( QgsColorBrewerColorRamp* ramp, QWidget* parent )
QgsColorBrewerColorRampDialog::QgsColorBrewerColorRampDialog( const QgsColorBrewerColorRamp& ramp, QWidget* parent )
: QDialog( parent )
, mRamp( ramp )
{
@@ -51,9 +51,9 @@ QgsColorBrewerColorRampDialog::QgsColorBrewerColorRampDialog( QgsColorBrewerColo
cboSchemeName->addItem( icon, schemeName );
}

cboSchemeName->setCurrentIndex( cboSchemeName->findText( ramp->schemeName() ) );
cboSchemeName->setCurrentIndex( cboSchemeName->findText( mRamp.schemeName() ) );
populateVariants();
cboColors->setCurrentIndex( cboColors->findText( QString::number( ramp->colors() ) ) );
cboColors->setCurrentIndex( cboColors->findText( QString::number( mRamp.colors() ) ) );

connect( cboSchemeName, SIGNAL( currentIndexChanged( int ) ), this, SLOT( setSchemeName() ) );
connect( cboColors, SIGNAL( currentIndexChanged( int ) ), this, SLOT( setColors() ) );
@@ -86,21 +86,21 @@ void QgsColorBrewerColorRampDialog::populateVariants()
void QgsColorBrewerColorRampDialog::updatePreview()
{
QSize size( 300, 40 );
lblPreview->setPixmap( QgsSymbolLayerUtils::colorRampPreviewPixmap( mRamp, size ) );
lblPreview->setPixmap( QgsSymbolLayerUtils::colorRampPreviewPixmap( &mRamp, size ) );
}

void QgsColorBrewerColorRampDialog::setSchemeName()
{
// populate list of variants
populateVariants();

mRamp->setSchemeName( cboSchemeName->currentText() );
mRamp.setSchemeName( cboSchemeName->currentText() );
updatePreview();
}

void QgsColorBrewerColorRampDialog::setColors()
{
int num = cboColors->currentText().toInt();
mRamp->setColors( num );
mRamp.setColors( num );
updatePreview();
}
@@ -18,6 +18,7 @@

#include <QDialog>

#include "qgscolorramp.h"
#include "ui_qgscolorbrewercolorrampdialogbase.h"

class QgsColorBrewerColorRamp;
@@ -30,7 +31,9 @@ class GUI_EXPORT QgsColorBrewerColorRampDialog : public QDialog, private Ui::Qgs
Q_OBJECT

public:
QgsColorBrewerColorRampDialog( QgsColorBrewerColorRamp* ramp, QWidget* parent = nullptr );
QgsColorBrewerColorRampDialog( const QgsColorBrewerColorRamp& ramp, QWidget* parent = nullptr );

QgsColorBrewerColorRamp ramp() const { return mRamp; }

public slots:
void setSchemeName();
@@ -42,7 +45,7 @@ class GUI_EXPORT QgsColorBrewerColorRampDialog : public QDialog, private Ui::Qgs

void updatePreview();

QgsColorBrewerColorRamp* mRamp;
QgsColorBrewerColorRamp mRamp;
};

#endif

0 comments on commit cf9292c

Please sign in to comment.