Skip to content
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

Resurvey opening hours new #2045

Merged
merged 12 commits into from
Aug 29, 2020
Merged

Conversation

westnordost
Copy link
Member

Parsing and generating done.

Missing is the UI.

@matkoniecz

@westnordost
Copy link
Member Author

(Ignore any changes on quests/quest forms in this draft, that's still WIP)

@westnordost
Copy link
Member Author

westnordost commented Aug 23, 2020

1830500 opening hours, 97.7% parseable. Of these 71.6% are supported.

Of the unsupported opening hours, 
55.0% are "24/7"
28.4% are with "off", "closed" etc. modifier
10.8% collide with themselves (likely an error)
0.1% contain years
0.1% contain week numbers
0.9% contain multiple month ranges in same rule
0.7% contain unsupported holiday definitions
0.6% contain unsupported weekday ranges
2.3% contain time events like sunset, sunrise, dusk, ...
0.0% contain otherwise unsupported time ranges
2.5% only some rules are month based
2.3% contain unsupported date ranges
2.0% contain no hours
0.1% contain fallback rules
0.0% contain comments

Thanks @camelCaseNick for the SPARQL query, worked like a charm

@matkoniecz
Copy link
Member

matkoniecz commented Aug 23, 2020

For now I see nothing interesting, I will certainly look at it again - more throughly and experiment with code or test actual quests, not just look at it.

@westnordost
Copy link
Member Author

55.0% are "24/7"

It would be easy to support, but I think places with opening hours 24/7 shouldn't be asked again anyway, so no need to support it.

@westnordost
Copy link
Member Author

westnordost commented Aug 23, 2020

Added support for "off" times and more. New summary:

1830684 opening hours, 97.7% parseable. Of these 78.3% are supported.

Of the unsupported opening hours, 
72.0% are "24/7"
6.4% are unsupported rules with "off", "closed" etc. modifier
16.2% collide with themselves (likely an error)
0.1% contain years
0.1% contain week numbers
1.2% contain multiple month ranges in same rule
1.0% contain unsupported holiday definitions
0.8% contain unsupported weekday ranges
3.1% contain time events like sunset, sunrise, dusk, ...
0.0% contain otherwise unsupported time ranges
3.3% only some rules are month based
3.0% contain unsupported date ranges
2.7% contain no hours
0.1% contain fallback rules
0.0% contain comments

Considering the following:

  • New feature: Allow to not specify the weekdays for a rule (by unselecting all weekdays in the dialog) and thus current opening_hours that are defined this way can be displayed in the UI and maybe recorded better
  • New feature: Better integrated in the UI how to add hour ranges to an existing rule. Just tap on the button and select "Add hours". (Before, you had to select the identical weekdays than for the rule before)
  • New feature: Allow to specify off days (without comment) and thus rules that contain off days can be displayed in the UI
  • Rules that collide with another are likely but not necessarily mistakes. Since they are not necessarily mistakes, the should not be treated as invalid, thus not be displayed in the simple UI that StreetComplete offers. To validate if such a rule is OK would need someone that very well exists the OH specification anyway
  • The app will also ask for the opening hours of places where the opening_hours tag is not parseable by @simonpoole's opening hours parser (not even in non-strict mode) and display the form to the user as if nothing was specified before
  • The app should not ask again for the opening hours of places that were specified to be open 24/7 because it is very unlikely to change for the same shop

This results in about 2.5% of all opening hours unaccounted for. So, there is little need to extend the UI further to for example also support sunrise-sunset (-0.6%), Apr-Jun, Sep-Oct (-0.2%) and mix of month-based rules and non-month-based rules (-0.7%) even though all of these would fit okay into the current UI.

(calculation: 1-((1-0.977)+0.977*(0.783+(1-0.783)*(0.72+0.162))))

@westnordost
Copy link
Member Author

westnordost commented Aug 23, 2020

Ok I am actually in such a good flow right now, I implemented also Apr-Jun, Sep-Oct and "mix of month-based rules and non-month-based rules".

1830646 opening hours, 97.7% parseable. Of these 78.5% are supported.

Of the unsupported opening hours, 
72.7% are "24/7"
6.0% are unsupported rules with "off", "closed" etc. modifier
16.4% collide with themselves (likely an error)
0.1% contain years
0.1% contain week numbers
1.0% contain unsupported holiday definitions
0.8% contain unsupported weekday ranges
3.1% contain time events like sunset, sunrise, dusk, ...
0.0% contain otherwise unsupported time ranges
3.0% contain unsupported date ranges
2.7% contain no hours
0.1% contain fallback rules
0.0% contain comments

97.7% are now accounted for (2.3% unaccounted for).

@westnordost
Copy link
Member Author

westnordost commented Aug 23, 2020

Screenshot_1598226270

@camelCaseNick
Copy link

@westnordost It is not obvious to me how those should be read? Is it like the opening hours syntax — then it would be closed on every Sunday regardless of the month?

@westnordost
Copy link
Member Author

yes

@westnordost
Copy link
Member Author

But I find it is not good form to not specify the months if one uses months in another rule, this is why the ugly "(Not specified)" string is displayed there. Same with weekdays.

@westnordost westnordost marked this pull request as ready for review August 24, 2020 00:24
@westnordost
Copy link
Member Author

@camelCaseNick but anyway the displayed opening hours there are Kraut und Rüben anyway, it's just to show what rules it can understand. Generally, the rules can be understood as additive because this is intuitive, f.e.

Monday - Friday     10 - 12
Tuesday, Thursday   14 - 16

can be added like that into the form.

@matkoniecz should be done now

@camelCaseNick
Copy link

camelCaseNick commented Aug 24, 2020

[...] Generally, the rules can be understood as additive [...]

That depends in then opening hours syntax on whether a , or a ; is used. Normally you'd see Mo-Fr 10:00-12:00, Tu,Th 14:00-16:00, then it is open on a Tuesday at 13:00. With Mo-Fr 10:00-12:00; Tu,Th 14:00-16:00 it would be closed on a Tuesday at 13:00 as this rules are equivalent to Mo,We,Fr 10:00-12:00; Tu,Th 14:00-16:00 and to Mo,We,Fr 10:00-12:00, Tu,Th 14:00-16:00.

@westnordost
Copy link
Member Author

westnordost commented Aug 24, 2020

@camelCaseNick I know. All taken care of. The , separator is used if any weekdays collide, otherwise the ; (because more parsers understand it they tell me)

@westnordost
Copy link
Member Author

westnordost commented Aug 24, 2020

It would be a cool feature if you could drag and drop the "Add opening hours" button in-between the different rows to add an item there because there is currently no way to add rules in-between. Or, make it possible to drag the items to re-sort them, like in the quest selection screen.

Though, I won't do this now, maybe later. Or maybe someone else would like to try this out.

@smichel17
Copy link
Member

@westnordost I recognize you've just done a bunch of work so I have a little hesitation with sharing this, but: if you are seeking UI inspiration, please take a look at https://whenisgood.net's method of selecting times. (On desktop, since dragging does not work on mobile).

@westnordost
Copy link
Member Author

westnordost commented Aug 24, 2020 via email

@westnordost
Copy link
Member Author

I've taken a look on desktop. Interesting concept, but not really eligible for small screens and variable opening hours that may be by the minute as well. And even for big screens, it's not that fast. I find the interface of YoHours for example better here.

But anyway, I think the current UI is good, because it mimics how the opening hours plates often look like, suggesting that one inputs the hours just as they are visible on the sign.

@westnordost
Copy link
Member Author

Ready to merge from my side. Any pending reviews I should wait for?

@westnordost westnordost merged commit 8b2b7db into resurvey-new Aug 29, 2020
@westnordost westnordost deleted the resurvey-opening_hours-new branch August 31, 2020 16:55
@westnordost
Copy link
Member Author

westnordost commented Sep 21, 2020

Just for documentation...:

Screenshot_1600715682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants