Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 21 additions & 12 deletions src/libkomwm.py
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,26 @@ def get_drape_priority(cl, dr_type, object_id, auto_dr_type = None, auto_comment
return 0


def get_type_tags(selectors):
"""Returns the tags that styles are evaluated against for a classificator type: the tags of the first
selector of its mapcss-mapping.csv row, e.g. '[highway=primary][bridge?]' -> {highway: primary, bridge: yes}.
The order matters, the first tag is the type's main one.
A forbidden '[!key]' is left out rather than set to "no", which MapCSS '[key]' would treat as set. So a key
that occurs only in such conditions is not a static tag, and styles can't test it (an "Unknown tag" error).
"""
tags = OrderedDict()
# Only the first selector: the others are alternative OSM spellings of the same type, e.g.
# "[natural=water][intermittent=yes],[natural=water][seasonal?]", while the style has to be evaluated
# against one canonical set of tags. So a style can't test a key that only a later selector carries:
# such a condition is false if another type makes that key a static tag, and an "Unknown tag" error if not.
# TODO: revisit - either take the tags of every selector into account, or validate styles against them.
for cond in selectors.split(',')[0].split('['):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have doubts about this line:

selectors.split(',')[0]

Why do we parse only the first selector? We have this code for a long time already. But let's put a comment to come back here later.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good question, and it is worth a comment - added one at that line.

Short answer: the remaining selectors are alternative OSM spellings of the same type, while the style has to be evaluated against a single canonical tag set, and the first selector is it. E.g. natural|water|intermittent matches [natural=water][intermittent=yes], [natural=water][seasonal?], [natural=water][basin=detention] - all the same classificator type, so its tags are those of the first one.

The consequence is a real blind spot: on the current mapcss-mapping.csv, 202 of 1378 live rows list several selectors and 179 of those carry keys the first selector lacks (37 distinct ones: seasonal, basin, leaf_type, location, ...). A style rule testing such a key is silently false for that type if another type makes it a static tag, and an "Unknown tag" error otherwise. Nothing in the current styles does that, so this is latent rather than broken.

So the comment states why only the first selector is used and leaves a TODO: either take the tags of every selector into account, or validate styles against them.

key, eq, value = cond.strip(']').partition('=')
if key and not key.startswith('!'):
tags[key.rstrip('?')] = value if eq else 'yes'
return tags


# TODO: Split large function to smaller ones
def komap_mapswithme(options):
if options.data and os.path.isdir(options.data):
Expand Down Expand Up @@ -523,19 +543,8 @@ def addPattern(dashes):
cl = row[0].replace("|", "-")
if cl in unique_types_check and row[2] != 'x':
raise Exception('Duplicate type: {0}'.format(row[0]))
pairs = [i.strip(']').split("=") for i in row[1].split(',')[0].split('[')]
kv = OrderedDict()
for i in pairs:
if len(i) == 1:
if i[0]:
if i[0][0] == "!":
kv[i[0][1:].strip('?')] = "no"
else:
kv[i[0].strip('?')] = "yes"
else:
kv[i[0]] = i[1]
if row[2] != "x":
classificator[cl] = kv
classificator[cl] = get_type_tags(row[1])
class_order.append(cl)
unique_types_check.add(cl)
# Mark original type to distinguish it among replacing types.
Expand Down
13 changes: 13 additions & 0 deletions tests/testLibkomwm.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,19 @@


class LibKomwmTest(unittest.TestCase):
def test_get_type_tags(self):
def items(selectors):
# The order matters: the first tag is the type's main one.
return list(libkomwm.get_type_tags(selectors).items())

self.assertEqual(items('[highway=primary][bridge?]'), [('highway', 'primary'), ('bridge', 'yes')])
# Only the first selector counts.
self.assertEqual(items('[amenity=parking][fee],[amenity=parking][parking=lane]'),
[('amenity', 'parking'), ('fee', 'yes')])
# A forbidden key is absent, so that MapCSS [!tunnel] matches the type.
self.assertEqual(items('[natural=water][intermittent=yes][!tunnel]'),
[('natural', 'water'), ('intermittent', 'yes')])

def test_generate_drules_mini(self):
assets_dir = Path(__file__).parent / 'assets' / 'case-2-generate-drules-mini'

Expand Down
Loading