Skip to content

Commit

Permalink
Correctly quote field name in categorized filters (fix #14118)
Browse files Browse the repository at this point in the history
  • Loading branch information
nyalldawson committed Jan 19, 2016
1 parent 17528d6 commit 8435fee
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 14 deletions.
4 changes: 2 additions & 2 deletions src/core/symbology-ng/qgscategorizedsymbolrendererv2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -589,11 +589,11 @@ QString QgsCategorizedSymbolRendererV2::filter( const QgsFields& fields )
}
else if ( defaultActive )
{
return QString( "(%1) NOT IN (%2)" ).arg( mAttrName, inactiveValues );
return QString( "(\"%1\") NOT IN (%2)" ).arg( mAttrName, inactiveValues );

This comment has been minimized.

Copy link
@nirvn

nirvn Jan 19, 2016

Contributor

@nyalldawson , you also need to insure that features with null values show up. The line should be:

return QString( "(\"%1\") NOT IN (%2) OR (\"%1\") IS NULL" ).arg( mAttrName, inactiveValues );

I've tried it locally, it works.

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Jan 19, 2016

Author Collaborator

Can you give some more background about when this will come up? I'll add a unit test to cover it

This comment has been minimized.

Copy link
@nirvn

nirvn Jan 19, 2016

Contributor

@nyalldawson , if you have a field with the following values: [ "value_a", "value_b", "", null ], the categorized symbology creates three nodes: empty, value_a, value_b

The empty node should group both the "" value as well as the null value (which it did in the past).

}
else
{
return QString( "(%1) IN (%2)" ).arg( mAttrName, activeValues );
return QString( "(\"%1\") IN (%2)" ).arg( mAttrName, activeValues );
}
}

Expand Down
24 changes: 12 additions & 12 deletions tests/src/python/test_qgscategorizedsymbolrendererv2.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,21 +62,21 @@ def testFilter(self):
self.assertEqual(renderer.filter(), '')
# remove categories, leaving default
assert renderer.updateCategoryRenderState(0, False)
self.assertEqual(renderer.filter(), "(field) NOT IN ('a')")
self.assertEqual(renderer.filter(), "(\"field\") NOT IN ('a')")
assert renderer.updateCategoryRenderState(1, False)
self.assertEqual(renderer.filter(), "(field) NOT IN ('a','b')")
self.assertEqual(renderer.filter(), "(\"field\") NOT IN ('a','b')")
assert renderer.updateCategoryRenderState(2, False)
self.assertEqual(renderer.filter(), "(field) NOT IN ('a','b','c')")
self.assertEqual(renderer.filter(), "(\"field\") NOT IN ('a','b','c')")
# remove default category
assert renderer.updateCategoryRenderState(3, False)
self.assertEqual(renderer.filter(), "FALSE")
# add back other categories, leaving default disabled
assert renderer.updateCategoryRenderState(0, True)
self.assertEqual(renderer.filter(), "(field) IN ('a')")
self.assertEqual(renderer.filter(), "(\"field\") IN ('a')")
assert renderer.updateCategoryRenderState(1, True)
self.assertEqual(renderer.filter(), "(field) IN ('a','b')")
self.assertEqual(renderer.filter(), "(\"field\") IN ('a','b')")
assert renderer.updateCategoryRenderState(2, True)
self.assertEqual(renderer.filter(), "(field) IN ('a','b','c')")
self.assertEqual(renderer.filter(), "(\"field\") IN ('a','b','c')")

renderer.deleteAllCategories()
# just default category
Expand All @@ -90,11 +90,11 @@ def testFilter(self):
renderer.addCategory(QgsRendererCategoryV2('a', createMarkerSymbol(), 'a'))
renderer.addCategory(QgsRendererCategoryV2('b', createMarkerSymbol(), 'b'))
renderer.addCategory(QgsRendererCategoryV2('c', createMarkerSymbol(), 'c'))
self.assertEqual(renderer.filter(), "(field) IN ('a','b','c')")
self.assertEqual(renderer.filter(), "(\"field\") IN ('a','b','c')")
assert renderer.updateCategoryRenderState(0, False)
self.assertEqual(renderer.filter(), "(field) IN ('b','c')")
self.assertEqual(renderer.filter(), "(\"field\") IN ('b','c')")
assert renderer.updateCategoryRenderState(2, False)
self.assertEqual(renderer.filter(), "(field) IN ('b')")
self.assertEqual(renderer.filter(), "(\"field\") IN ('b')")
assert renderer.updateCategoryRenderState(1, False)
self.assertEqual(renderer.filter(), "FALSE")

Expand All @@ -103,11 +103,11 @@ def testFilter(self):
renderer.addCategory(QgsRendererCategoryV2(1, createMarkerSymbol(), 'a'))
renderer.addCategory(QgsRendererCategoryV2(2, createMarkerSymbol(), 'b'))
renderer.addCategory(QgsRendererCategoryV2(3, createMarkerSymbol(), 'c'))
self.assertEqual(renderer.filter(), '(field) IN (1,2,3)')
self.assertEqual(renderer.filter(), '(\"field\") IN (1,2,3)')
assert renderer.updateCategoryRenderState(0, False)
self.assertEqual(renderer.filter(), "(field) IN (2,3)")
self.assertEqual(renderer.filter(), "(\"field\") IN (2,3)")
assert renderer.updateCategoryRenderState(2, False)
self.assertEqual(renderer.filter(), "(field) IN (2)")
self.assertEqual(renderer.filter(), "(\"field\") IN (2)")
assert renderer.updateCategoryRenderState(1, False)
self.assertEqual(renderer.filter(), "FALSE")

Expand Down

1 comment on commit 8435fee

@SebDieBln
Copy link
Contributor

Choose a reason for hiding this comment

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

@nyalldawson @nirvn
Does this also fix Redmine #13972 Categorize symbols uses wrong field after table join?

Please sign in to comment.