Modify

Opened 2 months ago

Closed 3 weeks ago

#17845 closed enhancement (fixed)

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

Reported by: taylor.smock Owned by: Don-vip
Priority: normal Milestone: 19.08
Component: Core mappaint Version:
Keywords: mapcss validator relation role member Cc:

Description

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");
}

Attachments (7)

17845.patch (1.7 KB) - added by taylor.smock 2 months ago.
Add has_role function to mapcss
17845_v2.patch (1.8 KB) - added by taylor.smock 2 months ago.
Count the number of members with a role instead of just returning true/false, rename function from has_role to count_role.
17845_v3.patch (1.9 KB) - added by taylor.smock 2 months ago.
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)
17845_v4.patch (5.0 KB) - added by taylor.smock 2 months ago.
Add test for count_roles
17845_varargs.patch (3.0 KB) - added by taylor.smock 8 weeks ago.
Initial work on allowing varargs to be used in mapcss functions
17845_temporary_workaround.patch (1.1 KB) - added by taylor.smock 8 weeks ago.
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
17845_v5.patch (4.2 KB) - added by taylor.smock 6 weeks ago.
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).

Download all attachments as: .zip

Change History (24)

Changed 2 months ago by taylor.smock

Attachment: 17845.patch added

Add has_role function to mapcss

comment:1 Changed 2 months ago by 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?

comment:2 in reply to:  1 Changed 2 months ago by 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.

Changed 2 months ago by taylor.smock

Attachment: 17845_v2.patch added

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

Changed 2 months ago by taylor.smock

Attachment: 17845_v3.patch added

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)

comment:3 Changed 2 months ago by Don-vip

Keywords: relation role member added
Milestone: 19.06

comment:4 Changed 2 months ago by Don-vip

Thanks! Can you please add a unit test too?

comment:5 Changed 2 months ago by 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));
    }

Changed 2 months ago by taylor.smock

Attachment: 17845_v4.patch added

Add test for count_roles

comment:6 Changed 8 weeks ago by Don-vip

Resolution: fixed
Status: newclosed

In 15196/josm:

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

comment:7 Changed 8 weeks ago by Klumbumbus

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

comment:8 in reply to:  7 Changed 8 weeks ago by 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");
}

comment:9 Changed 8 weeks ago by 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.

comment:10 Changed 8 weeks ago by Klumbumbus

Resolution: fixed
Status: closedreopened

Ok, I see count_roles() without any role doesn't work at all. (What I was trying to check requires more a count_members() function.) However with specified role it works for me only without varargs ....

Last edited 8 weeks ago by Klumbumbus (previous) (diff)

comment:11 in reply to:  9 Changed 8 weeks ago by 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.

comment:12 Changed 8 weeks ago by 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).

Changed 8 weeks ago by taylor.smock

Attachment: 17845_varargs.patch added

Initial work on allowing varargs to be used in mapcss functions

Changed 8 weeks ago by 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

comment:13 Changed 8 weeks ago by Don-vip

Milestone: 19.0619.07

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 :)

comment:14 Changed 6 weeks ago by Don-vip

Milestone: 19.0719.08

Milestone renamed

Changed 6 weeks ago by taylor.smock

Attachment: 17845_v5.patch added

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).

comment:15 Changed 3 weeks ago by Don-vip

Owner: changed from team to Don-vip
Status: reopenednew

comment:16 Changed 3 weeks ago by Don-vip

Status: newassigned

comment:17 Changed 3 weeks ago by Don-vip

Resolution: fixed
Status: assignedclosed

In 15275/josm:

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Don-vip.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.