Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to delete unused categories in the categorized symbol renderer widget #60641

Merged
merged 7 commits into from
Feb 23, 2025
Merged

Add an option to delete unused categories in the categorized symbol renderer widget #60641

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3b9a5f8
Add an option via a button in the categorized symbol renderer widget …
benwirf Feb 17, 2025
e535205
Rebase and incorporate suggestions from review
benwirf Feb 19, 2025
1d51900
sipify
benwirf Feb 19, 2025
862ebe1
Trying to fix ui file
benwirf Feb 19, 2025
cc1e3d4
install pre-commit hooks and fix ui file
benwirf Feb 19, 2025
dc70921
sipify
benwirf Feb 23, 2025
c159e7c
apply suggestions from review
benwirf Feb 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions python/PyQt6/gui/auto_generated/symbology/qgscategorizedsymbolrendererwidget.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ Applies the color ramp passed on by the color ramp button
void deleteCategories();
void deleteAllCategories();

void deleteUnusedCategories();
%Docstring
Deletes unused categories from the widget which are not used by the layer renderer.
%End

void showSymbolLevels();

void rowsMoved();
Expand Down Expand Up @@ -123,6 +128,13 @@ Changes the selected symbols alone for the change button, if there is a selectio
void applyChangeToSymbol();
%Docstring
Applies current symbol to selected categories, or to all categories if none is selected
%End

QList<QVariant> layerUniqueValues( const QString &attrName );
%Docstring
Returns the list of unique values in the current widget's layer for attribute name ``attrName``.

Called by :py:func:`~QgsCategorizedSymbolRendererWidget.addCategories` and :py:func:`~QgsCategorizedSymbolRendererWidget.deleteUnusedCategories`
%End

virtual QList<QgsSymbol *> selectedSymbols();
Expand Down
12 changes: 12 additions & 0 deletions python/gui/auto_generated/symbology/qgscategorizedsymbolrendererwidget.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ Applies the color ramp passed on by the color ramp button
void deleteCategories();
void deleteAllCategories();

void deleteUnusedCategories();
%Docstring
Deletes unused categories from the widget which are not used by the layer renderer.
%End

void showSymbolLevels();

void rowsMoved();
Expand Down Expand Up @@ -123,6 +128,13 @@ Changes the selected symbols alone for the change button, if there is a selectio
void applyChangeToSymbol();
%Docstring
Applies current symbol to selected categories, or to all categories if none is selected
%End

QList<QVariant> layerUniqueValues( const QString &attrName );
%Docstring
Returns the list of unique values in the current widget's layer for attribute name ``attrName``.

Called by :py:func:`~QgsCategorizedSymbolRendererWidget.addCategories` and :py:func:`~QgsCategorizedSymbolRendererWidget.deleteUnusedCategories`
%End

