Added locale support to utility functions in format-numbers.js #1394

ghost · 2018-10-20T08:35:56Z

This was a fix for issue #1322

codecov · 2018-10-20T08:42:02Z

Codecov Report

Merging #1394 into master will decrease coverage by 2.6%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1394      +/-   ##
==========================================
- Coverage   80.69%   78.09%   -2.61%     
==========================================
  Files         157      155       -2     
  Lines       10789    10539     -250     
  Branches     2622     2560      -62     
==========================================
- Hits         8706     8230     -476     
- Misses       1884     2082     +198     
- Partials      199      227      +28

Impacted Files	Coverage Δ
src/utils/format-numbers.js	`84.31% <100%> (+0.98%)`	⬆️
src/profile-logic/call-tree.js	`99.27% <100%> (+3.32%)`	⬆️
src/components/app/Root.js	`0% <0%> (-74.03%)`	⬇️
src/components/shared/thread/ActivityGraphFills.js	`55.09% <0%> (-37.97%)`	⬇️
.../components/shared/thread/SelectedActivityGraph.js	`64.28% <0%> (-28.58%)`	⬇️
src/components/shared/thread/ActivityGraph.js	`75.4% <0%> (-18.04%)`	⬇️
src/components/app/FooterLinks.js	`0% <0%> (-14.29%)`	⬇️
src/actions/profile-view.js	`83.45% <0%> (-6.7%)`	⬇️
src/components/shared/StackSettings.js	`60% <0%> (-6.67%)`	⬇️
src/profile-logic/profile-data.js	`78.58% <0%> (-5.74%)`	⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01ece9d...6bb436b. Read the comment docs.

julienw

Hey @monikamaheshwari, thanks for the patch!
The goal of this patch is to change the existing formatNumber to use the locale-aware function, and then use it. Can you look into it?

ghost · 2018-10-23T06:54:47Z

@julienw You want me to remove toFixed function and use toLocaleString ?

julienw · 2018-10-23T12:13:31Z

Yes I think that's the goal here.
Then I'd like to test the performance of this, because I believe toLocaleString can have performance issues, but I'm not sure... but we need to write the code to know it :)

brisad · 2018-10-23T19:55:13Z

Also note that when formatNumber get localized, the formatMilliseconds function will also be localized since it uses the former. That in turn means we can use the latter for the displayData totalTimeWithUnit instead of manually concatenating 'ms' to the end.

Though it does introduce extra function calls for a very simple thing, so we should check that is does not negatively impact performance as @julienw notes.

julienw · 2018-10-25T14:59:23Z

@monikamaheshwari hey, kind reminder that this PR is still awaiting some changes :-)

ghost · 2018-10-26T07:33:36Z

Do I need to add value.toLocaleString(undefined, { maxFractionalDigits: places }) instead of value.toFixed(places).

julienw · 2018-10-26T12:30:24Z

I believe you can even replace the whole function by: value.toLocaleString(undefined, { maximumFractionDigits: maxFractionalDigits, minimumSignificantDigits: significantDigits }.

You can also rename the arguments to minimumSignificantDigits and maximumFractionDigits so that you can use the shorthand: value.toLocaleString(undefined, { maximumFractionDigits, minimumSignificantDigits }.

Be careful to use the right property names.

I haven't tried this, so maybe this will need a bit more tweaking.

ghost · 2018-10-26T14:59:13Z

This is not working value.toLocaleString(undefined, { maximumFractionDigits: maxFractionalDigits, minimumSignificantDigits: significantDigits } nor value.toLocaleString(undefined, { maximumFractionalDigits: places })

julienw

I added a comment that seem to work. + some other comments to change more things in format-numbers.js.

And now you can remove formatNumberToString :)

src/utils/format-numbers.js

julienw · 2018-10-26T17:35:13Z

src/profile-logic/call-tree.js

-        ? _formatIntegerNumber
-        : _formatDecimalNumber;
+      const formattedTotalTime = formatNumber(totalTime, 2, 1);
+      const formattedSelfTime = formatNumber(selfTime, 2, 1);


I think you still want to check for _isIntegerInterval: then you can pass "0" as second parameter if it's true. And the first parameter should always be 1.

julienw · 2018-10-26T17:40:22Z

src/utils/format-numbers.js

+  return value.toLocaleString(undefined, {
+    maximumSignificantDigits: significantDigits,
+    maximumFractionDigits: maxFractionalDigits,
+  });
 }

 export function formatPercent(value: number): string {


you can also change this function, and use { style: 'percent' } in the second argument of toLocaleString.

Then you can use this in src/profile-logic/call-tree.js to format the %age value as well.

Adding only { style:'percent' } is not giving correct results.

can you tell m what you tried and what this yields? Thanks!

export function formatPercent(value: number): string { return value.toLocaleString(undefined, { style:'percent' }); }
This is not giving percentage in one point decimal

Of course not, because you need to give the right arguments to toLocaleString...

return value.toLocaleString(undefined, { minimumFractionDigits: 1, style:'percent' }); is giving one point decimal but when we divide it should not give percentage in decimal

can you please give me an example so that I can follow you better?

<div className="tooltipLabel"> Cells evicted: </div> - 15.9K / 26.6K (60%) + 15.9K / 26.6K (59.6%)

Maybe formatPercent should take the number of maximumFractionDigits so that we could use it in different ways ?

minimumFractionalDigits seems to work

julienw · 2018-10-29T17:33:25Z

src/utils/format-numbers.js

-   */
+    * Note that numDigitsOnLeft can be negative when the first non-zero digit
+    * is on the right of the decimal point.  0.01 = -1
+    */


note: I see you added a space in the indention, please remove it

julienw · 2018-10-29T17:34:01Z

src/utils/format-numbers.js

+  return value.toLocaleString(undefined, {
+    maximumSignificantDigits: significantDigits,
+    maximumFractionDigits: maxFractionalDigits,
+  });
 }

 export function formatPercent(value: number): string {


can you tell m what you tried and what this yields? Thanks!

julienw

Hey Monika, thanks for the change!

I think that we can do better for formatPercent, see below for more explanations.

julienw · 2018-10-31T12:44:00Z

src/profile-logic/call-tree.js

+        maxFractionalDigits
+      );
+
+      const formattedSelfTime = formatNumber(selfTime, 2, maxFractionalDigits);


I tried it and I think we should use 3 instead of 2 for these 2 calls, so that values like 13.3 are displayed. Previously we did it for all values (because toFixed(1) was used for all values), but for bigger values this is less useful.

julienw · 2018-10-31T12:45:07Z

src/utils/format-numbers.js

+  return value.toLocaleString(undefined, {
+    minimumFractionDigits: minFractionalDigits,
+    style: 'percent',
+  });


I see there are a few differences with what we were doing before.
For example, in markers (see this profile, by hovering the "GCMajor" marker), you can see that some values like "66%" are now shown with "66.0%".

I tried other things like using maximumFractionDigits but this brings other issues.
For example, here is what we want in the markers tooltips:

0.01356 => 0.0 (possibly 0.01)

0.1356 => 0.1 (possibly 0.14)

1.356 => 1.3

13.56 => 13

This means that we can't use maximumSignificantDigits: 2 because we'd get 0.014 for the first example. We can't use maximumFractionDigits: 1 because we'd get 13.6 for the last example. This means we need something in-between, and that's what we did in formatNumbers above.

That's why I now think we should reuse formatNumbers above, exactly like the previous code did. This can be done if formatNumbers can take a style parameter with a default "decimal" for other uses, but so that we can pass percent in this function.

Then the parameter we pass to formatPercent could be the maxFractionalDigits we pass to formatNumbers directly, with a default of 1. We can also use significantDigits = 2 in the call like we did before.

julienw · 2018-10-31T12:46:12Z

src/components/shared/MarkerTooltipContents.js

-                  timings.mmu_50ms,
-                  formatPercent
+                {_markerDetail('gcmmu50ms', 'MMU 50ms', timings.mmu_50ms, x =>
+                  formatPercent(x, 1)


1 is the default already, so you can use the existing code instead of changing it

julienw · 2018-10-31T13:12:28Z

src/utils/format-numbers.js

@@ -117,7 +120,7 @@ export function formatValueTotal(
  const value_total = formatNum(a) + ' / ' + formatNum(b);
  let percent = '';
  if (includePercent) {
-    percent = ' (' + formatPercent(a / b) + ')';
+    percent = ' (' + formatPercent(a / b, 0) + ')';


I think that with the previous change, then we don't need to change this code.

ghost · 2018-11-01T13:42:07Z

<div  className="tooltipLabel">
              Cells evicted
              :
            </div>
    -       15.9K / 26.6K (60%)
    +       15.9K / 26.6K (59.6%)``` 
not working in this case

julienw

Thanks fo the changes! We're nearly there :-)

Here are a few more changes to fix a problem I realized when testing it, and I believe this should also fix the problem in the tests you mentioned.

Thanks again!

julienw · 2018-11-05T16:49:09Z

src/utils/format-numbers.js

@@ -25,12 +25,13 @@ import type { Microseconds, Milliseconds } from '../types/units';
 export function formatNumber(
  value: number,
  significantDigits: number = 2,
-  maxFractionalDigits: number = 3
+  maxFractionalDigits: number = 3,
+  style: string = 'decimal'


nit: because the style can be either 'percent' or 'decimal', this should be reflected in the Flow typing. Can you please replace this line by:

style: 'decimal' | 'percent' = 'decimal'

julienw · 2018-11-05T16:51:22Z

src/utils/format-numbers.js

-   */
+  * Note that numDigitsOnLeft can be negative when the first non-zero digit
+  * is on the right of the decimal point.  0.01 = -1
+  */
  const numDigitsOnLeft = Math.floor(Math.log10(Math.abs(value))) + 1;


I realized something.
The value we pass for "percent" is something like 0.4 and not 40. Therefore numDigitsOnLeft is too small and we need to add 2 here when style is percent. Something like this:

let numDigitsOnLeft = Math.floor(Math.log10(Math.abs(value))) + 1; if (style === 'percent') { // We receive percent values as `0.4` but display them as `40`, so we // should add `2` here to account for this difference. numDigitsOnLeft += 2; } ...

I believe this should fix the difference we see in the tests.

<span className="treeViewRowColumn treeViewFixedColumn totalTimePercent" key="totalTimePercent" - title="100.0%" + title="100%" > - 100.0% + 100% </span> <span className="treeViewRowColumn treeViewFixedColumn totalTime" key="totalTime" title="1" @@ -226,13 +226,13 @@ } >

Not working in this case.

But this is actually better :) 100.0 was bad!

You can update the snapshots by running yarn test -u.

…fox-devtools#1394) Then these functions are used in a lot of places, especially in the call tree and in tooltips. Fixes firefox-devtools#1322

Add locale support to utility functions in format-numbers.js

0d1644a

ghost mentioned this pull request Oct 20, 2018

Add locale support to utility functions in format-numbers.js #1322

Closed

fix

a7e5f53

julienw requested changes Oct 22, 2018

View reviewed changes

toLocaleString in formatNumber func

0533d2f

julienw requested changes Oct 26, 2018

View reviewed changes

maheshwarimonika added 2 commits October 29, 2018 09:11

toLocaleString added

495e483

Merge branch fix

ea25a4e

julienw reviewed Oct 29, 2018

View reviewed changes

maheshwarimonika added 2 commits October 31, 2018 00:30

formatPercent localeString

ee93980

fix

d55d0af

julienw requested changes Oct 31, 2018

View reviewed changes

suggested changes

c83e398

julienw requested changes Nov 5, 2018

View reviewed changes

maheshwarimonika and others added 2 commits November 6, 2018 19:30

suggested changes

1a92783

Merge branch 'master' into locale-support-format-numbers

6bb436b

julienw approved these changes Nov 7, 2018

View reviewed changes

julienw merged commit 1d92528 into firefox-devtools:master Nov 7, 2018

julienw mentioned this pull request Nov 7, 2018

For profiles with non-integer values, the values displayed in the sidebar are not properly rounded #1466

Closed

julienw mentioned this pull request Nov 23, 2018

For profiles with non-integer values, the values displayed in the sidebar should be properly rounded #1491

Closed

Added locale support to utility functions in format-numbers.js #1394

Added locale support to utility functions in format-numbers.js #1394

Conversation

ghost commented Oct 20, 2018

codecov bot commented Oct 20, 2018 • edited

Codecov Report

julienw left a comment

Choose a reason for hiding this comment

ghost commented Oct 23, 2018 • edited by ghost

julienw commented Oct 23, 2018

brisad commented Oct 23, 2018

julienw commented Oct 25, 2018

ghost commented Oct 26, 2018

julienw commented Oct 26, 2018

ghost commented Oct 26, 2018

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 30, 2018 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 30, 2018 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 30, 2018 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 1, 2018 • edited by ghost

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 20, 2018 •

edited

ghost commented Oct 23, 2018 •

edited by ghost

ghost Oct 30, 2018 •

edited by ghost

ghost Oct 30, 2018 •

edited by ghost

ghost Oct 30, 2018 •

edited by ghost

ghost commented Nov 1, 2018 •

edited by ghost