Skip to content
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

Replace PrettyTime with DateUtils.getRelativeTimeSpanString() #3567

Closed
wants to merge 1 commit into from

Conversation

sryze
Copy link

@sryze sryze commented May 10, 2020

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

Remove dependency on PrettyTime by replacing it with a call to DateUtils.getRelativeTimeSpanString, which formats relative time in a similar way (although slightly differently):
https://developer.android.com/reference/android/text/format/DateUtils#getRelativeTimeSpanString(long)

One downside is that DateUtils abbreviates time unit names despite that the corresponding flag is not set (FORMAT_ABBREVIATE_RELATIVE), e.g. день -> дн. I didn't find a way to disable that...

Update:
Actually that's not true - I tested on a newer device (Android 10) and it's showing the time as expected, i.e. not abbreviated. It only happens with my old tablet running KitKat. Screenshot:

screenshot

Fixes the following issue(s)

PrettyTime has a bug with formatting relative time in Russian - ocpsoft/prettytime#182
For example, instead of "2 дня назад" (2 days ago) PrettyTime produces "2 дней назад", which is incorrect plural of "day" in Russian (same with other time units).

Testing apk

app-debug.zip

Agreement

@B0pol B0pol added bug Issue is related to a bug localisation / translation Everything that has to do with translations or Weblate labels May 10, 2020
Copy link
Member

@B0pol B0pol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use this method, but revert the removal of Prettytime:

  • This only follow system language. It should follow the language accessible with public static Locale getAppLocale(final Context context) in Localization class. You can set and test by changing the language in Settings -> Content -> App language
  • Some languages aren't supported by Android (it's the first reason I've made this language selector, along with debugging in english without changing phone language). E.g., Esperanto, but are supported by Prettytime.

This can be fixed though:

  • in Localization.changeAppLanguage(), add Locale.setDefault(loc);. The current setup only changed the resources, therefore the strings were in the good language, but the methods calling Locale.getDefault() (e.g. this one) were using phone's language instead of the language in the selector. This will fix my first demand.
  • Create a list of language only supported by prettytime. And then:
public static String relativeTime(final Calendar calendarTime, final Context context) {
    if (!getAppLocale(context).contains(THE_LIST)) {
        // return using prettytime
    } else {
        // return using DateUtils
    }
}

You'll have to adapt the add of context in relativeTime().

Second option, better:
Fork Prettytime and create a method to get the list of Locale supported. And instead of using our list locally, we'd use prettytime.
But as it requires an update from prettytime, it's better to implement our function, and remove it once this function has been merged in prettytime and update version.

Edit:
Using Locale.setDefault might break this

@sryze
Copy link
Author

sryze commented May 17, 2020

I didn't know that the Android version didn't support as many languages as PrettytTime, sorry. In that case, it's probably better to fix PrettyTime itself like you mentioned. I made a PR to fix this issue - ocpsoft/prettytime#189

@wb9688 wb9688 closed this Jun 25, 2020
@sryze
Copy link
Author

sryze commented Sep 26, 2020

FYI the PrettyTime PR was merged but the latest release (from April 2020) doesn't have it... so the bug is still present in NewPipe

@B0pol
Copy link
Member

B0pol commented Sep 26, 2020

Thanks for the information

@lincolnthree said there will be a release ASAP here

I hope it'll be released before the next newpipe version that should come really soon too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug localisation / translation Everything that has to do with translations or Weblate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants