Do not ask for surface of complicated roads (which already have surface:lanes* taged)

[mnalis]

• AddRoadSurface.kt - do not ask for surface when more detailed surface:lanes* is already present
• Surface Overlay - color ways with surface:lanes* as BLACK, same as ways with surface:note
• add tests

Supports tags surface:lanes, surface:lanes:forward, surface:lanes:backward, surface:lanes:both_lanes

Fixes #5330

[mnalis]

Compiles and seems to work fine, e.g. way 39159525 with surface:lanes=asphalt|asphalt|cobblestone turns black now:

[westnordost]

From #5330 (comment)

change surface quests so that when specifying paved or another general value on a way that has surface:lanes (etc.) a surface:note is not be required when selecting such a general value. Conversely, a general value plus surface:lanes (etc.) but without surface:note shall not trigger the creation of the quest

What about the first part?

[mnalis]

What about the first part?

Um, perhaps I'm not getting what you meant there completely, but surface quest does not show at all when the way has surface:lanes tagged ¹, so I don't even see how one would technically be able to "specify paved or another general value on a way that has surface:lanes" (or any other subsequent questions of that skipped quest, like the one for writing surface:note)

Or to rephrase: surface:notes is never asked for paved/unpaved way which has surface:lanes in surface quest (as you requested in the first part), for the simple reason that the whole surface quest is skipped altogether for ways which have surface:lanes tagged.

Or have I misunderstood what you meant there? 😕

Footnotes

1. the whole idea being that since the way already has surface:lanes (and its ilk) tagged, it is already described in much better detail than SC quest would be able to do, so there is no need for SC to bother the user with surface quest about that way. ↩

[westnordost]

Why should it not show at all? The filter is (extract):

!surface
or (
  surface ~ paved|unpaved|${INVALID_SURFACES.joinToString("|")}
  and !surface:lanes
)

So, it still shows when surface has not been tagged.

[mnalis]

Why should it not show at all? The filter is (extract):

Ops, you're correct, those should've been under and and not or -- I've fixed that now so it skips quest always when surface:lanes is present.

Why tests didn't catch it

Also, while scratching my head why my test of:

@Test fun `not applicable to tagged surface:lanes`() {
        assertIsNotApplicable("highway" to "residential", "surface:lanes" to "concrete|asphalt|asphalt")

did not catch that error (i.e. matched !surface or (...whatever...)), I've found that tests are not run by build-debug-apk.yml GitHub workflow, but by test.yml instead. 🤦‍♂️

I've also had to increase memory for tests to be run, details in PR #5457

I've provided new build at https://github.com/mnalis/StreetComplete/actions/runs/7651486933 and test on https://github.com/mnalis/StreetComplete/actions/runs/7651409738.

[westnordost]

There continues to be a misunderstanding. I wrote

The following PR would be accepted:

(In a nutshell: surface:lanes (etc.) is treated as if a surface:note was present. In detail:)

• change surface quests so that when specifying paved or another general value on a way that has surface:lanes (etc.) a surface:note is not be required when selecting such a general value. Conversely, a general value plus surface:lanes (etc.) but without surface:note shall not trigger the creation of the quest
• + Tests
• change surface overlay in the same way. For coloring: Red if surface is missing, the same color as when both surface and surface:note are present if surface and surface:lanes (etc.) are present.

#5330 (comment)

In other words, DO ask about missing surface always. But when any surface:lanes is specified, do not require a note to be specified when the user answers with a generic surface.

[mnalis]

In other words, DO ask about missing surface always

OK, I can do that (ask surface quest regardless of any surface:lanes), but in that quote above you said:

Conversely, a general value plus surface:lanes (etc.) but without surface:note shall not trigger the creation of the quest

Which seems to me to say that quest should be skipped if surface ~ paved|unpaved and surface:lanes and !surface:note ?
So that seems to me to be conflicting with that new requirement above to always ask the quest (which I interpret to mean "regardless of existence or not of surface:lanes"?) :puzzled:

But when any surface:lanes is specified, do not require a note to be specified when the user answers with a generic surface.

I can try to do that if you wish it so (although it seems to be problematic if the way has already tagged e.g. surface:lanes=asphalt|gravel - what should do user answer as surface? Any answer would be problematic there, including general ones like paved or unpaved, IMHO. I guess they should leave a note then?), but I'm hitting limit of my (quite tiny) Kotlin knowledge here so would ask for some guidance of more knowledgeable Kotlin programmers.

I think I found it should be checked here:

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/quests/surface/SurfaceDescriptionUtils.kt

Lines 9 to 14 in f8976a9

	fun collectSurfaceDescriptionIfNecessary(
	context: Context,
	surface: Surface,
	onDescribed: (description: String?) -> Unit
	) {
	if (!surface.shouldBeDescribed) {

But I am at loss how I can access all the other tags (like surface:lanes, surface:lanes:forward etc). from that function?

[westnordost]

Conversely, a general value plus surface:lanes (etc.) but without surface:note shall not trigger the creation of the quest

Which seems to me to say that quest should be skipped if surface ~ paved|unpaved and surface:lanes and !surface:note ?

Did you do a typo there? That expression doesn't make sense to me.

It should be skipped if surface ~ paved|unpaved and (surface:lanes or surface:note) or in other words, should only be asked if !surface or (surface ~ paved|unpaved and !surface:lanes and !surface:note).

Or, yet in other words, the presence of surface:lanes should be treated similarly as the presence of surface:note. I believe the state of the PR before my first review was fine in that regard.

I can try to do that if you wish it so (although it seems to be problematic if the way has already tagged e.g. surface:lanes=asphalt|gravel - what should do user answer as surface? Any answer would be problematic there, including general ones like paved or unpaved, IMHO.

Yes, in that case they'd have to leave a note. This is a situation that is extremely rare enough that it is fine to leave a note in that case.

I'm hitting limit of my (quite tiny) Kotlin knowledge here so would ask for some guidance of more knowledgeable Kotlin programmers.

Well, your code before my first review was a good start, the only thing that was completely missing was the one thing I noted. It looks to me you should just first revert the commits "never asks surface quest when surface:lanes* is present" and "really always skip surface:lanes*" and continue from there.

[westnordost]

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/quests/surface/AddRoadSurfaceForm.kt

Line 20 in f8976a9

collectSurfaceDescriptionIfNecessary(requireContext(), surface) { description ->

^- here you can access the element tags via element.tags and hence check for any surface lanes before you call "collectSurfaceDescriptionIfNecessary".

Not sure where the code is for the surface overlay, I guess the check needs to be added there too. so best put the hasSurfaceLanes(tags): Boolean function into the SurfaceUtils.kt

[westnordost]

I see you made some changes. Is it deliberate that it is still marked as draft?

[mnalis]

On Sun, Feb 11, 2024 at 02:42:15PM -0800, Tobias Zwick wrote: I see you made some changes. Is it deliberate that it is still marked as draft?

Yes, still needs changes.

[mnalis]

Not sure where the code is for the surface overlay, I guess the check needs to be added there too. so best put the hasSurfaceLanes(tags): Boolean function into the SurfaceUtils.kt

Well I think I found where overlay asks for surface note, it seems to be here

However, trying to access element.tags (as worked for Quest) fails with Unresolved reference: element and I have absolutely no idea how to have it access all tags for current element from there. Help @westnordost (or anyone else with knowledge of overlays/kotlin who might know)?

[westnordost]

I've seen your barrage of commits yesterday (I get a notification email for each of them) and suspected that you were struggling.

Am I right to assume that you are creating a PR without actually having Android Studio or a comparable IDE installed? Please, don't. You are not only massively wasting your own time by incapacitating yourself through absent tooling, you are wasting time of people who then help you.

It is no secret that for many PRs of contributors reviewed, I spent more time reviewing, explaining and helping that I would have just implementing it myself. And that's okay, but the expectation is that contributors sharpen their tools and learn from that.

It looks to me as if yesterday you massively wasted your time playing ping pong with the Github build job, in that time you could have installed and learned basic use of Android Studio ten times. In any proper IDE, you can easily navigate through the code, between the classes, inheritors, superclasses, what is called by whom, what methods are available in this, has autocompletion, formats the code automatically, immediately underlines syntax errors etc.

So, look, you are one of the top contributors by number of commits. The suggestion that you apparently contributed these with no tooling shocks me because it means you have been massively wasting your time (all along). What I am suggesting is, that you should rather take a few steps back and learn how to help yourself - or rather, learn how the IDE can help you.

Regarding your particular issue, it looks like you should pass the information whether the element has surface:lanes in the constructor of SurfaceAndNoteViewController as e.g. underspecifiedSurfacesShouldBeDescribed: Boolean. This is no issue related to kotlin or overlays, though.