[PATCH] There should be a method to check for roles in relations

[taylor.smock]

Since the child/parent selectors cannot be chained, and properties cannot be used on the left side of the child selector, it would be good to have a function to determine if a relation has a specific role.

Example:

relation[type=destination_sign][!has_role("to")] {
  throwError: tr("{0} has no {1} role", "destination_sign", "to");
}

What I had tried:

/* properties are not supported on the left side of a selector */
relation[type=destination_sign] >[role=to] * { set .to_role }
*.to_role  < relation[type=destination_sign] {
  throwError: tr("{0} has no {1} role", "destination_sign", "to");
}

/* Apparently chaining child/parent selectors isn't a thing */
relation[type=destination_sign] >[role=to] * < relation[type=destination_sign] {
  throwError: tr("{0} has no {1} role", "destination_sign", "to");
}

[taylor.smock]

Add has_role function to mapcss

[taylor.smock]

Would it be better to return the number of roles that the relation has? E.g., if it has 20 "to" roles, should it just return true, or should it return 20?

[Klumbumbus]

Replying to taylor.smock:

Would it be better to return the number of roles that the relation has?

That would be nice and more universal usable.

[taylor.smock]

Count the number of members with a role instead of just returning true/false, rename function from has_role to count_role.

[taylor.smock]

Same as v2, except with the ability to county multiple roles at once (count_role->count_roles) (probably unnecessary, count_role("to") + count_role("via") should be functionally equivalent)

[Don-vip]

Thanks! Can you please add a unit test too?

[taylor.smock]

For ExpressionFactory, how are the tests usually done? I'm not seeing anything in ExpressionFactoryTest except it checks for "well defined" functions. For now, I'll assume it is in MapCSSParserTest. That being said, I need to figure out why the function isn't even being called when the test is run. I figured I'd be able to copy the testParentTags function, but that doesn't appear to be the case. I've instead used ExpressionFactory.Function.count_roles(...).

    @Test
    public void testCountRoles() throws Exception {
        DataSet ds = new DataSet();
        Way way1 = TestUtils.newWay("highway=residential name=1",
                new Node(new LatLon(0, 0)), new Node((new LatLon(0.001, 0.001))));
        for (Node node : way1.getNodes()) {
            ds.addPrimitive(node);
        }
        ds.addPrimitive(way1);

        Relation rel1 = TestUtils.newRelation("type=destination_sign", new RelationMember("", way1));
        ds.addPrimitive(rel1);

        /* Check with empty role and one object */
        MapCSSStyleSource source = new MapCSSStyleSource("relation[type=destination_sign] {text: count_roles(\"\");}");
        source.loadStyleSource();
        assertEquals(1, source.rules.size());
        Environment e = new Environment(rel1, new MultiCascade(), Environment.DEFAULT_LAYER, null);
        assertTrue(source.rules.get(0).selector.matches(e));
        source.rules.get(0).declaration.execute(e);
        assertEquals((Integer) 1, e.getCascade(Environment.DEFAULT_LAYER).get("text", -1, Integer.class));

        /* Check with non-empty role and one object */
        source = new MapCSSStyleSource("relation[type=destination_sign] {text: count_roles(\"from\");}");
        source.loadStyleSource();
        assertEquals(0, source.rules.size());
        e = new Environment(rel1, new MultiCascade(), Environment.DEFAULT_LAYER, null);
        assertTrue(source.rules.get(0).selector.matches(e));
        source.rules.get(0).declaration.execute(e);
        assertEquals("0", e.getCascade(Environment.DEFAULT_LAYER).get("text", null, String.class));

        /* Check with empty role and two objects */
        Way way2 = TestUtils.newWay("highway=residential name=2", way1.firstNode(), way1.lastNode());
        ds.addPrimitive(way2);
        rel1.addMember(new RelationMember("", way2));
        source = new MapCSSStyleSource("relation[type=destination_sign] {text: count_roles(\"\");}");
        source.loadStyleSource();
        assertEquals(0, source.rules.size());
        e = new Environment(rel1, new MultiCascade(), Environment.DEFAULT_LAYER, null);
        assertTrue(source.rules.get(0).selector.matches(e));
        source.rules.get(0).declaration.execute(e);
        assertEquals("2", e.getCascade(Environment.DEFAULT_LAYER).get("text", null, String.class));

        /* Check with non-empty role and two objects */
        rel1.setMember(0, new RelationMember("from", way1));
        source = new MapCSSStyleSource("relation[type=destination_sign] {text: count_roles(\"from\");}");
        source.loadStyleSource();
        assertEquals(0, source.rules.size());
        e = new Environment(rel1, new MultiCascade(), Environment.DEFAULT_LAYER, null);
        assertTrue(source.rules.get(0).selector.matches(e));
        source.rules.get(0).declaration.execute(e);
        assertEquals("1", e.getCascade(Environment.DEFAULT_LAYER).get("text", null, String.class));
    }

