resurveying opening hours

[matkoniecz]

see #1676 (comment) for things requiring fixes

---

fixes #1651, fixes part of #1112

It is a quite big PR, pulling in a new parsing dependency, quite complex checker of a parsed object whatever it fits SC requirements and the first resurvey quest.

Icon is the same as for AddOpeningHours, at least for now. I though whatever and how to distinguish icons for resurvey quests from normal quests. I think that exactly the same icon for the normal quest and a resurvey quest should be OK. I am unsure how to handle disabling/enabling resurvey quests.

This one is added like any other quest, I would leave potential changing this for a future. Enable/disable silently following normal quest, making impossible to disable resurvey quests? Have them on the list of quest types as a separate entry like this one? Have a global setting „disable resurvey quests?”)

Note that opening hours may be reformatted, without changing structure. I think that it is OK, example changes are documented as tests. Reformatting would apply on user solving quest.
For example Tu-Su 10:30 - 18:00 to Tu-Su 10:30-18:00. Or Tu-Su 10:30-28:00 to Tu-Su 10:30-04:00. Or Mo 9-19 to Mo 9:00-19:00.

I think that it would be a good idea to start the code review from looking through tests added in this PR. Some decisions, like handling of cosmetic changes, are kind of subjective and maybe a different decision should be taken.

There are some opening hours displayable with SC (like ones with an open end) that are not handled for now to reduce complexity of this PR. It should be fairly easy to add them in the future as a separate PRs.

---

My work on this pull request and UX testing was sponsored by a NGI Zero Discovery grant

[matkoniecz]

@westnordost

I suspect that it is not intentional, but I want to check.

Why toOpeningWeekdays is merging only rules that directly next to each other?

For example rules about Monday, Monday, Tuesday will be merged by this function. Rules about Monday, Tuesday, Monday will not be merged by this function and may be partially merged in the next step (if time is not overlapping).

https://github.com/westnordost/StreetComplete/blob/1177bf20f4f55449b3628bf1cb577a8aead6aad2/app/src/main/java/de/westnordost/streetcomplete/quests/opening_hours/adapter/OpeningHoursModelCreator.kt#L13

see matkoniecz@d6f6151 for some tests documenting this behavior

I plan to switch it to always merging rule in this step, if weekdays is the same - and loop over all entries looking for matches, rather just checking the last one.

I ask as OH have plenty of weird behavior, code had some parts that seemed to be unneeded complications and turned out to be necessary.

---

I finally had time today to do some SC coding and nearly eliminated OpeningMonths.

I started going through and removing also OpeningWeekdays but it ended with functions of signatures such as private fun List<List<OpeningWeekdaysRow>>.toWeekdaysClusters(): List<List<List<OpeningWeekdaysRow>>> what I do not see as an improvement (unfinished wip is here).

I plan to keep OpeningWeekdays without removing them, used solely in generating tag value of opening_hours.

It would allow to keep one depth of function within an object, ending with private fun List<OpeningWeekdays>.toWeekdaysClusters(): List<List<OpeningWeekdays>>, and keeping toWeekdaysClusters and toOpeningWeekdays unchanged (except change mentioned above and relocating this functions).

[westnordost]

Why toOpeningWeekdays is merging only rules that directly next to each other?
For example rules about Monday, Monday, Tuesday will be merged by this function. Rules about Monday, Tuesday, Monday will not be merged by this function and may be partially merged in the next step (if time is not overlapping).

Because the opening hours should be WYSISWG. If the user explicitly did not add them below each other, he will have had a reason for that and thus probably not want that the rules are optimized in the background. The reason could be that this is how the opening hours are written on the sign. Especially looking at resurveying these opening hours, it is easier if the opening hours are organized in the app like on the sign.

Note that the opening hours adapter omits the weekdays from displaying for every row with same weekdays except the first.

[westnordost]

I plan to keep OpeningWeekdays without removing them, used solely in generating tag value of opening_hours.

I don't understand this decision right now, but maybe later when I see the big picture.

[Marc-marc-marc]

could you document:

• what is the logic (timestamp of the metadata ? no change for this key ?) behind triggering a resurvey ?
• what is the key used to store the information that it was checked when nothing has changed ?
• in general, shouldn't we first ask if the object still exists? at the time when I was reading a lot of notes, there was a lot of "impossible to x, the POI has been closed for X years"

[Helium314]

Because the opening hours should be WYSISWG. If the user explicitly did not add them below each other, he will have had a reason for that and thus probably not want that the rules are optimized in the background. The reason could be that this is how the opening hours are written on the sign. Especially looking at resurveying these opening hours, it is easier if the opening hours are organized in the app like on the sign.

Just some comment on this:
a) this is not mentioned in the app. I had previously expected that opening hours are optimized in the background.
b) I am not willing to enter some (typically bank) opening hours like
Mon 9:00 AM – 12:30 PM 1:30 PM – 4:00 PM
Tue 9:00 AM – 12:30 PM 1:30 PM – 4:30 PM
Wed 9:00 AM – 12:30 PM 1:30 PM – 4:00 PM
Thu 9:00 AM – 12:30 PM 1:30 PM – 5:00 PM
Fri 9:00 AM – 12:30 PM 1:30 PM – 3:00 PM
day by day. Instead I just set Mon - Fri 9:00 AM – 12:30 PM and then do the rest (which admittedly looks ugly in the opening_hours tag, but takes long enough already)
c) if optimization should be implemented, some don't optimize button could be added

[westnordost]

So I will take this over in frame of the microgrant

[westnordost]

FYI I am currently working on it, but cherry-picked some changes of yours into an own branch because it was not clear what was already done and what not (many TODO comments) plus I want to do a few things differently (with resurvey-syntax, it's easier now, plus rather have all in one quest)

[westnordost]

Would you be available to review my implementation then? There is a lot of complexity while I want to reduce the likelihood of mistakes as much as possible.

[matkoniecz]

Would you be available to review my implementation then?

Yes, I would be happy to do this! Especially as I really want to see it finally implemented.

[westnordost]

Wonderful, I will post the first part later today. Teaser, I wrote a script which checks how many % of opening hours strings are supported. This is the output (for BoundingBox(50.0, 7.0, 55.0, 12.0)):

151770 opening hours, 96% parseable. Of these 65% are supported.

Of the unsupported opening hours, 
32% are with "off" or "closed" modifier
10% are "24/7"
6% collide with themselves (likely an error)
1% contains sunrise, sunset,...
0% contains no hours

You don't happen to have a list of all opening hours of the world?

[matkoniecz]

You don't happen to have a list of all opening hours of the world?

https://taginfo.openstreetmap.org/keys/opening_hours#values used to create matkoniecz@7245406 with list in https://raw.githubusercontent.com/matkoniecz/Zazolc/7245406f9538c9a880a0b8805df348f347f21baa/app/src/test/java/de/westnordost/streetcomplete/quests/opening_hours/all_oh_values.txt (though it is without popularity info)

[westnordost]

This is obsolete now, new PR #2045