Helium314 / SCEE

OpenStreetMap surveyor app for experienced OSM contributors
GNU General Public License v3.0
141 stars 10 forks source link

Add quests for `capacity=`, `capacity:disabled=` and `orientation=` for `amenity=parking` #497

Closed wielandb closed 6 months ago

wielandb commented 10 months ago

This PR adds 3 quests relating to parking mapped as amenity=parking:

photo_2023-11-21_23-50-13 photo_2023-11-21_23-51-04
photo_2023-11-21_23-53-17 photo_2023-11-21_23-54-11
photo_2023-11-21_23-54-56 photo_2023-11-21_23-55-15
wielandb commented 10 months ago

There is still one thing I need to address: I don't know how I access the tags of the object in question when creating the form, so that I can skip adding the "Other Answer" for the AddDisabledCapacity quest. Can someone point me in the right direction or show me where in the code something like that is already done?

Helium314 commented 10 months ago

I don't know how I access the tags of the object in question when creating the form

You can access the element and thus element.tags after the form is created (loaded in AbstractOsmQuestForm.onCreate).

You seem to copy a lot of pretty huge vector graphics here. I would much prefer creating the orientation drawables like the parking overlay does it.

westnordost commented 9 months ago

Was the third quest ever rejected upstream? On first glance, it looks potentially useful to me, but I don't have an overview over discussions / decisions already taken on that matter.

Helium314 commented 9 months ago

Yes, it was rejected: https://github.com/streetcomplete/StreetComplete/issues/4163#issuecomment-1167270120

westnordost commented 9 months ago

Ah, I see, makes sense. Added one more comment in that ticket. Thanks for looking it up!

wielandb commented 9 months ago

You seem to copy a lot of pretty huge vector graphics here. I would much prefer creating the orientation drawables like the parking overlay does it.

I have tried to create the images for the parking orientation quest dynamically to no avail now for some time. I simply cannot make dynamically created drawables play nicely in a Quest where a selection is to be made, and there are no quests where drawables are dynamically created in a simple "select one" context. I have now remade the form to be a simple text radio select, which I hope will suffice. If deemed insufficient, I can also remove this quest for now.

westnordost commented 9 months ago

Curious: What problem did you encounter? To the Android system, it shouldn't matter if one drawable is a vector drawable, a raster drawable or some completely custom drawable that draws itself dynamically. But who knows, maybe there is a bug in the recycler view, or the recycler view never triggers a redraw on a drawable or something

Helium314 commented 9 months ago

How do the capacity quest work exactly? Should the normal capacity include disabled parking, because the question is "how many cars"? Should the disabled capacity include normal parking? I'd say no, but I didn't check OSM wiki and wonder because you cannot answer 0.

I have tried to create the images for the parking orientation quest dynamically to no avail now for some time

You can create the drawable using private fun ParkingOrientation.image(context: Context): DrawableImage = DrawableImage(StreetParkingDrawable(context, this, null, false, 128, 128, R.drawable.ic_car1)) (set the size or parking position to what you want). Then switch to using Item2: fun ParkingOrientation.asItem(context: Context) = Item2(this, image(context), ResText(titleResId)) and pass the context in the form. Further you need to remove the duplication of ParkingOrientation and use the existing class instead.

This works for me, but the image disappears if it is selected.

mcliquid commented 9 months ago

Should the normal capacity include disabled parking, because the question is "how many cars"?

capacity expresses the total capacity of the parking lot, including disabled, charging, women, parent, and so on. Here is an example: https://wiki.openstreetmap.org/wiki/File:Tagschemaparking_vialibera.jpg or here: https://wiki.openstreetmap.org/wiki/File:Street_Side_Parking_with_Parking_Spaces_for_Disabled.png

I think the quest for the general capacity should explain this as a hint that it is about the total amount of all parking spaces.

Should the disabled capacity include normal parking? I'd say no, but I didn't check OSM wiki and wonder because you cannot answer 0.

capacity:disabled only indicates the number of disabled parking spaces within a parking lot. According to the wiki, capacity:disabled=no should be set if there are none, but capacity:disabled=0 has twice as many usages.

wielandb commented 6 months ago

I would like to bring this PR to completion, as I still have a lot of ideas for PRs for SCEE that I would like to start working on. :)

This works for me, but the image disappears if it is selected.

If I understand correctly, you also struggled getting this to work, so I would conclude that it is not (in whole) due to my lack of experience with Android development, but something that may be out of scope for now.