virtual QList<QgsSymbol *> selectedSymbols();
Expand Down
86 changes: 58 additions & 28 deletions src/gui/symbology/qgscategorizedsymbolrendererwidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,7 @@ QgsCategorizedSymbolRendererWidget::QgsCategorizedSymbolRendererWidget( QgsVecto
connect( btnAddCategories, &QAbstractButton::clicked, this, &QgsCategorizedSymbolRendererWidget::addCategories );
connect( btnDeleteCategories, &QAbstractButton::clicked, this, &QgsCategorizedSymbolRendererWidget::deleteCategories );
connect( btnDeleteAllCategories, &QAbstractButton::clicked, this, &QgsCategorizedSymbolRendererWidget::deleteAllCategories );
connect( btnDeleteUnusedCategories, &QAbstractButton::clicked, this, &QgsCategorizedSymbolRendererWidget::deleteUnusedCategories );
connect( btnAddCategory, &QAbstractButton::clicked, this, &QgsCategorizedSymbolRendererWidget::addCategory );

connect( btnColorRamp, &QgsColorRampButton::colorRampChanged, this, &QgsCategorizedSymbolRendererWidget::applyColorRamp );
Expand Down Expand Up @@ -900,34 +901,7 @@ void QgsCategorizedSymbolRendererWidget::changeCategorySymbol()
void QgsCategorizedSymbolRendererWidget::addCategories()
{
const QString attrName = mExpressionWidget->currentField();
const int idx = mLayer->fields().lookupField( attrName );
QList<QVariant> uniqueValues;
if ( idx == -1 )
{
// Lets assume it's an expression
QgsExpression *expression = new QgsExpression( attrName );
QgsExpressionContext context;
context << QgsExpressionContextUtils::globalScope()
<< QgsExpressionContextUtils::projectScope( QgsProject::instance() )
<< QgsExpressionContextUtils::atlasScope( nullptr )
<< QgsExpressionContextUtils::layerScope( mLayer );

expression->prepare( &context );
QgsFeatureIterator fit = mLayer->getFeatures();
QgsFeature feature;
while ( fit.nextFeature( feature ) )
{
context.setFeature( feature );
const QVariant value = expression->evaluate( &context );
if ( uniqueValues.contains( value ) )
continue;
uniqueValues << value;
}
}
else
{
uniqueValues = qgis::setToList( mLayer->uniqueValues( idx ) );
}
const QList<QVariant> uniqueValues = layerUniqueValues( attrName );

// ask to abort if too many classes
if ( uniqueValues.size() >= 1000 )
Expand Down Expand Up @@ -1093,6 +1067,62 @@ void QgsCategorizedSymbolRendererWidget::deleteAllCategories()
emit widgetChanged();
}

void QgsCategorizedSymbolRendererWidget::deleteUnusedCategories()
{
if ( !mRenderer )
return;
const QString attrName = mExpressionWidget->currentField();
const QList<QVariant> uniqueValues = layerUniqueValues( attrName );

const QgsCategoryList catList = mRenderer->categories();

QList<int> unusedIndexes;

for ( int i = 0; i < catList.size(); ++i )
{
const QgsRendererCategory cat = catList.at( i );
if ( !uniqueValues.contains( cat.value() ) )
{
unusedIndexes.append( i );
}
}
mModel->deleteRows( unusedIndexes );
emit widgetChanged();
}

QList<QVariant> QgsCategorizedSymbolRendererWidget::layerUniqueValues( const QString &attrName )
{
const int idx = mLayer->fields().lookupField( attrName );
QList<QVariant> uniqueValues;
if ( idx == -1 )
{
// Lets assume it's an expression
QgsExpression *expression = new QgsExpression( attrName );
Copy link
Contributor

Choose a reason for hiding this comment

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

You have à memory leak here, please use std::unique_ptr here or remove the ptr .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have no problem with these suggestions and I'm happy to make these changes, but please note that this is part of the block of code which I reused from the existing addCategories() method. I.e. this code is in the current master branch:

QGIS/src/gui/symbology/qgscategorizedsymbolrendererwidget.cpp

Line 908 in 8789841

QgsExpression *expression = new QgsExpression( attrName );

Again, I'm happy to change it as part of this PR- is there anything else that you think should be modified here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, I would have spotted during my first review , but thank you for making the modification, one less memory leak.

Lgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely! Thanks again for your help @troopa81.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @troopa81, even though this PR is closed I just wanted to leave this here as a follow up. Firstly, I appreciate the time you gave me here- I should have picked these things up myself! As you can probably tell I'm fairly inexperienced in C++, but your reviews here prompted me to think/learn a bit more about memory management, heap/stack allocation etc. and to be a bit more analytical/critical when modifying existing QGIS code which will help me to submit better PRs in the future. So I just wanted to say thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@benwirf You're welcome! I'm always happy to see new contributors. Keep up the good work

QgsExpressionContext context;
context << QgsExpressionContextUtils::globalScope()
<< QgsExpressionContextUtils::projectScope( QgsProject::instance() )
<< QgsExpressionContextUtils::atlasScope( nullptr )
<< QgsExpressionContextUtils::layerScope( mLayer );

expression->prepare( &context );
QgsFeatureIterator fit = mLayer->getFeatures();
QgsFeature feature;
while ( fit.nextFeature( feature ) )
{
context.setFeature( feature );
const QVariant value = expression->evaluate( &context );
if ( uniqueValues.contains( value ) )
continue;
uniqueValues << value;
}
}
else
{
uniqueValues = qgis::setToList( mLayer->uniqueValues( idx ) );
}
return uniqueValues;
}

void QgsCategorizedSymbolRendererWidget::addCategory()
{
if ( !mModel )
Expand Down
12 changes: 12 additions & 0 deletions src/gui/symbology/qgscategorizedsymbolrendererwidget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ class GUI_EXPORT QgsCategorizedSymbolRendererWidget : public QgsRendererWidget,
void deleteCategories();
void deleteAllCategories();

/**
* Deletes unused categories from the widget which are not used by the layer renderer.
*/
void deleteUnusedCategories();

void showSymbolLevels();

void rowsMoved();
Expand Down Expand Up @@ -235,6 +240,13 @@ class GUI_EXPORT QgsCategorizedSymbolRendererWidget : public QgsRendererWidget,
//! Applies current symbol to selected categories, or to all categories if none is selected
void applyChangeToSymbol();

/**
* Returns the list of unique values in the current widget's layer for attribute name \a attrName.
*
* Called by addCategories() and deleteUnusedCategories()
*/
QList<QVariant> layerUniqueValues( const QString &attrName );

QList<QgsSymbol *> selectedSymbols() override;
QgsCategoryList selectedCategoryList();
void refreshSymbolView() override;
Expand Down
7 changes: 7 additions & 0 deletions src/ui/qgscategorizedsymbolrendererwidget.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,13 @@
</property>
</widget>
</item>
<item>
<widget class="QPushButton" name="btnDeleteUnusedCategories">
<property name="text">
<string>Delete Unused</string>
</property>
</widget>
</item>
<item>
<spacer>
<property name="orientation">
Expand Down