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

Separate view data in quest data classes #2494

Closed
westnordost opened this issue Jan 12, 2021 · 2 comments · Fixed by #2501
Closed

Separate view data in quest data classes #2494

westnordost opened this issue Jan 12, 2021 · 2 comments · Fixed by #2501

Comments

@westnordost
Copy link
Member

westnordost commented Jan 12, 2021

Task

For the cycleway quest, the cycleway answer options are cleanly seperated into two files: Cycleway.kt contains the "pure data" (the enum) and CyclewayItem contains the view-related part of a cycleway answer option.

This is better than how it is done for for example the surface quest (and others). There, in Surface.kt, the enum is not pure data because it contains reference to Android-specific view stuff. So for these quest(s), the data class should be refactored to be like for the cycleway class.

Example

So it should be....

enum class Surface(val value: String) { // or osmValue?
  ASPHALT("asphalt"),
  // etc....
}

// in SurfaceItem.kt
fun Surface.asItem(): Item<Surface> {
   // ... well, see CyclewayItem.kt, ...
}

Reason

Apart from it being good style generally to seperate UI from data logic, it is a step towards making #1892 or any other port from Android to something else a little less work to do.

Quests where this "old style" is used

  • BuildingType
  • Surface

And then

And there is more. Quest types should no longer have a String as parameter but an enum.

Compare AddRecyclingTypeForm (new style) with AddRailwayCrossingBarrier (old style). It is okay for the enum to define an OSM value like in the example with surface above.

@westnordost
Copy link
Member Author

Maybe you are interested in helping out here, @FloEdelmann

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants