Skip to content

Commit

Permalink
Fix "feature rendering order" checkbox is not honored (fix #14323)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nyalldawson
nyalldawson committed Feb 18, 2016
1 parent f36214c commit 4285d70
Show file tree
Hide file tree
Showing 14 changed files with 91 additions and 14 deletions.
21 changes: 21 additions & 0 deletions python/core/symbology-ng/qgsrendererv2.sip
View file
Original file line number Diff line number Diff line change
Expand Up @@ -352,15 +352,36 @@ class QgsFeatureRendererV2
/**
* Get the order in which features shall be processed by this renderer.
* @note added in QGIS 2.14
* @note this property has no effect if orderByEnabled() is false
* @see orderByEnabled()
*/
QgsFeatureRequest::OrderBy orderBy() const;

/**
* Define the order in which features shall be processed by this renderer.
* @note this property has no effect if orderByEnabled() is false
* @note added in QGIS 2.14
* @see setOrderByEnabled()
*/
void setOrderBy( const QgsFeatureRequest::OrderBy& orderBy );

/**
* Returns whether custom ordering will be applied before features are processed by this renderer.
* @note added in QGIS 2.14
* @see orderBy()
* @see setOrderByEnabled()
*/
bool orderByEnabled() const;

/**
* Sets whether custom ordering should be applied before features are processed by this renderer.
* @param enabled set to true to enable custom feature ordering
* @note added in QGIS 2.14
* @see setOrderBy()
* @see orderByEnabled()
*/
void setOrderByEnabled( bool enabled );

protected:
QgsFeatureRendererV2( const QString& type );

Expand Down
9 changes: 5 additions & 4 deletions src/core/qgsvectorlayerrenderer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,14 @@ bool QgsVectorLayerRenderer::render()
QgsRectangle requestExtent = mContext.extent();
mRendererV2->modifyRequestExtent( requestExtent, mContext );

QgsFeatureRequest::OrderBy orderBy = mRendererV2->orderBy();

QgsFeatureRequest featureRequest = QgsFeatureRequest()
.setFilterRect( requestExtent )
.setSubsetOfAttributes( mAttrNames, mFields )
.setExpressionContext( mContext.expressionContext() )
.setOrderBy( orderBy );
.setExpressionContext( mContext.expressionContext() );
if ( mRendererV2->orderByEnabled() )
{
featureRequest.setOrderBy( mRendererV2->orderBy() );
}

const QgsFeatureFilterProvider* featureFilterProvider = mContext.featureFilterProvider();
if ( featureFilterProvider )
Expand Down
2 changes: 2 additions & 0 deletions src/core/symbology-ng/qgscategorizedsymbolrendererv2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,7 @@ QDomElement QgsCategorizedSymbolRendererV2::save( QDomDocument& doc )
mOrderBy.save( orderBy );
rendererElem.appendChild( orderBy );
}
rendererElem.setAttribute( "enableorderby", ( mOrderByEnabled ? "1" : "0" ) );

return rendererElem;
}
Expand Down Expand Up @@ -1052,6 +1053,7 @@ QgsCategorizedSymbolRendererV2* QgsCategorizedSymbolRendererV2::convertFromRende
}

r->setOrderBy( renderer->orderBy() );
r->setOrderByEnabled( renderer->orderByEnabled() );

return r;
}
2 changes: 2 additions & 0 deletions src/core/symbology-ng/qgsgraduatedsymbolrendererv2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1201,6 +1201,7 @@ QDomElement QgsGraduatedSymbolRendererV2::save( QDomDocument& doc )
mOrderBy.save( orderBy );
rendererElem.appendChild( orderBy );
}
rendererElem.setAttribute( "enableorderby", ( mOrderByEnabled ? "1" : "0" ) );

return rendererElem;
}
Expand Down Expand Up @@ -1745,6 +1746,7 @@ QgsGraduatedSymbolRendererV2* QgsGraduatedSymbolRendererV2::convertFromRenderer(
}