[taylor.smock]

Add test for count_roles

[Don-vip]

In 15196/josm:

fix #17845 - Add a MapCSS method to check for roles in relations (patch by taylor.smock)

[Klumbumbus]

I can not get that to work. Taylor could you please add the usage docu at wiki:/Help/Styles/MapCSSImplementation#Evalexpressions?

[taylor.smock]

Replying to Klumbumbus:

I can not get that to work. Taylor could you please add the usage docu at wiki:/Help/Styles/MapCSSImplementation#Evalexpressions?

It looks like I didn't update the javadoc when I changed it from a return boolean function to a return int function. You should be able to use it somewhat like this:

relation[type=destination_sign][count_roles("to") == 0] {
  throwError: tr("{0} must have a {1} role", "destination_sign", "to");
}

I just did some checking as well, and I don't know if varargs works in functions that take an Environment variable. The only other instances of varargs are where there isn't an Environment variable. This may be why I wasn't able to (effectively) copy a test (see comment 5).

I removed the varargs bit and it works properly (public static int count_roles(final Environment env, String... roles) -> public static int count_roles(final Environment env, String roles)). I probably should have changed it to a WIP Patch when I switched from boolean to int as a return value, in order to give me time to test in a "real" environment -- in my unit test, I'm directly calling the function, which isn't ideal.

Can you post what you were trying, so I know what I need to emphasize?

---

The varargs was so you could do something like this:

relation[type=destination_sign][count_roles("intersection", "from") == 0] {
   throwError: tr("{0} should have either {1} or {2} or both", "destination_sign", "intersection", "from");
}

[Klumbumbus]

I was trying different syntaxes and combinations of them but none worked:

relation[count_roles() = 1] {
  throwWarning: tr("Relation with only one member or unset roles.");
}

relation[eval(count_roles()) = 1] {
  throwWarning: tr("Relation with only one member or unset roles.");
}

relation[eval(count_roles()) == 1] {
  throwWarning: tr("Relation with only one member or unset roles.");
}

relation[eval(count_roles("outer")) == 1] {
  throwWarning: tr("Relation with only one member or unset roles.");
}

Here we already use some similar rules with number_of_tags().

Your example

relation[type=destination_sign][count_roles("to") == 0] {
  throwError: tr("{0} must have a {1} role", "destination_sign", "to");
}

doesn't work for me as well.

[Klumbumbus]

Ok, I see count_roles() without any role doesn't work at all. However with specified role it should work.

[taylor.smock]

Sorry, I should have been clearer. I made a local modification to test what the issue was (it is the varargs that I am using). The example I gave in comment 10 is why I was using varargs (the ... in the function definition).

I should have changed the summary of the patch to "[WIP Patch] There should be a method to check for roles in relations" around comment 2.

When I had to rewrite the tests around comment 5, I should have done some more investigation into why I wasn't able to follow the template that another test was using.

At this point, it looks like I'm going to have to investigate how the varargs are being parsed by tr and the math functions (none of which have another variable).

As a short-term fix, I could change the function definition to be:

public static int count_roles(final Environment env, String... roles)

from

public static int count_roles(final Environment env, String roles)

This will allow for the simple use cases you are looking for. I was trying to get it so that instead of doing:

relation[count_roles("outer") + count_roles("inner") == 1] {
  throwWarning: tr("There is only one member in the relation with outer/inner");
}

You could instead do:

relation[count_roles("outer", "inner") == 1] {
  throwWarning: tr("There is only one member in the relation with outer/inner");
}

The code, tested by itself, works. Its when its used in a mapcss function that it fails. Like I said, I need to investigate how the varargs are handled by the mapcss parser.

[taylor.smock]

I've tracked down where it fails.
ExpressionFactory.java#1367​ needs to be modified for varargs, and I don't think we want to change it this close to a "stable" version.

As a short-term workaround, I can file a patch that removes the varargs section (temporarily). It will cause tests to fail.

I'll also post a patch that allows varargs in mapcss (if there is an environment variable).

[taylor.smock]

Initial work on allowing varargs to be used in mapcss functions

[taylor.smock]

Temporary workaround -- probably should have a warning that it should not be publically used until the next stable release due to a changing interface, and possibly increment the @since as well when reverted back to the original function definition

[Don-vip]

OK, let's fix this in next release. We simply need to not speak about this feature in the current release, as we have not broken anything :)

[Don-vip]

Milestone renamed

[taylor.smock]

Change javadoc to match function action (count_roles), add test for varargs in mapcss, and refactor evaluate code in in ExpressionFactory to allow for varargs in functions (also, deduplicate some code).

[Don-vip]

In 15275/josm:

fix #17845 - refactor evaluate code in ExpressionFactory to allow for varargs in functions (patch by taylor.smock)