overview does not work properly

[nest1ing]

After the commit 569ccf66e58aacd50eaf53518c50c0ad4d448333 the overview page looks like this:

As I find out, it happens because in my case the value of Date().toString() contains national characters in the time zone.

new Date().toString()
"Thu Nov 15 2018 14:54:55 GMT+0300 (Москва, стандартное время)"

The events log page have the same behavior.

[nest1ing]

Maybe would better use toUTCString() / toLocaleString() instead parse the output of toString() to get the name of time zone? In my case it looks like this:

new Date().toUTCString()
"Thu, 15 Nov 2018 12:09:10 GMT"
new Date().toLocaleString()
"15.11.2018, 15:09:20"

But I am not sure how it may be implemented in the template parser. The template looks like this:

{{(tmz == 'UTC' ? (event.Timestamp | date:'medium': tmz) : (event.Timestamp | date:'medium')) + ' ' + tmz}}

[nest1ing]

Fix me if I wrong, but I believe that this code, even if it successfully parsed string, will give the time zone of the browser instead of the BMC:

              // https://stackoverflow.com/questions/1091372/getting-the-clients-timezone-in-javascript
              // EDT (UTC - 04:00)
              $scope.bmc.timezone =
                  $scope.bmc.date.toString().match(/\(([A-Za-z\s].*)\)/)[1] +
                  ' ' + createOffset($scope.bmc.date);

same for the host:

              $scope.host.timezone =
                  $scope.host.date.toString().match(/([A-Z]+[\+-][0-9]+.*)/)[1];

[gtmills]

Cherry picked https://gerrit.openbmc-project.xyz/#/c/openbmc/phosphor-webui/+/15894/2/ Without 15894/2/ :

With 15894/2/ :

Lose the timezone for BMC time and the event log..

[nest1ing]

I've fixed it.

And @gtmills, @BeccaBroek: I want to get your attention to the my previous question. As I understood, in this case we want to use the time zone of the user's browser. I specifically left both of these methods (see app/configuration/controllers/date-time-controller.js), but I believe we should use only one of them.

In my browser it looks like this:

[gtmills]

Thoughts on https://gerrit.openbmc-project.xyz/#/c/openbmc/phosphor-webui/+/15972 ?

Overview page with 15972:

The IBM design team prefers this format over the "11/19/2018, 3:38:48 PM CST" format from 15894/3/ due to confusion with mm/dd and dd/mm

Datetime page with 15972:

This format is preferred by the IBM design team over the "America/Chicago" format on 15894/3/ due to the "Central Standard Time (UTC-06:00)" format being more common.

The UTC and user times are the same format with this patch.

[AlexanderAmelkin]

Speaking of formats I would go with YYYY/MM/DD for date, a standard timezone abbreviation (e.g. PST, CST, MSK, GMT, etc.) that every admin around the world is used to, and the time in locale-specific format (e.g. "11:59 PM" or "23:59").

Indeed, everybody always gets confused over mm/dd and dd/mm in formats with trailing yyyy, but there is just a single format with year first, and that is YYYY/MM/DD. Correct me if I'm wrong, but I've never seen YYYY/DD/MM used anywhere by anyone. Thus, if we put year first, we give user a hint that it is the month that follows and is followed by the day.

Any verbose natural language name for time zone poses a risk of not fitting into visual design, and of breaking language grammar rules. That may be not obvious for people speaking case-less languages like English where nouns and adjectives never change, but for languages like Russian it makes a big difference grammar-wise whether you say "Екатеринбургское время" (nominative case) or "Екатеринбургского времени" (genitive case) or "Екатеринбургскому времени" (prepositional case), and the required case differs based on context. Hence, writing "YEKT" or "MSK" or "CST" is much safer.

Representation of time though doesn't suffer from any of those problems and hence can safely use locale settings. Moreover, it is totally unacceptable to use 'am/pm' notation for at least Russia and all the ex-USSR countries, as nobody here gets the concept of 12:00pm following 11:59am. and as far as I'm aware, US citizens are also uncomfortable with 24h time format calling it 'military' (I would also be uncomfortable if I pronounced it in hundreds). Hence the proposal to use locale settings for this part.

