Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 31 commits into from
Mar 27, 2024
Merged

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

merged 31 commits into from
Mar 27, 2024

Conversation

wielandb
Copy link

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

  • How many cars can park here?
  • How many parking spots for disabled people are there?
  • In what orientation do cars park here?
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
Copy link
Author

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?

@wielandb wielandb changed the title Add quests for capacity=, capacity:disabled= and orientation for parking areas Add quests for capacity=, capacity:disabled= and orientation= for parking areas Nov 22, 2023
@wielandb wielandb changed the title Add quests for capacity=, capacity:disabled= and orientation= for parking areas Add quests for capacity=, capacity:disabled= and orientation= for amenity=parking Nov 22, 2023
@Helium314
Copy link
Owner

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
Copy link
Collaborator

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
Copy link
Owner

Yes, it was rejected: streetcomplete#4163 (comment)

@westnordost
Copy link
Collaborator

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

@wielandb
Copy link
Author

wielandb commented Jan 2, 2024

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.

@wielandb wielandb marked this pull request as ready for review January 2, 2024 18:01
@westnordost
Copy link
Collaborator

westnordost commented Jan 2, 2024

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
Copy link
Owner

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
Copy link

mcliquid commented Jan 3, 2024

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 wielandb marked this pull request as draft January 17, 2024 17:05
@wielandb
Copy link
Author

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:

  • Make the quest show the "big" images
  • Make the quest a simple AListQuestForm (currently implemented in the PR)
  • Remove the quest for now, and maybe revisit it later

How do you want to proceed @Helium314?

@wielandb
Copy link
Author

wielandb commented Mar 23, 2024

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
Copy link
Owner

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
Copy link
Author

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.

.map { it.asDialogItem(isRight, isContraflowInOneway, requireContext(), countryInfo) }

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
Copy link
Owner

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
Copy link
Author

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

video_2024-03-24_19-11-49.mp4

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
Copy link
Collaborator

Sorry, no idea

@Helium314
Copy link
Owner

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
Copy link
Author

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.

@wielandb wielandb marked this pull request as ready for review March 24, 2024 23:13
@Helium314
Copy link
Owner

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

  • showClarificationText always has the same value for the respective form, so it can be removed together with `ARG_SHOW_CLARIFICATION´ and related code.
  • AddDisabledParkingCapacityForm should better use String instead of Int, so the -1 -> yes and 0 -> no can happen directly in the form.
  • AddParkingOrientationForm has a bunch of unused imports
  • ParkingOrientation could re-use the existing ParkingOrientation class and ParkingOrientation.osmValue (copy or make it public)
  • orientation_display_val and position_display_val should follow the usual camelCase

@wielandb
Copy link
Author

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

@Helium314 Helium314 merged commit 2cc88a3 into Helium314:modified Mar 27, 2024
@wielandb wielandb deleted the parking-area-quests branch March 27, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants