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.

Sign up for GitHub

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

Jump to bottom

Sidewalk surface quest #3735

Merged
merged 16 commits into from
Mar 4, 2022
Merged

Sidewalk surface quest #3735

merged 16 commits into from
Mar 4, 2022

Conversation

arrival-spring
Copy link
Contributor

@arrival-spring arrival-spring commented Feb 7, 2022 •
edited
Loading

Allow to add the surface of sidewalks tagged on the road, fixes #1593

Shows up for any suitable road with valid and complete sidewalk tagging. If there's no sidewalk or it's separately mapped for one side then that side is not selectable.

Images:

Todo/questions:

  • It needs a new icon (it's just using the footway part surface icon at the moment)
  • I don't like how the images are shown in the rotater view after having been selected as the photos don't tile well.
    Two possible solutions:
  • Find/create images for all of the surfaces which will tile (vector graphics)
  • Use the icon overlay view used in the parking quest and just create circular images for each surface using the current images which would then essentially hover over that side.
  • Something else?
  • Should it use the same surface form as the other surface quests? I feel like this would need some reworking of that though, and is then less consistent with other quests for things on different sides
  • Should there be an explicit option to say that sidewalk is wrong? At the moment the only thing the user can do is the standard leave a note option

@arrival-spring arrival-spring marked this pull request as draft February 7, 2022 21:24
@westnordost

This comment was marked as resolved.

Sign in to view
@matkoniecz

This comment was marked as resolved.

Sign in to view
@westnordost

This comment was marked as resolved.

Sign in to view
@arrival-spring

This comment was marked as resolved.

Sign in to view
@westnordost
Copy link
Member

Ok, I think my concerns are partly refuted and partly not a reason (enough) to not include this in the app. Let's continue with this then, I'll hide the discussion about my concerns as "resolved".

@ghost
Copy link

ghost commented Feb 11, 2022

Hey, I came to github just to suggest this feature, but to my surprise it is already being worked on. Haha

sidewalk_surface

I think something like this could look nice as a quest image (a bit more polished of course). What are your opinions?

@arrival-spring
Copy link
Contributor Author

Oh no! You don't need a custom view for that nor an external dependency! Making a drawable circular is no black magic. Please check out view/RotatedCircleDrawable. You don't even need it rotated, but you can still just use that class.

Thanks! I did think I was probably missing something obvious

@arrival-spring
Copy link
Contributor Author

Looks much better now, and the last answer button works well too

I don't have any particular opinions on the icon, the one from @krisdoodle45 looks good to me though.

My other question was:

Should there be an explicit option to say that sidewalk is wrong? At the moment the only thing the user can do is the standard leave a note option

@matkoniecz
Copy link
Member

Should there be an explicit option to say that sidewalk is wrong? At the moment the only thing the user can do is the standard leave a note option

I think that note is fine. Though after release I would encourage looking at https://github.com/streetcomplete/StreetComplete/blob/master/CONTRIBUTING.md#solving-notes and checking is there large volume of notes caused by such issue.

@arrival-spring
Copy link
Contributor Author

I guess I also meant whether there should be an other answers item that says "Sidewalk is wrong" and then leads user to leave a note?

@matkoniecz
Copy link
Member

If it would do anything, then it could remove sidewalk tags. Though skipping it is fine and leaving regular way of leaving note is likely fine.

@ghost
Copy link

ghost commented Feb 16, 2022

Hey, here's a better looking image in SVG format as well:

sidewalk_surface

@arrival-spring arrival-spring marked this pull request as ready for review February 17, 2022 18:20
westnordost
westnordost reviewed Feb 23, 2022
View reviewed changes
Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man, ... this is a lot of code, and complexity! Whew!

And that must all be maintained ... I have a bad feeling about if one dwelved even deeper here - e.g. cycle track obligatory or not, cycle track segregated from sidewalk or not, smoothness of sidewalk, smoothness of cycle track, ...

Let's add this quest but let's not add any further detailing quests for street-sides for now.

I didn't look through everything now, but here are already some comments:

app/src/test/java/de/westnordost/streetcomplete/quests/surface/AddSidewalkSurfaceTest.kt Outdated Show resolved Hide resolved
app/src/main/java/de/westnordost/streetcomplete/quests/surface/AddSidewalkSurfaceForm.kt Show resolved Hide resolved
app/src/main/java/de/westnordost/streetcomplete/view/StreetSideSelectPuzzle.kt
Comment on lines +206 to +220
fun onlyLeftSideClickable() {
binding.leftSideContainer.isClickable = true
binding.rightSideContainer.isClickable = false
}

fun onlyRightSideClickable() {
binding.rightSideContainer.isClickable = true
binding.leftSideContainer.isClickable = false
}

fun bothSidesClickable() {
binding.rightSideContainer.isClickable = true
binding.leftSideContainer.isClickable = true
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the public interface explosion in StreetSideSelectPuzzle could be solved by only having one method to set one StreetSideSelectItem (or similar) that contains all that information (text, image, floating icon, visible or not, clickable or not, ...) since all of that stuff is set at the same time anyway. Or maybe expose an object on the interface so one can call puzzle.left.clickable = true etc.

But this needn't be part of this PR.

app/src/main/java/de/westnordost/streetcomplete/quests/surface/AddSidewalkSurface.kt Outdated Show resolved Hide resolved
app/src/main/java/de/westnordost/streetcomplete/quests/surface/AddSidewalkSurface.kt Show resolved Hide resolved
app/src/main/java/de/westnordost/streetcomplete/quests/surface/AddSidewalkSurface.kt Outdated Show resolved Hide resolved
app/src/main/java/de/westnordost/streetcomplete/quests/surface/AddSidewalkSurface.kt Outdated Show resolved Hide resolved
@arrival-spring arrival-spring mentioned this pull request Feb 26, 2022
Merged
@arrival-spring
Merge branch 'master' into sidewalk-surface
3431791 
# Conflicts:
#	app/src/main/java/de/westnordost/streetcomplete/quests/QuestModule.kt
#	app/src/main/java/de/westnordost/streetcomplete/quests/sidewalk/AddSidewalkForm.kt
@matkoniecz
Copy link
Member

This branch has conflicts that must be resolved

let me know if you have no idea how to solve THAT

@arrival-spring
Copy link
Contributor Author

let me know if you have no idea how to solve THAT

Thanks for the offer, but I've managed it.

So, I think this is all sorted, it all runs as expected and the tests all pass.

I believe I've fixed everything from reviews (except for things noted as not needed in this PR).

This was referenced Feb 28, 2022
Merged
Closed
Closed
@westnordost westnordost merged commit f3bf845 into streetcomplete:master Mar 4, 2022
@arrival-spring arrival-spring mentioned this pull request Mar 26, 2022
Closed
@Helium314
Copy link
Collaborator

Maybe I'm missing something obvious, but I didn't find any explicit comment on this:
Why is the sidewalk surface only asked for specific roads, i.e. highway ~ trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|unclassified|residential?

@matkoniecz
Copy link
Member

For which it is missing? It is likely done this way to avoid asking on say highway=proposed with sidewalk tag.

@Helium314
Copy link
Collaborator

I don't actually miss the quest anywhere, I was just curious.
Excluding highway=proposed and highway=construction does make sense, thanks.

@arrival-spring arrival-spring deleted the sidewalk-surface branch September 20, 2022 20:25
@arrival-spring arrival-spring mentioned this pull request Sep 22, 2022
Closed
HolgerJeromin added a commit to HolgerJeromin/StreetComplete that referenced this pull request Sep 28, 2022
@HolgerJeromin
add author for sidewalk_surface.svg
5b94f00 
source:
streetcomplete#3735 (comment)
@HolgerJeromin HolgerJeromin mentioned this pull request Sep 28, 2022
Merged
@arrival-spring arrival-spring mentioned this pull request May 16, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FloEdelmann FloEdelmann FloEdelmann left review comments

@westnordost westnordost westnordost approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ask about surface of sidewalks marked as tag on the road
6 participants
@arrival-spring @westnordost @matkoniecz @Helium314 @FloEdelmann @HolgerJeromin