[nest1ing]

It is the reason of the opening this issue. The browser with Russian locale give Thu Nov 15 2018 14:54:55 GMT+0300 (Москва, стандартное время) instead the awaited Thu Nov 15 2018 14:54:55 GMT+0300 (MSK). And I was unable to find any methods to get MSK without external dependencies.

And I checked it in different browsers, it is independent.

[AlexanderAmelkin]

It appears that timezone support is crippled in ECMAscript and differs from browser to browser. The only compatible method I could find is using moment-timezone.js. This code gives me 'MSK' in any browser using any browser i18n and with any system locale, provided that my computer is indeed in MSK timezone:

<script src="http://momentjs.com/downloads/moment-with-locales.min.js" language="Javascript"></script>
<script src="http://momentjs.com/downloads/moment-timezone-with-data-2012-2022.min.js" language="Javascript"></script>
...
function getUserTimezone(date) {
        const ro = Intl.DateTimeFormat().resolvedOptions();
        return moment.tz(date, ro.timeZone).format('z');
}

I guess we could add momentjs to phosphor-webui. It would take extra 76KB though.

[BeccaBroek]

Fix me if I wrong, but I believe that this code, even if it successfully parsed string, will give the time zone of the browser instead of the BMC:

              // https://stackoverflow.com/questions/1091372/getting-the-clients-timezone-in-javascript
              // EDT (UTC - 04:00)
              $scope.bmc.timezone =
                  $scope.bmc.date.toString().match(/\(([A-Za-z\s].*)\)/)[1] +
                  ' ' + createOffset($scope.bmc.date);

same for the host:

              $scope.host.timezone =
                  $scope.host.date.toString().match(/([A-Z]+[\+-][0-9]+.*)/)[1];

You're correct. We get the BMC and host times as an epoch from the back end, so I don't believe there's any way for our GUI to display the timezone of the of the host (although maybe that variable naming is unclear!). You're also correct that the BMC and HOST timezones should be consistent when the time owner is split. This was something I overlooked in my previous commit and it should be corrected.

[BeccaBroek]

@AlexanderAmelkin I also was looking into moment-timezone.js at one point because it would make managing date/time and timezone across browsers a lot more simple. I thought I had come up with a solution that avoided bringing in an external library, but clearly my commit is breaking these pages in certain timezones. I do have a branch locally from when I was working on this in which I had brought in moment-timezone that I could push up for review if the 76KB is not a concern. @gtmills @edtanous, any thoughts on bringing this library into the GUI?

[gtmills]

@AlexanderAmelkin Where does this 76KB for momentjs come from? Is this 76KB compressed ?

[gtmills]

I would prefer not bringing in momentjs but if it is needed, I understand.

[edtanous]