r->setOrderBy( renderer->orderBy() );
r->setOrderByEnabled( renderer->orderByEnabled() );

return r;
}
Expand Down
2 changes: 2 additions & 0 deletions src/core/symbology-ng/qgsheatmaprenderer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ QDomElement QgsHeatmapRenderer::save( QDomDocument& doc )
rendererElem.appendChild( colorRampElem );
}
rendererElem.setAttribute( "invert_ramp", QString::number( mInvertRamp ) );
rendererElem.setAttribute( "forceraster", ( mForceRaster ? "1" : "0" ) );

This comment has been minimized.

Sign in to view

Copy link
@SebDieBln

SebDieBln Feb 18, 2016

Contributor

Just to make sure: This is unrelated to the order-by issue but nonetheless intended?

This comment has been minimized.

Sign in to view

Copy link
@nyalldawson

nyalldawson Feb 18, 2016

Author Collaborator

yep. I actually don't think it's strictly needed (since heatmaps are ALWAYS rendered as rasters), but it was present in all the other renderers and it doesn't hurt to be safe

if ( mPaintEffect && !QgsPaintEffectRegistry::isDefaultStack( mPaintEffect ) )
mPaintEffect->saveProperties( doc, rendererElem );
Expand All @@ -371,6 +372,7 @@ QDomElement QgsHeatmapRenderer::save( QDomDocument& doc )
mOrderBy.save( orderBy );
rendererElem.appendChild( orderBy );
}
rendererElem.setAttribute( "enableorderby", ( mOrderByEnabled ? "1" : "0" ) );

return rendererElem;
}
Expand Down
1 change: 1 addition & 0 deletions src/core/symbology-ng/qgsinvertedpolygonrenderer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ QDomElement QgsInvertedPolygonRenderer::save( QDomDocument& doc )
mOrderBy.save( orderBy );
rendererElem.appendChild( orderBy );
}
rendererElem.setAttribute( "enableorderby", ( mOrderByEnabled ? "1" : "0" ) );

return rendererElem;
}
Expand Down
1 change: 1 addition & 0 deletions src/core/symbology-ng/qgspointdisplacementrenderer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@ QDomElement QgsPointDisplacementRenderer::save( QDomDocument& doc )
mOrderBy.save( orderBy );
rendererElement.appendChild( orderBy );
}
rendererElement.setAttribute( "enableorderby", ( mOrderByEnabled ? "1" : "0" ) );

return rendererElement;
}
Expand Down
14 changes: 14 additions & 0 deletions src/core/symbology-ng/qgsrendererv2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ void QgsFeatureRendererV2::copyRendererData( QgsFeatureRendererV2* destRenderer

destRenderer->setPaintEffect( mPaintEffect->clone() );
destRenderer->mOrderBy = mOrderBy;
destRenderer->mOrderByEnabled = mOrderByEnabled;
}

void QgsFeatureRendererV2::copyPaintEffect( QgsFeatureRendererV2 *destRenderer ) const
Expand All @@ -212,6 +213,7 @@ QgsFeatureRendererV2::QgsFeatureRendererV2( const QString& type )
, mCurrentVertexMarkerSize( 3 )
, mPaintEffect( nullptr )
, mForceRaster( false )
, mOrderByEnabled( false )
{
mPaintEffect = QgsPaintEffectRegistry::defaultStack();
mPaintEffect->setEnabled( false );
Expand Down Expand Up @@ -334,6 +336,7 @@ QgsFeatureRendererV2* QgsFeatureRendererV2::load( QDomElement& element )
// restore order by
QDomElement orderByElem = element.firstChildElement( "orderby" );
r->mOrderBy.load( orderByElem );
r->setOrderByEnabled( element.attribute( "enableorderby", "0" ).toInt() );
}
return r;
}
Expand All @@ -353,6 +356,7 @@ QDomElement QgsFeatureRendererV2::save( QDomDocument& doc )
mOrderBy.save( orderBy );
rendererElem.appendChild( orderBy );
}
rendererElem.setAttribute( "enableorderby", ( mOrderByEnabled ? "1" : "0" ) );
return rendererElem;
}

