search

Helium314 / SCEE

OpenStreetMap surveyor app for experienced OSM contributors
GNU General Public License v3.0
115 stars 8 forks source link

Create new PR with ServiceBuildingType #464

Closed mcliquid closed 8 months ago

mcliquid commented 8 months ago

This is a draft PR to work on the new Service Building Type values.

Helium314 commented 8 months ago

Could you add the other changes from https://github.com/Helium314/SCEE/commit/59c71c791c522c62ba87a560390ee70f92aa3936 too? So the changes can be tested directly. If you want I can do this too.

mcliquid commented 8 months ago

@Helium314 I'm not that fit with Git, so I guess now it looks like I made the changes because I merged your changes manually. I wasn't able to continue working on your commit because I obviously didn't have write permissions there. I hope that works now? You should always be able to edit as owner or?

Helium314 commented 8 months ago

All fine, looks good now. I'm allowed to edit the PR, but will only edit anything when it's clear you're ok with that.

mcliquid commented 8 months ago

@Helium314 of course, you're far more experienced! Where can I assist best? Icons for the categories only, or for every entry? Some descriptions per entry?

Helium314 commented 8 months ago

I'll adjust the code a little, so you can easily add proper names and descriptions and see how it looks in the app. Icons are a lot of work... if you can get or create good ones it's highly welcome, but it's not necessary.

Helium314 commented 8 months ago

I hope from my commit it's clear how to add titles and descriptions to the service building items. You will need to add a titleResId for every type, or you will get a build error. Descriptions are optional, so you only need to add them where you think it's necessary.

Helium314 commented 8 months ago

Icons can be added in the same style, but instead of e.g. R.string.quest_service_building_power you have to use a drawable, e.g. R.drawable.ic_building_terrace.

mcliquid commented 8 months ago

@Helium314 I did some more work on the quest. In the next step I would optimize the terminology and the descriptions.

Unfortunately, I just do not get it to display the icons, then I would create some more at least for the categories. If I rebuild it like the BuildingType Quest then the error "Cannot access 'iconResId': it is private in file" appears. Maybe you can help here?

In general I wonder if it should be possible to select the categories at all. I would make all categories unselectable, what do you think?

Helium314 commented 8 months ago

ServiceBuildingType.iconResId is never accessed. If you're using Android Studio you can see that the iconResId is displayed in grey, indicating it's not used. You can just delete val iconResId = null from ServiceBuildingType.asItem(), then it works (will use ServiceBuildingType.iconResId)

In general I wonder if it should be possible to select the categories at all

It could be useful e.g. if the user knows the service building is "something with telecommunication", but can't answer in more detail. Though if there is no applicable generic type, just set the ServiceBuildingType of the category to null, e.g. OTHER_SERVICE(null, listOf(...

mcliquid commented 8 months ago

@Helium314 With your support I have come a long way, thank you!

A few questions have occurred to me about the quest in general:

  1. Oil has only one entry, should there just be the category instead of just the category with only one entry?
  2. With Telecom there is also Internet Exchange, it is documented in the Wiki but has however only 6 uses. Should we include it or not?
  3. Under "Other" I put the values for Sewerage and Heating. That would actually be a category as well, but without further distinctions. How should we accommodate this?
  4. Since the quest is already very special, I have added a description for all values. This is an enormous amount of translation - is it justified? What do you think?
  5. In the power category there are "Switchgear" and "Power Plant". Nobody will recognize from the outside if it is a transformer or a switchgear in a building. And a Power Plant can be anything - should I delete the two values even if they are in use a few times?

In the next step I would create the icons at least for the categories. Whether this makes so much sense for the sub-values in terms of recognizability I do not know yet.

Current screenshot: https://github.com/Helium314/SCEE/assets/3351668/9f214907-05f5-4d3e-966e-562a04f6c65e

Helium314 commented 8 months ago
  1. Oil has only one entry, should there just be the category instead of just the category with only one entry?

You could just add it to the other category. I guess a category with 0 or 1 entries would just feel wrong.

  1. With Telecom there is also Internet Exchange, it is documented in the Wiki but has however only 6 uses. Should we include it or not?

In my opinion: yes, unless there is some undocumented but more frequently used alternative tagging.

  1. Under "Other" I put the values for Sewerage and Heating. That would actually be a category as well, but without further distinctions. How should we accommodate this?

Like 1.: adding to other is fine.

  1. Since the quest is already very special, I have added a description for all values. This is an enormous amount of translation - is it justified? What do you think?

Just judging from your screenshot, the descriptions are already helpful for me. They might not be necessary everywhere, but are certainly helpful to users who don't have much knowledge about service buildings. Hence I think they are justified. Though it should be possible to have some types without description, e.g. power plant should be pretty clear without further text. Same should go for ventilation shaft, railway wash station and maybe some more.

  1. In the power category there are "Switchgear" and "Power Plant". Nobody will recognize from the outside if it is a transformer or a switchgear in a building. And a Power Plant can be anything - should I delete the two values even if they are in use a few times?

For many service buildings I wouldn't be able to determine the type if it wasn't written on (warning) signs. If the user doesn't know what's inside they hopefully simply select the category.

In the next step I would create the icons at least for the categories. Whether this makes so much sense for the sub-values in terms of recognizability I do not know yet.

Current screenshot: https://github.com/Helium314/SCEE/assets/3351668/9f214907-05f5-4d3e-966e-562a04f6c65e

I think icons do help with selection, but for that it's necessary to have less similar icons than now. If you remove the service building and only show power/water/... icons it would be easier to tell apart. Not sure, but having icons for the categories only might look weird (didn't try though).

mcliquid commented 8 months ago

@Helium314 I changed the icons of the main categories but they aren't visible in the app (also after wiping the data in the simulator) - do you have a tip? See: https://github.com/Helium314/SCEE/assets/3351668/1f3fc0c5-afcd-4393-966b-b530bbb18bc3 image

Helium314 commented 8 months ago

These are the icons for ServiceBuildingType, while categories use icons for ServiceBuildingTypeCategory

mcliquid commented 8 months ago

@Helium314 oh gosh, thanks for that hint! Icons for main categories are working now: https://github.com/Helium314/SCEE/assets/3351668/c58cffff-fcd6-4eda-abf4-d7826609dc8c

Helium314 commented 8 months ago

Thanks, so far it looks good. I'll look over the code and make some adjustments where necessary.

Could you add the icon sources and licenses to https://github.com/Helium314/SCEE/blob/modified/res/graphics/authors.txt?

mcliquid commented 8 months ago

Current preview:

Helium314 commented 8 months ago

I updated the code a little. The most significant change is that I removed the ServiceBuildingTypes for unselectable categories. When replacing the category with null, it should really become unselectable instead of just crashing on clicking the ok button (didn't test though, will have time for that later today).

Is there anything else missing for tagging? E.g. https://wiki.openstreetmap.org/wiki/Tag:man_made%3Dventilation_shaft implies that man_made=ventilation_shaft should not have a building tag, which is removed when applying the answer in AddServiceBuildingType.

Further, a similar quest like https://github.com/streetcomplete/StreetComplete/issues/4505 could be done, possibly re-using a lot of code from this quest. Are you interested in that one too?

Helium314 commented 8 months ago

Some comment on details, all looks good. Only one minor thing: for some reason only the last 2 selections are stored. But fixing that is my task.

mcliquid commented 8 months ago

@Helium314 I changed some things to be combatible with the future street cabinet quest, removed some old comments and rearranged some code lines. I also checked the tagging again and would release it that way in good conscience 🚀

Helium314 commented 8 months ago

Great, thanks a lot for all this work!