organicmaps/organicmaps-tmp

Archived

Fork 1

[editor] Fix parsing of 'off' opening hours #7652

Merged

root merged 2 commits from openhours into master

2024-05-19 21:32:16 +00:00

gpesquero commented

2024-03-22 00:37:05 +00:00

Member

This PR fixes the parsing of the "opening hours" strings that start with an "off" statement.

One example of opening hours not being processed correctly is the opening_hours tag Mo off; Tu-Su 12:00-16:30, 19:00-24:00 of this restaurant in OSM.

Current opening hours parsing implementation does not like "off" statements if they are located in the first place, so this PR reorders the statements by placing the "off" ones at the end of the list.

This PR closes PR 1612

This PR fixes the parsing of the "opening hours" strings that start with an "off" statement. One example of opening hours not being processed correctly is the opening_hours tag `Mo off; Tu-Su 12:00-16:30, 19:00-24:00` of [this restaurant in OSM](https://www.openstreetmap.org/node/9592619990). Current opening hours parsing implementation does not like "off" statements if they are located in the first place, so this PR reorders the statements by placing the "off" ones at the end of the list. This PR closes [PR 1612](https://git.omaps.dev/organicmaps/organicmaps/issues/1612)

👍 2 🎉 5

biodranik (Migrated from github.com) reviewed 2024-03-22 19:09:47 +00:00

editor/ui2oh.cpp

biodranik (Migrated from github.com) commented

2024-03-22 19:09:47 +00:00

Why closed rules are not accepted?

Is it possible to convert "Mo off" text to the whole day closed rule?

Why closed rules are not accepted? Is it possible to convert "Mo off" text to the whole day closed rule?

gpesquero reviewed 2024-03-22 21:31:14 +00:00

editor/ui2oh.cpp

gpesquero commented

2024-03-22 21:31:14 +00:00

Author

Member

I haven't got into all the details of the opening hours (OH) parsing code, but here are my toughs/findings of what the problem might be:

The piece of code that rejects "Close" rules if they are at the start of the OH sequences is this one:

7cf0b2c2a6/editor/ui2oh.cpp (L341-L345)

I presume that the OH parsing code starts with an empty OH sequence, then expects to have one or several opening hours rules and finally a list of "off hours" rules to subtract them from the previous opening hours rules That's why an "off" rule in the first place is useless and then the parser returns false.

For example, the sequence Mo off; 9:00-17:00 any human would read it as: "On Mondays it's closed, and (for the rest of the days) it's open from 9 to 5".

But the OH parser does not like making these "assumptions", and expects to have first the actual opening hours, and later the "off" hours to subtract them: "9:00-17:00; Mo off" (open all days from 9 to 5, except for Mondays").

One way to avoid this problem is to move all the "off" rules at the end of the sequence, just what I've proposed in this PR.

I haven't got into all the details of the opening hours (OH) parsing code, but here are my toughs/findings of what the problem might be: The piece of code that rejects "Close" rules if they are at the start of the OH sequences is this one: https://github.com/organicmaps/organicmaps/blob/7cf0b2c2a6b9146214b455c1b7c72f2b2c0e2d13/editor/ui2oh.cpp#L341-L345 I _presume_ that the OH parsing code starts with an empty OH sequence, then expects to have one or several opening hours rules and finally a list of "off hours" rules to subtract them from the previous opening hours rules That's why an "off" rule in the first place is useless and then the parser returns false. For example, the sequence `Mo off; 9:00-17:00` any human would read it as: "On Mondays it's closed, and (for the rest of the days) it's open from 9 to 5". But the OH parser does not like making these "assumptions", and expects to have first the actual opening hours, and later the "off" hours to subtract them: "9:00-17:00; Mo off" (open all days from 9 to 5, except for Mondays"). One way to avoid this problem is to move all the "off" rules at the end of the sequence, just what I've proposed in this PR.

biodranik (Migrated from github.com) reviewed 2024-03-22 22:08:35 +00:00

editor/ui2oh.cpp

biodranik (Migrated from github.com) commented

2024-03-22 22:08:35 +00:00

Thanks for the details!

OSM spec says in "Syntax for optional modifiers" section:

closed or off: closed at the indicated dates and times (or permanently if no time is given), except at overriding dates and times that follow;

So using the off modifier in the beginning is perfectly fine.

So maybe the author of the following code/comment was wrong, and removing that check (and maybe fixing other code if necessary) will provide a better fix?

    if (rulePart.GetModifier() == osmoh::RuleSequence::Modifier::Closed)
    {
      // off modifier in the first part in oh is useless.
      if (first == true)
        return false;

Thanks for the details! [OSM spec](https://wiki.openstreetmap.org/wiki/Key:opening_hours) says in "Syntax for optional modifiers" section: > closed or off: closed at the indicated dates and times (or permanently if no time is given), except at overriding dates and times that follow; So using the off modifier in the beginning is perfectly fine. So maybe the author of the following code/comment was wrong, and removing that check (and maybe fixing other code if necessary) will provide a better fix? ``` if (rulePart.GetModifier() == osmoh::RuleSequence::Modifier::Closed) { // off modifier in the first part in oh is useless. if (first == true) return false; ```

vng (Migrated from github.com) reviewed 2024-03-24 19:49:36 +00:00

vng (Migrated from github.com) left a comment

Hm, is it only editor problem?
I mean basic routing like OpeningHours.IsOpen/IsClosed works fine with off?
If not, we should fix there first ..

Hm, is it only editor problem? I mean basic routing like ```OpeningHours.IsOpen/IsClosed``` works fine with _off_? If not, we should fix there first ..

biodranik (Migrated from github.com) approved these changes 2024-05-19 21:31:50 +00:00

biodranik (Migrated from github.com) left a comment

Thanks, let's start with this fix to slightly improve the current situation.

editor/editor_tests/ui2oh_test.cpp

biodranik (Migrated from github.com) commented

2024-05-19 21:31:08 +00:00

    TEST_EQUAL(tts.GetUnhandledDays(), OpeningDays({osmoh::Weekday::Monday}), ());

```suggestion TEST_EQUAL(tts.GetUnhandledDays(), OpeningDays({osmoh::Weekday::Monday}), ()); ```

This repo is archived. You cannot comment on pull requests.

No reviewers

biodranik

No labels

Android Automotive (AAOS)

API

AppGallery

AppStore

Battery and Performance

External Map Datasets

F-Droid

Fonts

Frequently User Reported

No milestone

No project

No assignees

2 participants

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#7652

No description provided.

Rows
Columns