WIP: Show abandoned and disused nodes in 'things' and 'places' overlays.

[blu256]

This patch intents to propose a solution for #831.

With this patch:

• abandoned nodes are now shown in 'things' and 'places' overlays;
• disused nodes are tinted gray;
• abandoned nodes are tinted red;
• abandoned nodes contain '(ABANDONED)' in their label, just like disused nodes contain '(DISUSED)' in the current version;
• disused shops now show the appropriate shop icon instead of a 'vacant' shop icon, as the state of disuse is marked by the icon color already.

Some improvements needed to be made to how icons are colored in night mode to accomodate tinted icons.

Since it's my first time working with the SC(EE) codebase and Kotlin/Android code in general, feedback is greatly appreciated :)

[mcliquid]

Thanks a lot! I'd really appreciate it if the prefixes would also work in the building overlay :)

[mnalis]

Thanks for taking this on, @blu256 !

Few preliminary opinions from me (but let's wait for @Helium314 to see with which part he agrees); I'll try to check how it works in practice later.

• in SCEE, we default to behaviour being the same as in StreetComplete, and there be an option in preferences to enable new/changed behaviour.
  (the idea is that the user can come over from SC and only enable one specific thing they need, without having a behaviour changes elsewhere which might confuse them).

  Thus, I think you should add an switch like "Show disabled/abandoned features too" in preferences (in Quests section)

• IMHO it might be better to leave original function names (but wait for confirmation from @Helium314 about that), because:

  • it reduces the merge effort going forward (otherwise, any time any of those files gets new includes, automatic merge will likely fail) and there is a lot of maintenance time going on those merges already IIRC
  • given the need for preference; new name or the old name might fit better, so there is not clear winner. (unless code is duplicated and we keep both, but call one or the other depending on the preference -- which has its own set of problems)

• you should preferably only change default strings in values/strings_ee.xml:

  • SCEE-specific strings should go in values/strings_ee.xml, not values/strings.xml
  • other languages (values-*/strings.xml) are translated via external service (see README.md), and doing it via PR could cause merge conflicts and translations for whole language to fail IIRC.
  • unrelated changes to PR (e.g. those values-ast/strings.xml whitespace changes) should be fixed elsewhere (in that specific case, in StreetComplete poeditor translator)

Thanks again for your contribution!

[blu256]

Thank you for the detailed review, @mnalis :)

You make some very good points.

I will make the needed changes once I get some more free time, perhaps this weekend.

[Helium314]

Thus, I think you should add an switch like "Show disabled/abandoned features too" in preferences (in Quests section)

I fully agree on this part.

You could e.g. have Element.isPlaceOrDisusedPlace() do something like isPlace() || isDisusedPlace() || (preferences.showAbandonedPlaces && isAbandonedPlace()).
Tell me if you need help with adding or accessing the setting (haven't touched this in quite a while, so will need to look it up myself).

Showing disused and abandoned places in overlays should also be a setting, not sure where though. There are no per-overlay settings, so maybe also in the SCEE Quests section?

leave original function names
it reduces the merge effort going forward

I also agree on this. Changes in SCEE should take into account that I regularly merge SC changes (and I consider this more important than readability).
Currently files are even in different locations in SC and SCEE, which is causing issues for even slightly different files on the next merge (which I cancelled and plan to wait for some more major SC changes before continuing).

you should preferably only change default strings in values/strings_ee.xml

Original SC strings.xml should not be touched, all SCEE strings should go into strings_ee.xml.
Ideally you only touch the file in values, and not the translations, as changing translations may cause issues with translation changes done via weblate.

Btw I wonder why the same English strings are present 3 times in SC. Usually the priority is language+region, then language, then default. So having e.g. disused in the en and en_AU files seems useless (and a waste of space, as xml resources aren't compressed in the APK).

[blu256]

Tell me if you need help with adding or accessing the setting (haven't touched this in quite a while, so will need to look it up myself).

I have added the setting OK, but I couldn't figure out how to access it from Places.kt or Things.kt yet. How do I access an instance of the Preferences class?

leave original function names
it reduces the merge effort going forward
you should preferably only change default strings in values/strings_ee.xml
Original SC strings.xml should not be touched, all SCEE strings should go into strings_ee.xml.

Ok, I had no idea how SCEE applies changes to SC without diverging too much to keep things maintainable, so this information is most helpful. Will certainly fix these issues.

[mnalis]

I have added the setting OK, but I couldn't figure out how to access it from Places.kt or Things.kt yet. How do I access an instance of the Preferences class?

Easiest would be looking at code for existing similar check and copying what it does 😺

For example, if you search for REALLY_ALL_NOTES and reallyAllNotes ; you'll e.g. find in app/src/androidMain/kotlin/de/westnordost/streetcomplete/data/preferences/Preferences.kt:

    var reallyAllNotes: Boolean by prefs.boolean(Prefs.REALLY_ALL_NOTES, false)

and then those prefs.reallyAllNotes being used elsewhere.

(or, you can look e.g. at RESURVEY_DATE if you want another example to compare. Every one is little different, but have more commonalities too).

But if you get stuck with that issue of not being able to read from preferences, but the other stuff is working, no worries.

Just push to this PR other changes that you've made so far (especially those maintenance cleanups suggested above, which will make PR much easier to read, i.e.):

You could e.g. have Element.isPlaceOrDisusedPlace() do something like isPlace() || isDisusedPlace() || (preferences.showAbandonedPlaces && isAbandonedPlace()).

and simply define your showAbandonedPlaces beforehand to always be true, and then we'll work out how to make it actually read from preferences.

---

BTW, GitHub has Convert to draft / Mark as ready for review buttons (so they can be used instead of adding/removing "WIP" from title). I'll click the former know, as that seems intended by what "WIP".

[mnalis]

@blu256 and of course feel free to ask if you get stuck anywhere. This PR looks quite interesting, so I'd love to see it progress! (but do not feel pressured, do it on your own schedule as it best suites you!)

[blu256]

I am currently busy, I will resume work on this once I get some more time.