If moment.js is truly 76k compressed and minified (which I'd be really surprised of), I'm going to draw a line in the sand and say that we can't afford it, if this is the only thing that it fixes. If we start using it for all dates, it gets closer to worthwhile, but at 76k, we're talking about something that's bigger than angularjs. Right now the entire webui compresses down to ~400k after the 8 gagillion compression tricks we use. Adding 76K to fix a date feels like a backtrack. If this is truly the case, there are some options. Implementing "tree shaking" https://webpack.js.org/guides/tree-shaking/ Should strip momentjs down to only the parts we use, which, if we're only formatting a date, should be a function or two, which could make it able to fit. I'm not sure if anyone is interested in looking into tree shaking, but it could be worthwhile.

Alternatively, has anyone considered just fixing the regex that's causing the issue? I probably should've caught this in code review, but the regex here: https://github.com/openbmc/phosphor-webui/blob/1c43b312769fcf9fe4e41bf6d7336402d06bed6e/app/overview/controllers/system-overview-controller.js#L163 Doesn't look right to handle this case

new Date().toString().match(/(([A-Za-z\s].*))/)[1]; looking deeper, that regex could be a bit simpler. I'm pretty sure it could just be

\((.*)\)$

That, and an appropriate range check on the match groups before using should avoid this issue.

That seems to match the timezone correctly regex tester. https://regex101.com/r/j6hUbY/1

[nest1ing]

Alternatively, has anyone considered just fixing the regex that's causing the issue?

Yes, it was first what I tried. But it gives the name of the time zone in Russian and it looks strange.

[AlexanderAmelkin]

@edtanous, yes it's 76k compressed when bundled "with data" and "with locales" as in my example above. The code itself is quite small.

Fixing regexp doesn't work because it will capture "Москва, стандартное время" instead of "MSK" if Firefox has Russian localization installed. The only solution I was able to find that would always give "MSK" (not "Europe/Moscow", not "Москва, стандартное время", not "GMT+3:00", etc.) regardless of user's browser settings was momentjs. I realize and agree that bringing in 76k of compressed and minified javascript is a bit of overkill for just this task, but I couldn't find anything better. There is no standard API for that, unfortunately.

[AlexanderAmelkin]

If we drop locales support (and I guess we can do so since we only want momentjs to give us timezone abbreviation in en locale), then it will be like 41k compressed. Most of the space is occupied by timezone data.

[nest1ing]

In my opinion, we may accept that the browser doesn't know the abbreviation of the Moscow time zone and it will be shown as GMT+3:

But there still is one strange thing: at data/time settings page it looks like this:

[nest1ing]

I guess we could add momentjs to phosphor-webui. It would take extra 76KB though.

If we drop locales support ... then it will be like 41k compressed.

I guess you tried non minified versions and/or compression with default level. I download it, then run gzip -9 and now it looks like this:

$ ls -l 
total 32
-rw-rw-r-- 1 shurik shurik 16755 Oct 29 06:13 moment.min.js.gz
-rw-rw-r-- 1 shurik shurik  9177 Oct 29 06:13 moment-timezone-with-data-2012-2022.min.js.gz

[gtmills]

@nest1ing That seems more reasonable. It might be worth using at that size.

[AlexanderAmelkin]

@gtmills, good. Then let's do it. Let's draw the bottom line:

1. Import minified momentjs without locales support as we only need it for en-based timezone abbreviations
2. Import minified moment-timezone with data 2012-2022
3. Change dates format to: '<YYYY/MM/DD> ', e.g. 2018/11/30 15:27 MSK or 2018/11/30 6:27 AM CST.

As I said before, I believe the YYYY/MM/DD format is the most compact, language-neutral, unambiguous and text sorting friendly. Time must be represented in locale-specific way because nobody outside the English-speaking world understands the AM/PM format, and the English-speaking world doesn't like the 24-hour format.

Do you agree?

[gtmills]

The IBM design team is not a fan of YYYY/MM/DD. One comment was "spelling out the month is helpful and how it's done on a lot of modern UIs". @susantjasinski Can weigh in more here. It appears the angularjs date filter does not work as it should. Sorry for leading us down that path. I see PM posted in the example above (I believe this is incorrect for the locale).

Any way we can still make toLocaleTimeString() work? What about this:

var event = new Date(Date.UTC(2012, 11, 20, 3, 0, 0));
var options = { year: 'numeric', month: 'short', day: 'long', day: 'numeric', hour: 'numeric', minute: 'numeric', second: 'numeric', timeZoneName: 'short' };

// British English output: "19 Dec 2012, 21:00:00 GMT-6"
console.log(event.toLocaleString('en-GB', options ));

//United Statues English output: "Dec 19, 2012, 9:00:00 PM CST"
console.log(event.toLocaleString('en-US', options));

// Arabic output:  ٩:٠٠:٠٠ م غرينتش-٦" 
console.log(event.toLocaleString('ar-EG', options));

// German(Germany) output: "19. Dez. 2012, 21:00:00 GMT-6"
console.log(event.toLocaleString('de-DE', options));

// Russian output: "19 дек. 2012 г., 21:00:00 GMT-6"
console.log(event.toLocaleString('RU', options));

The timezone would still display as GMT +/- x. Is that okay? The date/time would be in the locale language is that okay?

The actual code would look like:

var ro = Intl.DateTimeFormat().resolvedOptions();
var options = { year: 'numeric', month: 'short', day: 'long', day: 'numeric', hour: 'numeric', minute: 'numeric', second: 'numeric', timeZoneName: 'short' };
console.log(event.toLocaleString(ro.lang, options ));

[edtanous]

All of the compression information above has been presented so far are based on either grabbing the pre-minified versions of the library, or gzipping things manually. Given that we already have infrastructure to minify and gzip as part of the build process, I would like to see numbers from the current build process (preferably in the form of a patch submitted to gerrit) before we pass judgement on whether or not the size increase is acceptable. Said another way, I care about how big it is as deployed in the current environment. All other measurements (manually minifying, manually gzipping, pre-minified, xz compressed) are unimportant to this discussion.

If we want to change how the minification and compression works, that is another topic of discussion, and should be had in a different issue, don't think anyone is advocating for that.

@AlexanderAmelkin For #1 above. We will not import the minified version. We will use the minification infrastructure that exists in the build already, as it allows selectable debug builds, doesn't check in any libraries directly, and makes us more immune to the minification injection attacks that seem to be plaguing npm as of late.

2 Agree, the time limited package seems like the best bet. Most of the systems we're working on will be long out of support window by the time 2022 rolls around.

@nest1ing: "looks strange" is much better than a crash. While I agree, it's not perfect, I think we should fix the regex anyway just to get the UI working for Russian Locales, then work to come up with a "best" solution as we figure out the minification and size issues on momentjs.

@gtmills "spelling out the month is helpful and how it's done on a lot of modern UIs" My opinion here isn't very strong in either direction. With numbers, I like the ability find the duration between event times quickly, on the other hand, spelling out the month is easier to read if looking in the context of a single event. I"m fine with whatever solution you guys come up with.

[nest1ing]

@edtanous: You are right, when momentjs was installed by the npm install command it increased used space for around 75K, and I agree - it is too many.

[AlexanderAmelkin]

@BeccaBroek, can you check how much space does momentjs occupy for you in the final image being brought in without pre-minification as @edtanous suggested. Our quick check gave us 75k which is very strange provided we got just about 25k from the pre-minified and manually compressed versions.

[gtmills]

I had a few comments on https://gerrit.openbmc-project.xyz/#/c/openbmc/phosphor-webui/+/15894/ but why not go with that patch instead of adding momentjs ?

[nest1ing]

I don't see reasons to make a patch for using the momentjs. As I wrote earlier, it used too much space.

$ npm run-script build
$ du -bs dist
295253  dist/

Then I run commands:

$ npm install moment --save
$ npm install moment-timezone-with-data-2010-2020 --save

and modify app/common/filters/index.js:

diff --git a/app/common/filters/index.js b/app/common/filters/index.js
index c760158..59fcf66 100644
--- a/app/common/filters/index.js
+++ b/app/common/filters/index.js
@@ -1,3 +1,6 @@
+import moment from 'moment';
+import moment_timezone from 'moment-timezone-with-data-2010-2020';
+
 window.angular && (function(angular) {
   'use strict';

@@ -49,7 +52,8 @@ window.angular && (function(angular) {
             day: 'numeric'
           }) + ' ' +
               dt.toLocaleTimeString(
-                  ro.locale, {timeZone: tz, timeZoneName: 'short'});
+                  ro.locale, {timeZone: tz}) +
+                moment.tz(dt, tz).format(' z');
         }
       });
 })(window.angular);

And run again:

$ npm run-script build
$ du -bs dist
370456  dist/

In my opinion, the (370456 - 295253) = 75203 is too many.