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

Refactor sidewalk surface quest #3873

Conversation

arrival-spring
Copy link
Contributor

Refactoring sidewalk surface quest to use AStreetSideSelectFragment as suggested in original PR.

Things missing were:

  • initStateFromTags (here to disable one side or the other if there's no sidewalk)
  • a way to do a follow up question after tapping on a side (here to ask about generic surfaces)

Both of these are good things to have if cycleway and parking quests were to be refactored to use AStreetSideSelectFragment

I'm not 100% sure about the way in which I've added them both, but they do work.

The problem remaining though is the last answer button. LastPickedValuesStore assumes that it can convert the last answer to and from a string, however here the last answer value is something like SurfaceAnswer(value=ASPHALT, note=null). This gets split on the comma and totally fails when it attempts to convert it back.

I'm not sure of the best way to solve this in general.

@arrival-spring
Copy link
Contributor Author

Also, downgrade is that the icons for each side are no longer shown in circles, and the title of the surface is displayed on each side, but you can't really see it behind the icon.

@westnordost
Copy link
Member

On the LastPickedValuesStore, you could require subclasses of AStreetSideSelectFragment to specify a a serializer and deserializer function.

downgrade is that the icons for each side are no longer shown in circles

Why though? The side select puzzle supports setting a de.westnordost.streetcomplete.view.Image though, no? Couldn't the data model (be an interface) and have an Image instead of a @DrawableRes Int?

# Conflicts:
#	app/src/main/java/de/westnordost/streetcomplete/quests/surface/AddSidewalkSurfaceForm.kt
@arrival-spring
Copy link
Contributor Author

I've done the serializing and it all seems to work

Why though? The side select puzzle supports setting a de.westnordost.streetcomplete.view.Image though, no? Couldn't the data model (be an interface) and have an Image instead of a @DrawableRes Int?

The problem I've run into with the above is that in items I want to have RotatedCircleDrawable(resources.getDrawabale(... but then I get IllegalStateException: Fragment AddSidewalkSurfaceForm ... not attached to a context. (from using resources)

@westnordost
Copy link
Member

can't items be
override val items: List<Item> get() = ...
(i.e. a function instead of a value set in constructor?

(Sorry I didn't look into the code yet)

@arrival-spring arrival-spring marked this pull request as ready for review April 3, 2022 15:17
@arrival-spring
Copy link
Contributor Author

That was it, thanks.

Now ready for review.

The only change in functionality from before is that it now shows the surface on the road in the view after it has been selected (but somewhat hidden behind the icon).

@westnordost
Copy link
Member

Sorry, forgot about this. Will review this soon

@westnordost westnordost self-assigned this Apr 9, 2022
@westnordost
Copy link
Member

I'm working on this now. I needed to checkout the branch to read up on it, so I started to do all kinds of small adjustments (some unrelated to the PR but to the topic), so whatever I'll find maybe imperfect with this PR I'll fix it myself. Maybe I'll also get around to also let the cycleway quest to inherit from that or even refactor it to use that ViewController pattern I mentioned in the discussion forum some time ago.

@westnordost
Copy link
Member

Hmm, I'll do the ViewController stuff later after all. What I changed on your PR was to

  • replace the serialize/deserialize with an abstract function where implementing classes must return the display item for any given value
  • don't save to favs if a generic surface was selected (solves issue btw that would return corrupt data if the user included a "#" in his note)

@westnordost westnordost merged commit e117956 into streetcomplete:master Apr 12, 2022
@arrival-spring
Copy link
Contributor Author

Thanks for doing all that!

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.

2 participants