Expand Down Expand Up @@ -617,6 +621,16 @@ void QgsFeatureRendererV2::setOrderBy( const QgsFeatureRequest::OrderBy& orderBy
mOrderBy = orderBy;
}

bool QgsFeatureRendererV2::orderByEnabled() const
{
return mOrderByEnabled;
}

void QgsFeatureRendererV2::setOrderByEnabled( bool enabled )
{
mOrderByEnabled = enabled;
}

void QgsFeatureRendererV2::convertSymbolSizeScale( QgsSymbolV2 * symbol, QgsSymbolV2::ScaleMethod method, const QString & field )
{
if ( symbol->type() == QgsSymbolV2::Marker )
Expand Down
23 changes: 23 additions & 0 deletions src/core/symbology-ng/qgsrendererv2.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,15 +375,36 @@ class CORE_EXPORT QgsFeatureRendererV2
/**
* Get the order in which features shall be processed by this renderer.
* @note added in QGIS 2.14
* @note this property has no effect if orderByEnabled() is false
* @see orderByEnabled()
*/
QgsFeatureRequest::OrderBy orderBy() const;

/**
* Define the order in which features shall be processed by this renderer.
* @note this property has no effect if orderByEnabled() is false
* @note added in QGIS 2.14
* @see setOrderByEnabled()
*/
void setOrderBy( const QgsFeatureRequest::OrderBy& orderBy );

/**
* Returns whether custom ordering will be applied before features are processed by this renderer.
* @note added in QGIS 2.14
* @see orderBy()
* @see setOrderByEnabled()
*/
bool orderByEnabled() const;

/**
* Sets whether custom ordering should be applied before features are processed by this renderer.
* @param enabled set to true to enable custom feature ordering
* @note added in QGIS 2.14
* @see setOrderBy()
* @see orderByEnabled()
*/
void setOrderByEnabled( bool enabled );

protected:
QgsFeatureRendererV2( const QString& type );

Expand Down Expand Up @@ -447,6 +468,8 @@ class CORE_EXPORT QgsFeatureRendererV2

QgsFeatureRequest::OrderBy mOrderBy;

bool mOrderByEnabled;

private:
Q_DISABLE_COPY( QgsFeatureRendererV2 )
};
Expand Down
1 change: 1 addition & 0 deletions src/core/symbology-ng/qgsrulebasedrendererv2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -993,6 +993,7 @@ QDomElement QgsRuleBasedRendererV2::save( QDomDocument& doc )
mOrderBy.save( orderBy );
rendererElem.appendChild( orderBy );
}
rendererElem.setAttribute( "enableorderby", ( mOrderByEnabled ? "1" : "0" ) );

return rendererElem;
}
Expand Down
3 changes: 3 additions & 0 deletions src/core/symbology-ng/qgssinglesymbolrendererv2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,8 @@ QDomElement QgsSingleSymbolRendererV2::save( QDomDocument& doc )
mOrderBy.save( orderBy );
rendererElem.appendChild( orderBy );
}
rendererElem.setAttribute( "enableorderby", ( mOrderByEnabled ? "1" : "0" ) );

return rendererElem;
}

Expand Down Expand Up @@ -481,6 +483,7 @@ QgsSingleSymbolRendererV2* QgsSingleSymbolRendererV2::convertFromRenderer( const
if ( r )
{
r->setOrderBy( renderer->orderBy() );
r->setOrderByEnabled( renderer->orderByEnabled() );
}

return r;
Expand Down
17 changes: 9 additions & 8 deletions src/gui/symbology-ng/qgsrendererv2propertiesdialog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,15 +136,15 @@ QgsRendererV2PropertiesDialog::QgsRendererV2PropertiesDialog( QgsVectorLayer* la
// setup slot rendererChanged()
connect( cboRenderers, SIGNAL( currentIndexChanged( int ) ), this, SLOT( rendererChanged() ) );
//setup order by
if ( mOrderBy.isEmpty() )
if ( mLayer->rendererV2()->orderByEnabled() )
{
btnOrderBy->setEnabled( false );
checkboxEnableOrderBy->setChecked( false );
lineEditOrderBy->setEnabled( false );
checkboxEnableOrderBy->setChecked( true );
}
else
{
checkboxEnableOrderBy->setChecked( true );
btnOrderBy->setEnabled( false );
checkboxEnableOrderBy->setChecked( false );
lineEditOrderBy->setEnabled( false );
}
lineEditOrderBy->setReadOnly( true );
connect( checkboxEnableOrderBy, SIGNAL( toggled( bool ) ), btnOrderBy, SLOT( setEnabled( bool ) ) );
Expand Down Expand Up @@ -222,7 +222,7 @@ void QgsRendererV2PropertiesDialog::rendererChanged()
{
if ( mMapCanvas )
mActiveWidget->setMapCanvas( mMapCanvas );
changeOrderBy( mActiveWidget->renderer()->orderBy() );
changeOrderBy( mActiveWidget->renderer()->orderBy(), mActiveWidget->renderer()->orderByEnabled() );
connect( mActiveWidget, SIGNAL( layerVariablesChanged() ), this, SIGNAL( layerVariablesChanged() ) );
}
}
Expand All @@ -248,6 +248,7 @@ void QgsRendererV2PropertiesDialog::apply()
renderer->setPaintEffect( mPaintEffect->clone() );
// set the order by
renderer->setOrderBy( mOrderBy );
renderer->setOrderByEnabled( checkboxEnableOrderBy->isChecked() );

mLayer->setRendererV2( renderer->clone() );
}
Expand Down Expand Up @@ -278,11 +279,11 @@ void QgsRendererV2PropertiesDialog::showOrderByDialog()
}
}

void QgsRendererV2PropertiesDialog::changeOrderBy( const QgsFeatureRequest::OrderBy& orderBy )
void QgsRendererV2PropertiesDialog::changeOrderBy( const QgsFeatureRequest::OrderBy& orderBy, bool orderByEnabled )
{
mOrderBy = orderBy;
lineEditOrderBy->setText( mOrderBy.dump() );
checkboxEnableOrderBy->setChecked( !orderBy.isEmpty() );
checkboxEnableOrderBy->setChecked( orderByEnabled );
}


Expand Down
2 changes: 1 addition & 1 deletion src/gui/symbology-ng/qgsrendererv2propertiesdialog.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class GUI_EXPORT QgsRendererV2PropertiesDialog : public QDialog, private Ui::Qgs
private slots:
void showOrderByDialog();

void changeOrderBy( const QgsFeatureRequest::OrderBy& orderBy );
void changeOrderBy( const QgsFeatureRequest::OrderBy& orderBy, bool orderByEnabled );

protected:

Expand Down
7 changes: 6 additions & 1 deletion tests/src/python/test_qgssinglesymbolrenderer.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,19 @@ def tearDown(self):

def testOrderBy(self):
self.renderer.setOrderBy(QgsFeatureRequest.OrderBy([QgsFeatureRequest.OrderByClause('Value', False)]))
self.renderer.setOrderByEnabled(True)

# Setup rendering check
renderchecker = QgsMultiRenderChecker()
renderchecker.setMapSettings(self.mapsettings)
renderchecker.setControlName('expected_singlesymbol_orderby')
result = renderchecker.runTest('singlesymbol_orderby')

assert result

# disable order by and retest
self.renderer.setOrderByEnabled(False)
result = renderchecker.runTest('single')


if __name__ == '__main__':
unittest.main()

2 comments on commit 4285d70

@nirvn
Copy link
Contributor

@nirvn nirvn commented on 4285d70 Feb 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nyalldawson I think this created a regression with the 2.5D symbology. Now, the order by [x] checkbox is always de-activated for 2.5D symbology, messing the 2.5D stacking of features.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 4285d70 Feb 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Nyall,

I just started to look into this issue and wondered why it already looked like there's nothing left to do 😆

Please sign in to comment.