Summary: | Web Inspector: Font Details sidebar - Fractional variation axis ranges/default values are rounded. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick Angle <pangle> | ||||||||||||
Component: | Web Inspector | Assignee: | Patrick Angle <pangle> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bburg, ews-watchlist, hi, inspector-bugzilla-changes, mmaxfield, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Patrick Angle
2021-01-08 12:37:42 PST
Created attachment 417654 [details]
Patch v1.0
Created attachment 417656 [details]
Screenshot of Patch v1.0
Comment on attachment 417654 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=417654&action=review r=me > Source/WebInspectorUI/ChangeLog:14 > + - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number. Are we sure that localizers will know what this is? Aside: I wonder if we actually want to use this instead of `f` more often π€ > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1096 > + g: function(substitution, token) Please add some tests for this :) IIRC there's tests for other stuff in `LayoutTests/inspector/unit-tests/string-utilities.html`. Style: you should be able to `g(substitution, token)` instead of having the `: function` :) Comment on attachment 417654 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=417654&action=review >> Source/WebInspectorUI/ChangeLog:14 >> + - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number. > > Are we sure that localizers will know what this is? > > Aside: I wonder if we actually want to use this instead of `f` more often π€ I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise. Comment on attachment 417654 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=417654&action=review >>> Source/WebInspectorUI/ChangeLog:14 >>> + - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number. >> >> Are we sure that localizers will know what this is? >> >> Aside: I wonder if we actually want to use this instead of `f` more often π€ > > I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise. I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved? Perhaps Greg or someone working on Safari knows who to contact. Comment on attachment 417654 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=417654&action=review >>>> Source/WebInspectorUI/ChangeLog:14 >>>> + - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number. >>> >>> Are we sure that localizers will know what this is? >>> >>> Aside: I wonder if we actually want to use this instead of `f` more often π€ >> >> I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise. > > I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved? Perhaps Greg or someone working on Safari knows who to contact. In looking this up in our own docs (https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html) which in turn references (http://www.opengroup.org/onlinepubs/009695399/functions/printf.html), I'm surprised to find that the behavior I've always taken for granted with `g` is actually a side effect, and the main purpose is actually to selectively use exponential form. There doesn't appear to be a clear analogue to the behavior I'm trying to achieve in `printf` parlance. π This leads me down a path where maybe it makes more sense to have a utility function on Number (e.g. `Number.decimalString(fraction, maximumPrecision, minimumPrecision)` or similar?) for this functionality. This patch is one of those cases where for 90%+ of fonts I don't expect any fractions, and the clutter makes it more difficult to read the values at a glance. Thoughts? Comment on attachment 417654 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=417654&action=review >>>>> Source/WebInspectorUI/ChangeLog:14 >>>>> + - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number. >>>> >>>> Are we sure that localizers will know what this is? >>>> >>>> Aside: I wonder if we actually want to use this instead of `f` more often π€ >>> >>> I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise. >> >> I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved? Perhaps Greg or someone working on Safari knows who to contact. > > In looking this up in our own docs (https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html) which in turn references (http://www.opengroup.org/onlinepubs/009695399/functions/printf.html), I'm surprised to find that the behavior I've always taken for granted with `g` is actually a side effect, and the main purpose is actually to selectively use exponential form. There doesn't appear to be a clear analogue to the behavior I'm trying to achieve in `printf` parlance. π > > This leads me down a path where maybe it makes more sense to have a utility function on Number (e.g. `Number.decimalString(fraction, maximumPrecision, minimumPrecision)` or similar?) for this functionality. This patch is one of those cases where for 90%+ of fonts I don't expect any fractions, and the clutter makes it more difficult to read the values at a glance. Thoughts? Maybe just use `Number.maxDecimals` (another one of our utilities) and `%s` instead? Comment on attachment 417654 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=417654&action=review >>>>>> Source/WebInspectorUI/ChangeLog:14 >>>>>> + - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number. >>>>> >>>>> Are we sure that localizers will know what this is? >>>>> >>>>> Aside: I wonder if we actually want to use this instead of `f` more often π€ >>>> >>>> I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise. >>> >>> I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved? Perhaps Greg or someone working on Safari knows who to contact. >> >> In looking this up in our own docs (https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html) which in turn references (http://www.opengroup.org/onlinepubs/009695399/functions/printf.html), I'm surprised to find that the behavior I've always taken for granted with `g` is actually a side effect, and the main purpose is actually to selectively use exponential form. There doesn't appear to be a clear analogue to the behavior I'm trying to achieve in `printf` parlance. π >> >> This leads me down a path where maybe it makes more sense to have a utility function on Number (e.g. `Number.decimalString(fraction, maximumPrecision, minimumPrecision)` or similar?) for this functionality. This patch is one of those cases where for 90%+ of fonts I don't expect any fractions, and the clutter makes it more difficult to read the values at a glance. Thoughts? > > Maybe just use `Number.maxDecimals` (another one of our utilities) and `%s` instead? I don't think that'll work here, since the decimal point needs to be localized still, and `maxDecimals` just rounds the number off. We would need to pass through `toLocaleString` at some point, with a `minimumFractionDigits` of `0` and a `maximumFractionDigits` of `2`, at which point the rounding wasn't necessary. Comment on attachment 417654 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=417654&action=review >>>>>>> Source/WebInspectorUI/ChangeLog:14 >>>>>>> + - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number. >>>>>> >>>>>> Are we sure that localizers will know what this is? >>>>>> >>>>>> Aside: I wonder if we actually want to use this instead of `f` more often π€ >>>>> >>>>> I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise. >>>> >>>> I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved? Perhaps Greg or someone working on Safari knows who to contact. >>> >>> In looking this up in our own docs (https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html) which in turn references (http://www.opengroup.org/onlinepubs/009695399/functions/printf.html), I'm surprised to find that the behavior I've always taken for granted with `g` is actually a side effect, and the main purpose is actually to selectively use exponential form. There doesn't appear to be a clear analogue to the behavior I'm trying to achieve in `printf` parlance. π >>> >>> This leads me down a path where maybe it makes more sense to have a utility function on Number (e.g. `Number.decimalString(fraction, maximumPrecision, minimumPrecision)` or similar?) for this functionality. This patch is one of those cases where for 90%+ of fonts I don't expect any fractions, and the clutter makes it more difficult to read the values at a glance. Thoughts? >> >> Maybe just use `Number.maxDecimals` (another one of our utilities) and `%s` instead? > > I don't think that'll work here, since the decimal point needs to be localized still, and `maxDecimals` just rounds the number off. We would need to pass through `toLocaleString` at some point, with a `minimumFractionDigits` of `0` and a `maximumFractionDigits` of `2`, at which point the rounding wasn't necessary. Well if need be we could just have this be a special case and call `.toLocaleString(undefined, {minimumFractionDigits: 0, maximumFractionDigits: 2})` where needed in `Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js` (maybe even make a private `_formatVariationValue`). Created attachment 417701 [details]
Patch v1.5 - Revised approach for converting fractions to strings
Comment on attachment 417701 [details] Patch v1.5 - Revised approach for converting fractions to strings View in context: https://bugs.webkit.org/attachment.cgi?id=417701&action=review r=me > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:5 > +localizedStrings[" (Range: %s-%s, Default: %s) @ Font Details Sidebar"] = " (Range: %s-%s, Default: %s)"; Hopefully localizers can figure out the context based on the string. > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:78 > +localizedStrings["%s%%"] = "%s%%"; It would be wise to explain what this is with a localization comment. Created attachment 417910 [details]
Patch v1.6 - Review Notes
Committed r271620: <https://trac.webkit.org/changeset/271620> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417910 [details]. |