So I see three options on how to continue regarding the AddParkingOrientation quest:

How do you want to proceed @Helium314?

wielandb commented 6 months ago

I have a added hint about the capacity to "quest_parking_capacity_hint" as @mcliquid suggested.

I have also made it possible to answer 0 for the disabled capacity quest. (I chose to tag no insted of the more popular 0)

Helium314 commented 6 months ago

If I understand correctly, you also struggled getting this to work, so I would conclude that it is not (in whole) due to my lack of experience with Android development, but something that may be out of scope for now.

I don't really remember, as that was quite a while ago... but I think getting the images to show was quite simple, and I didn't investigate further after noticing the issue when selecting the image (I think I was already working on something else when I noticed, and didn't bother to check further for the time being).

I think it's worth trying to use the StreetParkingDrawable as proposed above.

wielandb commented 6 months ago

Curious: What problem did you encounter? To the Android system, it shouldn't matter if one drawable is a vector drawable, a raster drawable or some completely custom drawable that draws itself dynamically. But who knows, maybe there is a bug in the recycler view, or the recycler view never triggers a redraw on a drawable or something

I have tried once again and can now report where I'm stuck.

In contrast to a simple resource, the function StreetParkingDrawable() needs a context as an argument (so that the images can be drawn in the right proportion to the screen, I think). That means, a Context object needs to be handed down through the function calls. I have looked at other Forms that use Item2, namely the AddCyclewayQuest, on how they handle it. When the call asDialogItem() (which is asItem() in my case) is made, requireContext() is passed as the context parameter.

https://github.com/Helium314/SCEE/blob/4e58ad719cc920798ce6482ee97ee8bb910204a5/app/src/main/java/de/westnordost/streetcomplete/quests/cycleway/AddCyclewayForm.kt#L171

So I tried doing the same thing:

    override val items = ParkingOrientation.values().map { it.asItem(requireContext()) }

It at least compiles fine, but as soon as the quest is opened, the following error occurs:

Process: de.westnordost.streetcomplete.expert.debug, PID: 25607
java.lang.IllegalStateException: Fragment AddParkingOrientationForm{9da85ca} (99166b38-8581-4c52-861f-d1dfe2df4391) not attached to a context.
at androidx.fragment.app.Fragment.requireContext(Fragment.java:972)
at de.westnordost.streetcomplete.quests.parking_orientation.AddParkingOrientationForm.<init>(AddParkingOrientationForm.kt:14)
at de.westnordost.streetcomplete.quests.parking_orientation.AddParkingOrientation.createForm(AddParkingOrientation.kt:24)

I am unsure how to "attach" this fragment to a context, as it works in AddCyclewayForm and I can't see anything special is done here to get a context. Help on this would be much apprechiated.

Helium314 commented 6 months ago

The issue is that items are initialized before the Fragment has a Context

Instead of override val items = ParkingOrientation.values().map { it.asItem(requireContext()) } try override val items get() = ParkingOrientation.values().map { it.asItem(requireContext()) }.

wielandb commented 6 months ago

Thank you! That was the last piece to the puzzle!

Good news: It works! I managed to reproduce what @Helium314 mentioned was possible. Bad news: Now I see the issue for myself... 😳

https://github.com/Helium314/SCEE/assets/29458682/85fa912c-a670-40ff-9c36-fa2e7ac8a440

But who knows, maybe there is a bug in the recycler view, or the recycler view never triggers a redraw on a drawable or something

Pinging @westnordost, maybe you can make conclusions from this video if your theory is correct? (And maybe even know how to fix it if it's a quick fix? 😳)

westnordost commented 6 months ago

Sorry, no idea

Helium314 commented 6 months ago

While I don't have an idea what's causing this, a simple solution is converting the drawable to a bitmap drawable (using .asBitmapDrawable(context.resources).

@wielandb why did you choose to add new strings for parking orientation instead of using the existing ones? They are used for the same thing, and are already translated.

wielandb commented 6 months ago

why did you choose to add new strings for parking orientation instead of using the existing ones? They are used for the same thing, and are already translated.

When the quest was a radio-select quest, I used strings with more explanation which were then boiled down to the one word descriptions. I removed them now and used the already available ones.

Helium314 commented 6 months ago

Looks good now, only a few rather minor things I noticed:

wielandb commented 6 months ago

Implemented all your suggestions. Should be ready for review now. 🙂