mockturtl / cinnamon-weather

DEPRECATED. Use linuxmint/cinnamon-spices-applets instead.
https://cinnamon-spices.linuxmint.com/applets/view/17
28 stars 35 forks source link

24-hour time display not working OK #162

Closed spekulatius-fb closed 8 years ago

spekulatius-fb commented 8 years ago

24-hour time display not working correctly on debian jessie

e.g. 9:33pm is displayed as 12:33

To correct this issue: change the line return (parseInt(hh) + 12).toString() +':' + mm to return (parseInt(hh,10) + 12).toString() +':' + mm in "weather@mockturtl/applet.js":

, convertTo24: function convertTo24(timeStr) { let s = timeStr.indexOf(':') let t = timeStr.indexOf(' ') let n = timeStr.length let hh = timeStr.substr(0, s) let mm = timeStr.substring(s + 1, t)

if (parseInt(hh) < 10) // pad
  hh = '0' + hh

let beforeNoon = timeStr.substr(n - 2).toLowerCase() == 'am'
if (beforeNoon) {
  if (hh == '12') // 12 AM -> 00
    hh = '00'

  return hh + ':' + mm
}

if (hh == '12') // 12 PM -> ok
  return hh + ':' + mm

 return (parseInt(hh,10) + 12).toString() +':' + mm

}

mockturtl commented 8 years ago

Hi @spekulatius-fb, please provide

Thanks!

mockturtl commented 8 years ago

If I understand correctly, cjs uses spidermonkey, and its parseInt does use base 10 by default. (docs)

# run the interpreter in a terminal; requires `libmozjs-24-bin`
$ js24 
js> parseInt('09')
9
js> parseInt('09', 10)
9

In addition, the applet formats 24-hour time correctly for me (mint betsy/debian jessie). I wonder if localization might make a difference. (en_US here)

spekulatius-fb commented 8 years ago

Hi @mockturtl

I use a debian jessie 64bit

cinnamon: Installiert: 2.2.16-5

Weather 1.13.2

locale

LANG=de_DE.UTF-8 LANGUAGE= LC_CTYPE="de_DE.UTF-8" LC_NUMERIC="de_DE.UTF-8" LC_TIME="de_DE.UTF-8" LC_COLLATE="de_DE.UTF-8" LC_MONETARY="de_DE.UTF-8" LC_MESSAGES="de_DE.UTF-8" LC_PAPER="de_DE.UTF-8" LC_NAME="de_DE.UTF-8" LC_ADDRESS="de_DE.UTF-8" LC_TELEPHONE="de_DE.UTF-8" LC_MEASUREMENT="de_DE.UTF-8" LC_IDENTIFICATION="de_DE.UTF-8" LC_ALL=

I attached 2 screen shots without modification: weather_orig

with modification: weather_modified

In the parseInt doc I found: If the input string begins with "0", radix is eight (octal) or 10 (decimal). Exactly which radix is chosen is implementation-dependent. ECMAScript 5 specifies that 10 (decimal) is used, but not all browsers support this yet. For this reason always specify a radix when using parseInt.

mockturtl commented 8 years ago

In the parseInt doc I found:

My understanding is that Cinnamon uses the Mozilla JS engine (libmozjs-24), where parseInt has correctly defaulted to decimal since version 21.

So the behavior you see is a little mysterious to me. Can you test the command-line interpreter, to see whether parseInt treats a 0-padded string with no radix as decimal or octal?

cinnamon: 2.2.16-5

I have 3.0.6+betsy. I wonder how differently Debian packages Cinnamon.

Is cjs installed? (apt-cache policy cjs) Does it use libmozjs-24? (apt-cache depends cjs) Ah! Jessie's cjs uses 1.8.5 (circa Firefox 4). That explains it.

I wonder why they haven't updated.

Meanwhile, thank you for the report and the fix!

Can you confirm this is the only change needed?

@@ -629,7 +629,7 @@
     if (hh == '12') // 12 PM -> ok
       return hh + ':' + mm

-    return (parseInt(hh) + 12).toString() + ':' + mm
+    return (parseInt(hh, 10) + 12).toString() + ':' + mm
   }

 , destroyCurrentWeather: function destroyCurrentWeather() {
spekulatius-fb commented 8 years ago

Can you confirm this is the only change needed?

Yes I can confirm the patch works for my system.

I wonder why they haven't updated.

in jessie-backports repository cinnamon got upgraded already

cinnamon:
  Installiert:           3.0.4-2~bpo8+1
  Installationskandidat: 3.0.4-2~bpo8+1
  Versionstabelle:
 *** 3.0.4-2~bpo8+1 0
        600 http://ftp.de.debian.org/debian/ jessie-backports/main amd64 Packages
        100 /var/lib/dpkg/status
     2.2.16-5 0
        500 http://ftp.de.debian.org/debian/ jessie/main amd64 Packages

I upgraded one of my systems to Cinnamon 3.0.4 and the 24h time display works (as expected) out of the box.

So for standard debian-jessie installations the patch seems to be necessary but for more actual systems it is not required.

Meanwhile, thank you for the report and the fix!

Welcome - and thank you for the applet ;)