-
-
Notifications
You must be signed in to change notification settings - Fork 353
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
special speed zone sign layout for many countries #3359
Conversation
Looking at this https://commons.wikimedia.org/wiki/Category:Square_speed_limit_road_signs_with_circle , please do the following:
|
I have bad news here! It is not working anymore. I spotted it after my mcc localization of untranslatable zone string failed to work. I will try to locate what broke this, I remember that it used to work. If my memory and understanding of code is right - traffic signs in Sweden should have yellow background. 1429ab2 is broken, v32.0 is broken, v31.0 compiled with google/dagger#1339 (comment) bugfix applied[0] and in Sweden traffic light are still white, v20.0 has not yet this implemented, v25.1 has white signs despite
giving
and https://en.wikipedia.org/wiki/Mobile_country_code listing Sweden as 240. I even set GPS position to Sweden just in case. Just in case: https://user-images.githubusercontent.com/899988/136814332-38c87dca-0002-4c0f-a425-48c337c5cb86.png (I will continue bisection). [0]
replace
with
If this does not exist, look for Earlier it seemed that databinding would be suitable for parametrized layouts but...
|
|
so in maxspeed quest it is also not working? |
Hmm, the inflator has a modified context (that sets the MCC to the current country) to inflate country-specific layouts, see However, it seems that that (modified) context is not used to resolve any resources within the country-specific layouts but another context - probably the activity context(?). In early 2019, I added this comment
that mentioned this. So maybe it would be possible to instead modify the activity context rather than the fragment context. This is probably the same hackery that needs to be done for the language-selection to work smoothly without a lot of code. The other thing that should work is to use the fragment's modified context ( |
Or maybe get back to custom layouts without mcc-dependented resources? I admit that when I looked at commit that introduced But given that magic is actually not working and requires even more magic to work... I would consider reverting ec2529a as one more solution (I am not saying that it is preferable, but I would at least consider it as an option). |
Just look at all the I mean, that the layouts are per mcc is also magic we rely on to work. If we were to abandon that, we'd need to get rif of all resources referring to MCCs but instead put this into code. A possibility of course, but again, this is a lot of work. And not sure what we gain with this. |
Let's grant this bug an own issue and be this ticket only about the speed zone sign which should not be touched by this issue directly. |
streetcomplete#2954 - initial part
covers Italy (label on top) covers North Macedonia (label on bottom) covers Poland (no label)
0201488
to
caad4bf
Compare
label was not actually on top
@westnordost Thanks for handling this bug! |
If you see anything wrong in #3359 (comment) list - please comment. If you know how zone speed sign look for one of missing countries (or you know that country is without it) - also please comment. |
Bulgaria doesn't have such signs since there is no definition in the traffic law. Feel free to ping me if you get any Cyrillic signs that you can't decipher. |
Thanks @Dimitar5555 - every part of that was very useful. Now just China is left :)
Fortunately it seems that all are using the same text that I managed to obtain.
I will likely open a new issue for that... |
The Chinese sign is |
I don't see Croatia in that list; we have text |
Japan: https://commons.wikimedia.org/wiki/File:ZONE30-Sign.jpg bottom "区域 ここあら" (zone, begin)
https://commons.wikimedia.org/wiki/File:Ireland_road_sign_F_403.svg is only used together with a wrong |
@mnalis @Kovoschiz Thanks!
As slow zone is handled separately, than means that Ireland de facto has no zone speed limits. Also, it will be worth checking is there already support for yellow background on living zone traffic sign. Note: I will likely keep this list for some time when I have time but are for some reason unable to handle something more complex as it is mostly drudgery with translating country names to https://en.wikipedia.org/wiki/Mobile_country_code and creating bunch of layouts and texts, like in https://github.com/streetcomplete/StreetComplete/blob/caad4bfbc1378ee813f626a467a12d189bf79cc5/app/src/main/res/values-mcc294/strings.xml https://github.com/streetcomplete/StreetComplete/blob/caad4bfbc1378ee813f626a467a12d189bf79cc5/app/src/main/res/layout-mcc294/quest_maxspeed_zone_sign.xml |
Sweden has a sign for speed zones, but doesn't use it. I've never seen any other zones apart from no parking/no stopping zones and the occasional parking zone. We don't have any signs for default speed limits either, all speed limits are signed with a regular speed limit sign. The exception are recommended lower speed limits which use an end sign.
Denmark has two different signs for speed zones. The blue one is for areas where there is traffic calming and the the speed is generally 30 or 40 km/h (20-45 allowed). The regular one is for what you'd expect. |
Alternatively you can put the strings in the code as well and set the text programmatically which is one line of code. Maybe that would be both less effort and is much easier to read. E.g. private fun getZoneText(countryCode: String) = when(countryCode) {
"DE" -> "ZONE"
"HR" -> "ZÓNA"
...etc
} Even doing that for the layout itself may be less effort than creating XML resources for dozens of countries. |
Or maybe a file default: false # no speed limit zones known by default
DE: "ZONE"
HT: "ZÓNA"
… |
Right, also possible, this is what the country metadata is for after all. Then e.g.
could also be done |
What about:
|
from hat I remember there was something wrong with asserts capture if body into {}
Proposed changelog entry (mirrored in top post): Speed limit quest - labels on speed limit zone sign are now country-specific (by @matkoniecz). Note that speed limit quest is disabled by default as it requires more involved survey, you can enable it in settings. This specific quest is near the bottom of the quest list. |
Is there anyone from Ireland here or familiar with signs there? I am confused a bit - are there valid This will likely not be done as part of this PR but I added it on my TODO pile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks fine, but I haven't tested this in action. EDIT: Works and looks fine in Germany.
If anyone would test in specific country: please mention it here so I can mark proper checkbox in the initial post (or mark them directly if you can do this) |
What exactly is missing here? Is it ready for review&merge? In the list it shows up as 16/46 tasks finished. |
It is finished and ready. Checkboxes mark where it was tested, what - I think - covered various variations. Though if you think that it would be useful I can also test in remaining 30 locations. |
app/src/main/java/de/westnordost/streetcomplete/quests/max_speed/AddMaxSpeedForm.kt
Outdated
Show resolved
Hide resolved
Nah that's fine |
Since no new strings were added and you tested this well, this might as well go into a minor release (in case there'll be one), so we can merge it now |
@westnordost if you want you can use following as a changelog entry
(I can also add it directly if you think that it is fine) |
Cleaned up:
I think technically "it requires more involved survey" should be either "…a more involved survey" or "…more involved surveying", but for changelog purposes it's fine. |
Regarding updates on the changelog:
To keep the overview till which commit the changelog was maintained, you know. In general, the changelog entries should be very short / simplified. For more information and notes, there is always the link to the actual ticket. So for this PR, I'd rather add someting like "display speed limit zone signs as they appear in your country in the form" or similar. |
Any anyone with write acces can do this, without a PR, if he follows these rules. I wanted to put this into the "working together efficiently" post in the discussion forum but then again I thought this is not going to be a big effort for me as I need to go through the commit history anyway before each release again to see if hte changelog is up-to-date. |
source: trusted contributor in streetcomplete/StreetComplete#3359 (comment)
Thanks! done in streetcomplete/countrymetadata#8 in general, for such thing it is better to open new issue - closed PRs and issues can be no longer watched, and if not processed immediately it may be forgotten Now once SC on new release gets new countrymetadata data, then Bulgaria should get slow zone tagging option. |
changelog:
Speed limit quest - labels on speed limit zone sign are now country-specific (by @matkoniecz). Note that speed limit quest is disabled by default as it requires more involved survey, you can enable it in settings. This specific quest is near the bottom of the quest list.
fixes #2954
It has quite irritating layout duplication, but maybe it is safe to assume that it will be stable?
And alternative would be having layouts "zone text above sign", "zone text below sign" modified in various countries and it may not be sufficient anyway.
Progress (all are handled, ones with checkbox are also tested):