GNOME Bugzilla – Bug 740836
Support localization of opening hours specification
Last modified: 2015-04-08 06:34:17 UTC
When showing the search bubble for an object and overpass provides a value for the tag "opening_hours" we currently show the raw tag value. This means that this string is not translated. The specification of the tag value format can be found here: http://wiki.openstreetmap.org/wiki/Key:opening_hours I think supporting the full specification in a way that allows for translations is probably hard to do and get right. I would propose supporting the most common, simple cases. Some examples could be: Specifying weekdays as intervals or single weekday with one time interval per "component": Mo-Fr 09:00-17:00; Sa 10:00-15:00 Specifying weekdays as lists: Mo,We,Th 09:00-17:00 Combinations of these: Mo,We-Fr 09:00-16:00 And supporting splitting the time range in two parts (common for when there is a lunch break): Mo-Fr 09:00-12:00,13:00-18:00 I think with some restrictions, like a maximum of i.e. 3 parts in the comma-separated weekday specification and maximum of two time interval parts, it might be possible to produce a nested "unfolding" using format strings that could be marked for translation. Also some special cases like "24/7" and possibly others could be added. I'll do some more thinking on this.
Created attachment 292569 [details] [review] Place: translate opening hours Parse the opening_hours tag from OSM to produce translatable strings. To make it manageable, it handles up to three different time specifications (those separated by ;), each day range can contain up to three comma-separated day lists of each either a single day or in the form start-end. For each component, the time specification list can contain one or two elements (each with start and end time, typically two are used i.e. when there is a lunch break). This is done to avoid doing a list iteration which could potentially be difficult to translate right in some cases. Furthermore, 24/7 and 00:00-24:00 are recognized as "around the clock". It also handles the special cases PH and SH for public and school holidays.
Created attachment 292587 [details] [review] Place: translate opening hours Parse the opening_hours tag from OSM to produce translatable strings. To make it manageable, it handles up to three different time specifications (those separated by ;), each day range can contain up to three comma-separated day lists of each either a single day or in the form start-end. For each component, the time specification list can contain one or two elements (each with start and end time, typically two are used i.e. when there is a lunch break). This is done to avoid doing a list iteration which could potentially be difficult to translate right in some cases. Furthermore, 24/7 and 00:00-24:00 are recognized as "around the clock". It also handles the special cases PH and SH for public and school holidays. Fixes a couple of bugs from previous patch.
Review of attachment 292587 [details] [review]: Thanks for doing this! It looks nice! But, holy wall of code in place.js, Batman! Maybe we want to move this (add the wheelchair translation) to somewhere else? Like translations.js or something? And make them static function and having the clock settings as a global there? ::: src/place.js @@ +162,3 @@ + } else { + /* split "components" */ + let splitted_parts = string.split(';'); For reaons beyond me, js people think camelCase is the prettiest way to do things. So this should probably be splitParts. @@ +167,3 @@ + case 1: + return this._translateOpeningHoursPart(splitted_parts[0].trim()); + case 2: { Is there a special reasong for the case n: { ... } syntax? I think we are using case n: ... break; else where in Maps. @@ +205,3 @@ + */ + _translateOpeningHoursPart: function(string) { + let splitted_string = string.split(" "); Same here and below. @@ +240,3 @@ + return this._translateOpeningHoursDayInterval(splitted_parts[0].trim()); + case 2: { + let day_interval1 = just interval1, interval2... I think. Or use camelCase. @@ +281,3 @@ + + /* + * Parse a day inteval consisting of either a single day interval.
Created attachment 292707 [details] [review] Place: translate opening hours Parse the opening_hours tag from OSM to produce translatable strings. To make it manageable, it handles up to three different time specifications (those separated by ;), each day range can contain up to three comma-separated day lists of each either a single day or in the form start-end. For each component, the time specification list can contain one or two elements (each with start and end time, typically two are used i.e. when there is a lunch break). This is done to avoid doing a list iteration which could potentially be difficult to translate right in some cases. Furthermore, 24/7 and 00:00-24:00 are recognized as "around the clock". It also handles the special cases PH and SH for public and school holidays. Fixes a couple of bugs from previous patch. Break out the translation functions to a separate file. Also handle times ending in :00 so that translations can opt to skip the minute part in that case.
The format supported by this patch is as follows: It handles up to three parts in the ;-separated string, such as: Mo-Fr 08:00-19:00 Mo-Fr 08:00-19:00; Sa 12:00-16:00 Mo-Fr 08:00-19:00; Sa 12:00-16:00; Su 12:00-15:00 Each part can consist of a day-range and a time part. The day range part can contain up to three ,-separated parts where each part is either a single day or an interval with a start and end day (such as "Mo-Fr"). In addition to the basic weekdays (Mo-Su) the special values PH and SH is supported for public and school holidays. The time range part can have one or two parts (,-separated) each of which is an interval with a start and end time in the hh:mm format. A special case is made in case the day part equals "Mo-Su" in which case "every day" is substituded. Furthermore, special cases is made for "24/7" and "Mo-Su 00:00-24:00" which is translated as "around clock" (open 24/7). In each case, where for example more components are found than what is supported by the parser, this value is returned in the plain form from the input.
Created attachment 292708 [details] [review] Place: translate opening hours Parse the opening_hours tag from OSM to produce translatable strings. To make it manageable, it handles up to three different time specifications (those separated by ;), each day range can contain up to three comma-separated day lists of each either a single day or in the form start-end. For each component, the time specification list can contain one or two elements (each with start and end time, typically two are used i.e. when there is a lunch break). This is done to avoid doing a list iteration which could potentially be difficult to translate right in some cases. Furthermore, 24/7 and 00:00-24:00 are recognized as "around the clock". It also handles the special cases PH and SH for public and school holidays. Fixes a couple of bugs from previous patch. Break out the translation functions to a separate file. Also handle times ending in :00 so that translations can opt to skip the minute part in that case. Fixed a typo.
(In reply to comment #3) > Review of attachment 292587 [details] [review]: > > Thanks for doing this! > > It looks nice! > > But, holy wall of code in place.js, Batman! > Maybe we want to move this (add the wheelchair translation) to somewhere else? > Like translations.js or something? > And make them static function and having the clock settings as a global there? > I broke out this code into a translationUtils.js I plan on moving the wheelchair code in a later commit. > ::: src/place.js > @@ +162,3 @@ > + } else { > + /* split "components" */ > + let splitted_parts = string.split(';'); > > For reaons beyond me, js people think camelCase is the prettiest way to do > things. So this should probably be splitParts. > Fixed > @@ +167,3 @@ > + case 1: > + return > this._translateOpeningHoursPart(splitted_parts[0].trim()); > + case 2: { > > Is there a special reasong for the case n: { ... } syntax? I think we are using > case n: ... break; else where in Maps. Fixed these > > @@ +205,3 @@ > + */ > + _translateOpeningHoursPart: function(string) { > + let splitted_string = string.split(" "); > > Same here and below. Fixed > > @@ +240,3 @@ > + return > this._translateOpeningHoursDayInterval(splitted_parts[0].trim()); > + case 2: { > + let day_interval1 = > > just interval1, interval2... I think. Or use camelCase. > Fixed. > @@ +281,3 @@ > + > + /* > + * Parse a day inteval consisting of either a single day > > interval. Fixed
Review of attachment 292708 [details] [review]: Thanks! Could you maybe include a screenshot of how the formatting of the tag looks with this? ::: src/translationUtils.js @@ +46,3 @@ + * Other variants, such as month-specific opening hours and other interval variants are + * not currently supported. In these cases, the data is returned as-is. + */ Maybe we can include a link to the OSM wiki for opening_hours in this comment? @@ +56,3 @@ + } else { + /* split "components" */ + let splittedParts = string.split(';'); nit: I think splitParts is more grammatically correct? @@ +321,3 @@ + let time = GLib.DateTime.new_local(1, 1, 1, h, min, 0.0); + + switch (clockFormat) { If there really only is two options "24h" or else... Maybe this should not be a switch construct but rather an if?
Created attachment 292771 [details] [review] Place: translate opening hours Parse the opening_hours tag from OSM to produce translatable strings. To make it manageable, it handles up to three different time specifications (those separated by ;), each day range can contain up to three comma-separated day lists of each either a single day or in the form start-end. For each component, the time specification list can contain one or two elements (each with start and end time, typically two are used i.e. when there is a lunch break). This is done to avoid doing a list iteration which could potentially be difficult to translate right in some cases. Furthermore, 24/7 and 00:00-24:00 are recognized as "around the clock". It also handles the special cases PH and SH for public and school holidays. Fixes a couple of bugs from previous patch. Break out the translation functions to a separate file. Also handle times ending in :00 so that translations can opt to skip the minute part in that case. Fixed a typo. Re-wrote a switch on clock format into an if-else. Changed the day parse function to pull the day abbreviations directly from the locale using GDateTime::format().
Some screenshots showing various cases of searches using this patch: These shows cases where the minute parts have been truncated away, and one one case with a full time spec. One case with a 24/7 time and one where the number of sub-parts is exceeded and the raw data value is displayed. http://www.update.uu.se/~ml/screenshots/1.png http://www.update.uu.se/~ml/screenshots/2.png http://www.update.uu.se/~ml/screenshots/3.png http://www.update.uu.se/~ml/screenshots/4.png http://www.update.uu.se/~ml/screenshots/5.png
(In reply to comment #8) > Review of attachment 292708 [details] [review]: > > Thanks! > > Could you maybe include a screenshot of how the formatting of the tag looks > with this? > > ::: src/translationUtils.js > @@ +46,3 @@ > + * Other variants, such as month-specific opening hours and other interval > variants are > + * not currently supported. In these cases, the data is returned as-is. > + */ > > Maybe we can include a link to the OSM wiki for opening_hours in this comment? > Good idea! Fixed. > @@ +56,3 @@ > + } else { > + /* split "components" */ > + let splittedParts = string.split(';'); > > nit: I think splitParts is more grammatically correct? Fixed. > > @@ +321,3 @@ > + let time = GLib.DateTime.new_local(1, 1, 1, h, min, 0.0); > + > + switch (clockFormat) { > > If there really only is two options "24h" or else... Maybe this should not be a > switch construct but rather an if? Changed this into an if-else. I also changed the day parse routine to pull the day abbreviations directly from the current locale using GDateTime as suggested by Leftmost on #i18n
Review of attachment 292771 [details] [review]: Thanks for this work! ::: src/translations.js @@ +62,3 @@ + let part1, part2, part3; + + switch (splittedParts.length) { Still some cases of 'splittedParts' here and below, should be 'splitParts' through out. @@ +85,3 @@ + * such as "Mo-Fr 10:00-19:00 Sa 10:00-17:00 Su 12:00-16:00" + * The space between the format placeholders could be substituded + * with the appropriate separator. And there is no way to get the appropriate separator from locale? @@ +359,3 @@ + */ + return time.format(C_("time format 12h", "%l\u2236%M\u2009%p")); + } Is there no way t oget these format strings from GDateTime or some other util directly? Can we look up the users locale in some way and decide?
Created attachment 292935 [details] [review] Place: translate opening hours Parse the opening_hours tag from OSM to produce translatable strings. To make it manageable, it handles up to three different time specifications (those separated by ;), each day range can contain up to three comma-separated day lists of each either a single day or in the form start-end. For each component, the time specification list can contain one or two elements (each with start and end time, typically two are used i.e. when there is a lunch break). This is done to avoid doing a list iteration which could potentially be difficult to translate right in some cases. Furthermore, 24/7 and 00:00-24:00 are recognized as "around the clock". It also handles the special cases PH and SH for public and school holidays. Fixes a couple of bugs from previous patch. Break out the translation functions to a separate file. Fixed a typo. Re-wrote a switch on clock format into an if-else. Changed the day parse function to pull the day abbreviations directly from the locale using g_date_time_format. Automaticall pull the time formatting from the locale with g_date_time_format
(In reply to comment #12) > Review of attachment 292771 [details] [review]: > > Thanks for this work! > > ::: src/translations.js > @@ +62,3 @@ > + let part1, part2, part3; > + > + switch (splittedParts.length) { > > Still some cases of 'splittedParts' here and below, should be 'splitParts' > through out. Fixed. > > @@ +85,3 @@ > + * such as "Mo-Fr 10:00-19:00 Sa 10:00-17:00 Su 12:00-16:00" > + * The space between the format placeholders could be > substituded > + * with the appropriate separator. > > And there is no way to get the appropriate separator from locale? > I haven't found any obvious way to do this automatically. > @@ +359,3 @@ > + */ > + return time.format(C_("time format 12h", > "%l\u2236%M\u2009%p")); > + } > > Is there no way t oget these format strings from GDateTime or some other util > directly? Can we look up the users locale in some way and decide? Yes, changed this to use "%R" or "%r" with g_date_time_format (depending on clock format) and skipped the :00 special casing.
Review of attachment 292935 [details] [review]: Thanks! Some comments below. ::: src/translations.js @@ +28,3 @@ + +// in org.gnome.desktop.interface +const CLOCK_FORMAT_KEY = 'clock-format'; const CLOCK_FORMAT_KEY = 'clock-format'; @@ +56,3 @@ + return _("around the clock"); + } else if (string === "sunrise-sunset") { + return _("from sunrise to sunset"); Please use ' ' style quotes when it is not used for gettext translation @@ +101,3 @@ + */ +function _translateOpeningHoursPart(string) { + let splitString = string.split(" "); ' ' @@ +130,3 @@ + */ +function _translateOpeningHoursDayIntervalList(string) { + let splitParts = string.split(","); ',' @@ +176,3 @@ + */ +function _translateOpeningHoursDayInterval(string) { + let splittedString = string.split("-"); splitString @@ +234,3 @@ + // unknown value, just return the input + return string; + } Do we want a loop? if (string === 'PH') return _("public holidays"); if (string === 'SH') return _("school holidays"); let days = ['Mo', 'Tu', 'We', 'Th', 'Fr', 'Sa', 'Su']; for (let i = 0; i < days.length; i++) { if (string === days[i]) { time = time.add_days(i); return time.format('%a'); } } @@ +243,3 @@ + */ +function _translateOpeningHoursTimeIntervalList(string) { + let splittedString = string.split(","); splitString @@ +277,3 @@ + } + + let splittedString = string.split("-"); splitString @@ +303,3 @@ + */ +function _translateOpeningHoursTime(string) { + let splittedString = string.split(":"); ':' @@ +328,3 @@ + let time = GLib.DateTime.new_local(1, 1, 1, h, min, 0.0); + + if (clockFormat === "24h") { '24h' @@ +329,3 @@ + + if (clockFormat === "24h") { + return time.format("%R"); '%R' @@ +331,3 @@ + return time.format("%R"); + } else { + return time.format("%r"); '%r' @@ +332,3 @@ + } else { + return time.format("%r"); + } For these ifs, and elsewhere in this patch, if there is only one statement, braces aren't needed. You could move the comment above the if, and omit the braces. Maybe we can get the size of this code down a bit :)
Created attachment 293006 [details] [review] Place: translate opening hours Parse the opening_hours tag from OSM to produce translatable strings. To make it manageable, it handles up to three different time specifications (those separated by ;), each day range can contain up to three comma-separated day lists of each either a single day or in the form start-end. For each component, the time specification list can contain one or two elements (each with start and end time, typically two are used i.e. when there is a lunch break). This is done to avoid doing a list iteration which could potentially be difficult to translate right in some cases. Furthermore, 24/7 and 00:00-24:00 are recognized as "around the clock". It also handles the special cases PH and SH for public and school holidays. Fixes a couple of bugs from previous patch. Break out the translation functions to a separate file. Fixed a typo. Re-wrote a switch on clock format into an if-else. Changed the day parse function to pull the day abbreviations directly from the locale using g_date_time_format. Automaticall pull the time formatting from the locale with g_date_time_format
(In reply to comment #15) > Review of attachment 292935 [details] [review]: > > Thanks! > > Some comments below. > > ::: src/translations.js > @@ +28,3 @@ > + > +// in org.gnome.desktop.interface > +const CLOCK_FORMAT_KEY = 'clock-format'; > > const CLOCK_FORMAT_KEY = 'clock-format'; Fixed. > > @@ +56,3 @@ > + return _("around the clock"); > + } else if (string === "sunrise-sunset") { > + return _("from sunrise to sunset"); > > Please use ' ' style quotes when it is not used for gettext translation > Changed all non-gettext string constants to use single quotes. > @@ +101,3 @@ > + */ > +function _translateOpeningHoursPart(string) { > + let splitString = string.split(" "); > > ' ' > > @@ +130,3 @@ > + */ > +function _translateOpeningHoursDayIntervalList(string) { > + let splitParts = string.split(","); > > ',' > > @@ +176,3 @@ > + */ > +function _translateOpeningHoursDayInterval(string) { > + let splittedString = string.split("-"); > > splitString Changed remaining splittedString -> splitString > > @@ +234,3 @@ > + // unknown value, just return the input > + return string; > + } > > Do we want a loop? > > if (string === 'PH') > return _("public holidays"); > > if (string === 'SH') > return _("school holidays"); > > let days = ['Mo', 'Tu', 'We', 'Th', 'Fr', 'Sa', 'Su']; > > for (let i = 0; i < days.length; i++) { > if (string === days[i]) { > time = time.add_days(i); > return time.format('%a'); > } > } > Yes, it makes the a bit tighter, changed this. > @@ +243,3 @@ > + */ > +function _translateOpeningHoursTimeIntervalList(string) { > + let splittedString = string.split(","); > > splitString > > @@ +277,3 @@ > + } > + > + let splittedString = string.split("-"); > > splitString > > @@ +303,3 @@ > + */ > +function _translateOpeningHoursTime(string) { > + let splittedString = string.split(":"); > > ':' > > @@ +328,3 @@ > + let time = GLib.DateTime.new_local(1, 1, 1, h, min, 0.0); > + > + if (clockFormat === "24h") { > > '24h' > > @@ +329,3 @@ > + > + if (clockFormat === "24h") { > + return time.format("%R"); > > '%R' > > @@ +331,3 @@ > + return time.format("%R"); > + } else { > + return time.format("%r"); > > '%r' > > @@ +332,3 @@ > + } else { > + return time.format("%r"); > + } > > For these ifs, and elsewhere in this patch, if there is only one statement, > braces aren't needed. You could move the comment above the if, and omit the > braces. Maybe we can get the size of this code down a bit :) Changed one-line free-standing instances of ifs to drop the braces.
Review of attachment 293006 [details] [review]: Thanks! It is starting to look good from a code perspective. I would really like some translators comments on this to see that we are doing stuff as we should :) Maybe a mail to the i18n-list is in order?
Not very sure about how the strings will appear in the GUI. Could you please give us an example? If would be really helpful for us (translators) to know if there is something wrong in this side. Thanks!
(In reply to comment #19) > Not very sure about how the strings will appear in the GUI. Could you please > give us an example? If would be really helpful for us (translators) to know if > there is something wrong in this side. > > Thanks! There are some screenshots in comment 10 (https://bugzilla.gnome.org/show_bug.cgi?id=740836#c10) does that help?
(In reply to comment #20) > (In reply to comment #19) > > Not very sure about how the strings will appear in the GUI. Could you please > > give us an example? If would be really helpful for us (translators) to know if > > there is something wrong in this side. > > > > Thanks! > > There are some screenshots in comment 10 > (https://bugzilla.gnome.org/show_bug.cgi?id=740836#c10) does that help? Sure! I missed them, sorry. Seems ok for me. Maybe you'd like to hear other translator's opinnion, but I guess there will be no problem. Just a note: in the future, try to upload screenshots with the software in English, so things will be more clear. Not a big problem in this case, but take care of it next time ;-) Thanks for the quick response!
Review of attachment 293006 [details] [review]: Thanks! Pushed this with minor indent/line break/spelling fixes. Lets see if the translators find any issues with it while translating.
Patch was commited some time ago, and several translations have been commited without (apparently) any problem, so I think bug can be closed. Thanks!