Fixes #3841 'time_ago' & quantity plurals should be combined

[mbobiosio]

Explanation

Fixes #3841
Fixes #3842
This PR includes the required update to combine strings together

Essential Checklist

• The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
• Any changes to scripts/assets files have their rationale included in the PR explanation.
• The PR follows the style guide.
• The PR does not contain any unnecessary code changes from Android Studio (reference).
• The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
• The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

• Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
• For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
• Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
• Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

[BenHenning]

Thanks @mbobiosio. I took a first pass and had some comments--PTAL.

Also, CI checks are failing--please fix.

[BenHenning]

Why this change? Per https://developer.android.com/kotlin/style-guide#formatting_2 the previous version is acceptable (and is what we use for such cases). While I linked to the KDoc one, the same also applies for Java: https://google.github.io/styleguide/javaguide.html#s7.1-javadoc-formatting.

Ditto for the others.

[mbobiosio]

Corrected this

[BenHenning]

Is this indentation still correct?

[mbobiosio]

Yes this is correct

[BenHenning]

I'm not opposed to including this & other cleanup changes in this PR, but something to keep in mind in the future is that it's not always best practice to include unrelated changes in a PR to the central thing the PR is trying to accomplish. Doing so actually introduces some risk in that it can cause the PR to need to be reverted if the refactor has an unexpected behavior change, or it can be difficult to revert the PR due to a larger number of changes than what's actually necessary to accomplish its goal.

[BenHenning]

Why is this being used vs. passing in the current time? I think we conventionally use OppiaClock for calculating the current time, so I'm trying to understand this difference.

[BenHenning]

Unused import--please remove.

[BenHenning]

Are these still needed? It seems like your change is only using the new plurals.

Also, are there any old strings that can now be removed in favor of the new solution?

[BenHenning]

For this & the constants below, could we use TimeUnit, instead? That both avoids having to do time math (which can be easily misunderstood and/or written), and may help improve readability. An example of what this might look like:

val diffSeconds = TimeUnit.MILLISECONDS.toSeconds(currentTimeMillis - timestampMillis)
return when {
  diffSeconds < TimeUnit.MINUTES.toSeconds(1) -> ...
  diffSeconds < TimeUnit.HOURS.toSeconds(1) -> ...
  ...
}

I think TimeUnit could also simplify your diffTo* functions by providing easier conversions. There might even be a way to generalize this slightly, e.g. something like:

private val timestampFormatters by lazy {
  listOf(
    TimestampFormatter(threshold = TimeMoment.ONE_MINUTE, timeUnit = TimeUnit.SECONDS, formatTimestamp = this::formatSecondsTimestamp),
    TimestampFormatter(threshold = TimeMoment.ONE_HOUR, timeUnit = TimeUnit.MINUTES, formatTimestamp = this::formatMinutesTimestamp),
    ...
  )
}

fun formatTimeAgoTimestamp(timestampMillis: Long): String {
  val timestamp = TimeMoment(amount = timestampMillis, unit = TimeUnit.MILLISECONDS)
  val currentTime = TimeMoment(amount = oppiaClock.getCurrentTimeMillis(), unit = TimeUnit.MILLISECONDS)
  val timeSpent = currentTime - timestamp
  return timestampFormatters.firstOrNull {
    it.canFormatWithinThreshold(timeSpent)
  }?.formatString(timeSpent) ?: error("Encountered impossible time: $timeSpent (for time: $timestamp)")
}

private class TimestampFormatter(val threshold: TimeSpan, val timeUnit: TimeUnit, val formatTimestamp: (TimeMoment) -> String) {
  fun canFormatWithinThreshold(timestamp: TimeMoment): Boolean = timestamp.isEarlierThan(threshold)

  fun format(timestamp: TimeMoment): String = formatTimestamp(timestamp.convertTo(timeUnit))
}

private fun formatSecondsTimestamp(seconds: TimeMoment): String =
  resourceHandler.getStringInLocale(R.string.just_now)

private fun formatMinutesTimestamp(minutes: TimeMoment): String {
  return resourceHandler.getQuantityStringInLocaleWithWrapping(
    R.plurals.minutes_ago, minutes.asInt, minutes.asString
  )
}
...

// A simple alternative for Java 8-only time utilities.
private data class TimeMoment(val amount: Long, val unit: TimeUnit) {
  val inMilliseconds: Long
    get() = TimeUnit.MILLISECONDS.convert(amount, unit)

  val asInt: Int
    get() = amount.toInt()

  val asString: String
    get() = amount.toString()

  fun isEarlierThan(other: TimeMoment): Boolean = inMilliseconds < other.inMilliseconds

  fun convertTo(unit: TimeUnit): TimeMoment = TimeMoment(unit.convert(amount, this.unit), unit)

  operator fun minus(other: TimeMoment): TimeMoment =
    TimeMoment(inMilliseconds - other.inMilliseconds, TimeUnit.MILLISECONDS)

  companion object {
    val ONE_MINUTE = TimeMoment(amount = 1, unit = TimeUnit.MINUTES)
    val ONE_HOUR = TimeMoment(amount = 1, unit = TimeUnit.HOURS)
    ...
  }
}

(As this is just a concept, more work & reformatting is needed).

[BenHenning]

Method names should be verbs or verb phrases describing what's happening (e.g. formatting, or converting, in this case).

[BenHenning]

Please add new tests for this method to cover all possible pluralizations that should now be supported.

[BenHenning]

Are these new binding adapters needed for this PR?

[oppiabot]

Hi @mbobiosio, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!