-
Notifications
You must be signed in to change notification settings - Fork 719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Latency message after streaming now reports with decimal point #1314
base: master
Are you sure you want to change the base?
Latency message after streaming now reports with decimal point #1314
Conversation
1b443e7
to
eb4cd73
Compare
message = getResources().getString(R.string.conn_hardware_latency)+" "+averageDecoderLat+" ms"; | ||
} | ||
// Will format to match `\d+\.\d` (formats 1f as "1.0", 0.12f as "0.1", and 12.34f as "12.3") | ||
DecimalFormat decimalFormat = new DecimalFormat("0.0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this do the right thing for locales that use other separators (such as ,
) for decimal values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch. I had assumed it would inherit the same local as getResources().getString()
, but that may be wrong according to the docs:
To obtain a
NumberFormat
for a specific locale, including the default locale, call one ofNumberFormat
's factory methods, such asgetInstance()
. In general, do not call theDecimalFormat
constructors directly, since theNumberFormat
factory methods may return subclasses other thanDecimalFormat
. If you need to customize the format object, do something like this:NumberFormat f = NumberFormat.getInstance(loc); if (f instanceof DecimalFormat) { ((DecimalFormat) f).setDecimalSeparatorAlwaysShown(true); }
Stay tuned for a fix...
} | ||
// Will format to match `\d+\.\d` (formats 1f as "1.0", 0.12f as "0.1", and 12.34f as "12.3") | ||
DecimalFormat decimalFormat = new DecimalFormat("0.0"); | ||
final String millisecondSuffix = " ms"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of localization, I noticed in someone's Discord video that there are localized versions of the millisecond suffix that we sometimes use and sometimes don't:
I'll see if there is an existing string I can leverage instead of hardcoding it as we have been in this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it appears I'd have to make a new localized string or edit the existing ones to be format strings. Am I allowed to change or add new strings, or is that discouraged because it breaks existing localization? Would it be better for me to surgically steal the localized string for the suffix (like that мс
in the Russian(?) string above) so that we don't have to get the localizers to do another pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgutman, please let me know which way you'd prefer. The former seems more "correct", but the latter seems easier and probably safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have to make a new localized string
@kentyman23 I think this is the correct approach.
@cgutman I'm wondering how the translations are occurring. It seems like it's all manually done through regular PRs? I could set something up on CrowdIn like I do for LizardByte and Sunshine. I have a mono project on CrowdIn that handles all the translations for code projects https://crowdin.com/project/lizardbyte (and another for docs). If you select any language you can see how the translations are divided by GitHub repo and priority. CrowdIn is free for open source. I opted to put everything in one project for ease of management. Let me know if you want me to setup something similar for Moonlight projects.
Edit: I see that translations occur at https://hosted.weblate.org/projects/moonlight/moonlight-android/ ... is only the ./app/src/main/res/values/strings.xml
supposed to be modified in git?
The Show latency message after streaming toast previously reported truncated integers because integer division was used to calculate them (5.99 would be reported as 5). This led to inaccuracies reported here: https://docs.google.com/spreadsheets/d/1WSyOIq9Mn7uTd94PC_LXcFlUi9ceZHhRgk-Yld9rLKc/
Since we're using floating-point arithmetic, we no longer have to avoid divide-by-zero errors. Corner cases one might see are "∞ ms" (positive time divided by 0 frames) or "NaN ms" (0 time divided by 0 frames). I felt these were perfectly fine to output in these theoretical, degenerate cases. Because of this, I was able to simplify the logic in
Game.onStop
to no longer check for0
s. This simpler logic means we don't need bothconn_client_latency_hw
andconn_hardware_latency
localized strings; I opted to remove the former and keep the latter and put a newline between them, as it was hard to quickly read both stats when the visual location of the values depended upon how the wrapping worked out. This is how it looks:(Note that PR #1313 was abandoned after I realized I pushed to my fork's
master
instead of a branch.)