Quest: How much do you have to pay to use this parking spot?#6653
Quest: How much do you have to pay to use this parking spot?#6653marekkrug wants to merge 91 commits into
Conversation
paulklie
left a comment
There was a problem hiding this comment.
great work so far, here are some ideas / topics for discussion
westnordost
left a comment
There was a problem hiding this comment.
Regarding UI design, I'd somewhat prefer a UI like this
[ ] EUR / [ hour ▼ ]
The currency simply being a text (unless there are actually several currency in use in a country??).
(Bonus: It would be nice if not the ISO currency id was used in display, but the actual currency symbol, e.g. €, $, £, ...)
The amount, being a decimal input field, you can use the generic composable I created de.westnordost.streetcomplete.ui.common.input.DecimalInput which handles decimal points locale-aware. I guess allowed would be at most 2 decimal digits?
Finally, in the issue it came up that actual pricing may vary depending on a number of factors which may confuse the user what to answer. An example from the discussion on the wiki page:
- 1st hour: 2$ / hour
- subsequent hours: 1$ / hour
Is there a reasonable hint text that could be added to explain what the user should answer in this case?
|
Also, I supposed specifically for this quest, it would make sense to always add |
|
ah, right
El 31 de enero de 2026 2:59:22 GMT+04:00, Marek Krug ***@***.***> escribió:
…marekkrug left a comment (streetcomplete/StreetComplete#6653)
writing `2.00 USD/hours` would be a bit weird in my opinion, in the wiki, the correct way to write it is `2.00 USD/hour`...
--
Reply to this email directly or view it on GitHub:
#6653 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
@marekkrug Sorry, I didn't mean to close your PR. I cleaned up and deleted the I expected GitHub to behave like GitLab, which would have just changed this PR's target branch to Could you please open another PR? Sorry for the extra work! |
|
I just created a new branch |
|
Thanks! I could change the target branch back to |
|
The quest is now running and working as far as I can see. Are there any more problems that need to be fixed in this quest? Otherwise, it would be fine for me if it was merged in this state and to later add quests like "How much do you have to pay to use this toilet?" or similar in a seperate merge request :) |
|
I haven't been following this PR lately. Are all the previous comments resolved? As a PR author, you can tap that button yourself. |
|
I read through them and resolved a lot of them. Looks like there's nothing really remaining other than potential other quests. |
|
Alright, I will do a final review soon |
westnordost
left a comment
There was a problem hiding this comment.
Phew, I am sorry to say this, but the currency formatter stuff is quite bonkers.
Each only in parts:
- the formatting is all over the place and there is commented out stuff plus open TODO comments left
- the doc comments make no sense
- the iOS implementation at least again in parts can not work. E.g.
getCurrencyCodeFromLocalejust completely ignores the passed parameter and looks like you copy-pasted the code from the other method - the API of
CurrencyFormatter(again, as for every point here in parts) make no sense (why isgetCurrencyCodeFromLocalea method of an instance ofCurrencyFormatter, what does the class have to do with a currency code?) - the tests for
CurrencyFormatteronly test half of the interface - the implementation of
CurrencyFormatteron both platforms is over-complex and also wrong. The parameter is a country code and you search through all available locales to find the first locale that uses the currency of the country with the given code in order to format it (because you need a locale to format). Now, imagine the Euro is used in a country where one formats currency like this "1.500,00 €" (France) and in another country like this "€ 1 500.00" (Ireland). Oopsie! So, always theLocale(with region) must be passed toCurrencyFormatter. - the second static function of
CurrencyFormatElementslooks like it is completely copy-paste from the other one, plus, only one of these is used (outside tests) anyway! Funnily enough, the one that is not tested is the one that is actually used, while the one that is tested is not used in production 🤦 - well, maybe more things, I lost the overview
I reckon that at least parts of this confusion might have been introduced as misunderstandings due to my requests to change your initial implementation of this, of course.
And since I looked into this code so deeply already, it makes sense if I rewrite that part myself rather than play PR ping pong. So, that's what I will do, you don't need to worry about this part.
The other parts I only skimmed through, but nevertheless I found some stuff. First and foremost, please make sure that you review your own code, too. I notice there are some TODOs and similar comments as well as sections of leftover commented out code.
For the other stuff, see below. Note however that this was just from skimming through it, there might be more in the details.
| fun DurationUnit.getDisplayName(form: AddParkingChargeForm): String = when (this) { | ||
| DurationUnit.HOURS -> form.getString(R.string.quest_parking_charge_hour) | ||
| DurationUnit.DAYS -> form.getString(R.string.quest_parking_charge_day) | ||
| DurationUnit.MINUTES -> form.getString(R.string.quest_parking_charge_minute_short) | ||
| } |
There was a problem hiding this comment.
Have a look at how the string resource is created for other quests / composable UI. Could also use the DurationUnitDropdown but add a parameter that decides whether the unit should be written always in singular or not (analogous to my comment above)
# Conflicts: # app/src/androidMain/res/values-de/strings.xml
…rationUnit. Also added formatting Option to DurationUnit
… quest to let it compile again for debugging
|
I finally took the time and fixed a lot of the remaining remarks. In the meantime, the getTitle function described at https://github.com/streetcomplete/StreetComplete/blob/master/CONTRIBUTING_A_NEW_QUEST.md#gettitle seems to have been updated, so I don't know how to fix the title. In order to let it compile again, I temporarily took the title from another quest and added a todo until I figure it out :) |
westnordost
left a comment
There was a problem hiding this comment.
There are still some things commented out, and several TODOs. Do you need input for something?
| MINUTES -> if (value != 1.0) "minutes" else "minute" | ||
| HOURS -> if (value != 1.0) "hours" else "hour" | ||
| DAYS -> if (value != 1.0) "days" else "day" | ||
| fun toOsmValue(alwaysSingular: Boolean = false, ignoreValue: Boolean = false): String { |
There was a problem hiding this comment.
Always singular? Ignore value? Looks like what you really want is to extend DurationUnit like this
enum class DurationUnit {
MINUTES,
HOURS,
DAYS;
fun toOsmValue(usePlural: Boolean = false): String = when (this) {
MINUTES -> if (usePlural) "minutes" else "minute"
HOURS -> if (usePlural) "hours" else "hour"
DAYS -> if (usePlural) "days" else "day"
}
}and use that accordingly, ditch alwaysSingular: Boolean = false, ignoreValue: Boolean = false
| fun DurationUnit.getDisplayName(form: AddParkingChargeForm): String = when (this) { | ||
| DurationUnit.HOURS -> form.getString(R.string.quest_parking_charge_hour) | ||
| DurationUnit.DAYS -> form.getString(R.string.quest_parking_charge_day) | ||
| DurationUnit.MINUTES -> form.getString(R.string.quest_parking_charge_minute_short) | ||
| } |
This PR is a new Quest with the name "How much do you have to pay to use this parking spot?" and was described in the Issue #6651.