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.

Sign up for GitHub

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

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

Closed
wants to merge 15 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4feb541
Added "Profile deleted successfully" on profile deleted
mbobiosio Jul 19, 2022
a486ff9
fixed ktlint issues
mbobiosio Jul 20, 2022
4d92f25
fixed ktlint issues
mbobiosio Jul 20, 2022
e3a46e1
updated timeAgo approach
mbobiosio Jul 26, 2022
c985faf
updated timeAgo approach for lastVisit
mbobiosio Jul 26, 2022
431412c
updated timeAgo approach for lastVisit
mbobiosio Jul 26, 2022
0c2cdd8
ktlint format
mbobiosio Jul 26, 2022
6908948
ktlint format
mbobiosio Jul 26, 2022
4048eee
ktlint format
mbobiosio Jul 26, 2022
022fb82
updated "time ago" implementation
mbobiosio Aug 3, 2022
416dba4
removed unrelated changes
mbobiosio Aug 11, 2022
6ae083e
refactored timeago
mbobiosio Aug 19, 2022
af93514
refactored timeago
mbobiosio Aug 19, 2022
44f718a
minor code formatting
mbobiosio Aug 20, 2022
4096e57
updated time ago java approach
mbobiosio Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ...c/main/java/org/oppia/android/app/administratorcontrols/appversion/AppVersionViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import androidx.lifecycle.ViewModel
import org.oppia.android.R
import org.oppia.android.app.fragment.FragmentScope
import org.oppia.android.app.translation.AppLanguageResourceHandler
import org.oppia.android.app.utility.getLastUpdateTime
import org.oppia.android.app.utility.getVersionName
import org.oppia.android.app.viewmodel.ObservableViewModel
import org.oppia.android.util.extensions.getLastUpdateTime
import org.oppia.android.util.extensions.getVersionName
import javax.inject.Inject

/** [ViewModel] for [AppVersionFragment]*/
Expand All @@ -25,7 +25,8 @@ class AppVersionViewModel @Inject constructor(

fun computeLastUpdatedDateText(): String =
resourceHandler.getStringInLocaleWithWrapping(
R.string.app_last_update_date, getDateTime(lastUpdateDateTime)
R.string.app_last_update_date,
getDateTime(lastUpdateDateTime)
)

private fun getDateTime(lastUpdateTime: Long): String =
Expand Down
49 changes: 49 additions & 0 deletions app/src/main/java/org/oppia/android/app/databinding/TextViewBindingAdapter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package org.oppia.android.app.databinding

import android.app.Activity
import android.content.ContextWrapper
import android.view.View
import android.widget.TextView
import androidx.databinding.BindingAdapter
import org.oppia.android.R
import org.oppia.android.app.translation.AppLanguageActivityInjectorProvider
import org.oppia.android.app.translation.AppLanguageResourceHandler

@BindingAdapter("profile:lastVisited")
Copy link
Member

Choose a reason for hiding this comment

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

Are these new binding adapters needed for this PR?

fun TextView.setProfileLastVisitedText(timestamp: Long?) {
timestamp?.let {
// text =
}
}

@BindingAdapter("profile:created")
fun setProfileDataText(textView: TextView, timestamp: Long) {
val resourceHandler = getResourceHandler(textView)
val time = resourceHandler.computeDateString(timestamp)
textView.text = resourceHandler.getStringInLocaleWithWrapping(
R.string.profile_edit_created,
time
)
}

// TODO: Move this to a common place.
fun getResourceHandler(view: View): AppLanguageResourceHandler {
val provider =
getAttachedActivity(view) as AppLanguageActivityInjectorProvider
return provider.getAppLanguageActivityInjector().getAppLanguageResourceHandler()
}

private fun getAttachedActivity(view: View): Activity {
var context = view.context
while (context != null && context !is Activity) {
check(context is ContextWrapper) {
(
"Encountered context in view (" + view + ") that doesn't wrap a parent context: " +
context
)
}
context = context.baseContext
}
checkNotNull(context) { "Failed to find base Activity for view: $view" }
return context as Activity
}
51 changes: 24 additions & 27 deletions app/src/main/java/org/oppia/android/app/databinding/TextViewBindingAdapters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,14 @@
import org.oppia.android.util.system.OppiaClock;
import org.oppia.android.util.system.OppiaClockInjectorProvider;

/** Holds all custom binding adapters that bind to [TextView]. */
/**
* Holds all custom binding adapters that bind to [TextView].
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Corrected this

*/
public final class TextViewBindingAdapters {

/** Binds date text with relative time. */
/**
* Binds date text with relative time.
*/
@BindingAdapter("profile:created")
public static void setProfileDataText(@NonNull TextView textView, long timestamp) {
AppLanguageResourceHandler resourceHandler = getResourceHandler(textView);
Expand All @@ -30,7 +34,9 @@ public static void setProfileDataText(@NonNull TextView textView, long timestamp
));
}

/** Binds last used with relative timestamp. */
/**
* Binds last used with relative timestamp.
*/
@BindingAdapter("profile:lastVisited")
public static void setProfileLastVisitedText(@NonNull TextView textView, long timestamp) {
AppLanguageResourceHandler resourceHandler = getResourceHandler(textView);
Expand All @@ -45,7 +51,10 @@ public static void setProfileLastVisitedText(@NonNull TextView textView, long ti
}

// TODO(#4345): Add test for this method.
/** Binds an AndroidX KitKat-compatible drawable top to the specified text view. */

/**
* Binds an AndroidX KitKat-compatible drawable top to the specified text view.
*/
@BindingAdapter("app:drawableTopCompat")
public static void setDrawableTopCompat(
@NonNull TextView imageView,
Expand All @@ -56,7 +65,9 @@ public static void setDrawableTopCompat(
);
}

/** Binds an AndroidX KitKat-compatible drawable end to the specified text view. */
/**
* Binds an AndroidX KitKat-compatible drawable end to the specified text view.
*/
@BindingAdapter("app:drawableEndCompat")
public static void setDrawableEndCompat(
@NonNull TextView imageView,
Expand All @@ -68,35 +79,34 @@ public static void setDrawableEndCompat(
}

private static String getTimeAgo(View view, long lastVisitedTimestamp) {
long timeStampMillis = ensureTimestampIsInMilliseconds(lastVisitedTimestamp);
long currentTimeMillis = getOppiaClock(view).getCurrentTimeMs();
AppLanguageResourceHandler resourceHandler = getResourceHandler(view);

if (timeStampMillis > currentTimeMillis || timeStampMillis <= 0) {
if (lastVisitedTimestamp > currentTimeMillis || lastVisitedTimestamp <= 0) {
return resourceHandler.getStringInLocale(R.string.last_logged_in_recently);
}

long timeDifferenceMillis = currentTimeMillis - timeStampMillis;
long timeDifferenceMillis = currentTimeMillis - lastVisitedTimestamp;

if (timeDifferenceMillis < (int) TimeUnit.MINUTES.toMillis(1)) {
return resourceHandler.getStringInLocale(R.string.just_now);
} else if (timeDifferenceMillis < TimeUnit.MINUTES.toMillis(50)) {
return getPluralString(
resourceHandler,
resourceHandler,
R.plurals.minutes,
(int) TimeUnit.MILLISECONDS.toMinutes(timeDifferenceMillis)
);
} else if (timeDifferenceMillis < TimeUnit.DAYS.toMillis(1)) {
return getPluralString(
resourceHandler,
resourceHandler,
R.plurals.hours,
(int) TimeUnit.MILLISECONDS.toHours(timeDifferenceMillis)
);
} else if (timeDifferenceMillis < TimeUnit.DAYS.toMillis(2)) {
return resourceHandler.getStringInLocale(R.string.yesterday);
}
return getPluralString(
resourceHandler,
resourceHandler,
R.plurals.days,
(int) TimeUnit.MILLISECONDS.toDays(timeDifferenceMillis)
);
Expand All @@ -107,24 +117,11 @@ private static String getPluralString(
@PluralsRes int pluralsResId,
int count
) {
// TODO(#3841): Combine these strings together.
return resourceHandler.getStringInLocaleWithWrapping(
R.string.time_ago,
resourceHandler.getQuantityStringInLocaleWithWrapping(
return resourceHandler.getQuantityStringInLocaleWithWrapping(
pluralsResId, count, String.valueOf(count)
Copy link
Member

Choose a reason for hiding this comment

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

Is this indentation still correct?

Copy link
Author

Choose a reason for hiding this comment

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

Yes this is correct

)
);
}

private static long ensureTimestampIsInMilliseconds(long lastVisitedTimestamp) {
// TODO(#3842): Investigate & remove this check.
if (lastVisitedTimestamp < 1000000000000L) {
// If timestamp is given in seconds, convert that to milliseconds.
return TimeUnit.SECONDS.toMillis(lastVisitedTimestamp);
}
return lastVisitedTimestamp;
}

private static AppLanguageResourceHandler getResourceHandler(View view) {
AppLanguageActivityInjectorProvider provider =
(AppLanguageActivityInjectorProvider) getAttachedActivity(view);
Expand All @@ -136,8 +133,8 @@ private static Activity getAttachedActivity(View view) {
while (context != null && !(context instanceof Activity)) {
if (!(context instanceof ContextWrapper)) {
throw new IllegalStateException(
"Encountered context in view (" + view + ") that doesn't wrap a parent context: "
+ context
"Encountered context in view (" + view + ") that doesn't wrap a parent context: "
+ context
);
}
context = ((ContextWrapper) context).getBaseContext();
Expand Down
44 changes: 22 additions & 22 deletions app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditFragmentPresenter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,16 @@ class ProfileEditFragmentPresenter @Inject constructor(
ProfileId.newBuilder().setInternalId(internalProfileId).build(),
binding.profileEditAllowDownloadSwitch.isChecked
).toLiveData().observe(
activity,
Observer {
if (it is AsyncResult.Failure) {
oppiaLogger.e(
"ProfileEditActivityPresenter", "Failed to updated allow download access", it.error
)
}
activity
) {
if (it is AsyncResult.Failure) {
oppiaLogger.e(
"ProfileEditActivityPresenter",
"Failed to updated allow download access",
it.error
)
}
)
}
}
return binding.root
}
Expand All @@ -132,22 +133,21 @@ class ProfileEditFragmentPresenter @Inject constructor(
profileManagementController
.deleteProfile(ProfileId.newBuilder().setInternalId(internalProfileId).build()).toLiveData()
.observe(
fragment,
Observer {
if (it is AsyncResult.Success) {
if (fragment.requireContext().resources.getBoolean(R.bool.isTablet)) {
val intent =
Intent(fragment.requireContext(), AdministratorControlsActivity::class.java)
intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP)
fragment.startActivity(intent)
} else {
val intent = Intent(fragment.requireContext(), ProfileListActivity::class.java)
intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP)
fragment.startActivity(intent)
}
fragment
Copy link
Member

Choose a reason for hiding this comment

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

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.

) {
if (it is AsyncResult.Success) {
if (fragment.requireContext().resources.getBoolean(R.bool.isTablet)) {
val intent =
Intent(fragment.requireContext(), AdministratorControlsActivity::class.java)
intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP)
fragment.startActivity(intent)
} else {
val intent = Intent(fragment.requireContext(), ProfileListActivity::class.java)
intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP)
fragment.startActivity(intent)
}
}
)
}
}

/** This loads the dialog whenever requested by the listener in [AdministratorControlsActivity]. */
Expand Down
46 changes: 46 additions & 0 deletions app/src/main/java/org/oppia/android/app/utility/datetime/DateTimeUtil.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package org.oppia.android.app.utility.datetime
import org.oppia.android.R
import org.oppia.android.app.translation.AppLanguageResourceHandler
import org.oppia.android.util.locale.OppiaLocale
import java.util.Calendar
import javax.inject.Inject

/** Per-activity utility to manage date and time for user-facing strings. */
Expand Down Expand Up @@ -30,4 +31,49 @@ class DateTimeUtil @Inject constructor(
/** Returns [DateTimeUtil] for the current Dagger graph. */
fun getDateTimeUtil(): DateTimeUtil
}

/**
* Returns the readable string of the duration from the provided time in [Long].
*/
fun timeAgoFromTimestamp(timestamp: Long, referenceTime: Long): String {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

val diff = (currentDate() - timestamp) / MILLI_SECONDS
Copy link
Member

Choose a reason for hiding this comment

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

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).


return when {
diff < SECONDS -> resourceHandler.getStringInLocale(R.string.just_now)
diff < HOUR -> resourceHandler.getQuantityStringInLocaleWithWrapping(
R.plurals.minutes_ago, diff.diffToInt(SECONDS), diff.diffToString(SECONDS)
)
diff < DAY -> resourceHandler.getQuantityStringInLocaleWithWrapping(
R.plurals.hours_ago, diff.diffToInt(HOUR), diff.diffToString(HOUR)
)
diff < WEEK -> resourceHandler.getQuantityStringInLocaleWithWrapping(
R.plurals.days_ago, diff.diffToInt(DAY), diff.diffToString(DAY)
)
diff < WEEKS -> resourceHandler.getQuantityStringInLocaleWithWrapping(
R.plurals.weeks_ago, diff.diffToInt(WEEK), diff.diffToString(WEEK)
)
diff < MONTHS -> resourceHandler.getQuantityStringInLocaleWithWrapping(
R.plurals.months_ago,
diff.diffToInt(WEEKS),
diff.diffToString(WEEKS)
)
else -> resourceHandler.getQuantityStringInLocaleWithWrapping(
R.plurals.years_ago, diff.diffToInt(MONTHS), diff.diffToString(MONTHS)
)
}
}

companion object {
fun currentDate() = Calendar.getInstance().timeInMillis
Copy link
Member

Choose a reason for hiding this comment

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

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.

const val MILLI_SECONDS = 1000L
const val SECONDS = 60L
const val HOUR = 60 * SECONDS
const val DAY = 24 * HOUR
const val WEEK = 7 * DAY
const val WEEKS = 2_628_000L
const val MONTHS = 31_536_000L
}
}

private fun Long.diffToInt(duration: Long) = div(duration).toInt()
private fun Long.diffToString(duration: Long) = div(duration).toString()
34 changes: 34 additions & 0 deletions app/src/main/res/values/plurals.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>

<plurals name="minutes_ago">
Copy link
Member

Choose a reason for hiding this comment

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

We usually define plurals in strings.xml--is there a reason these can't go there?

Copy link
Member

Choose a reason for hiding this comment

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

For this & the others: the plural name should more clearly indicate what this is being used for as that provides better context to translators and within code. For example: last_logged_in_minutes_ago might be a much clearer alternative.

<item quantity="one">%s minute ago</item>
<item quantity="other">%s minutes ago</item>
</plurals>

<plurals name="hours_ago">
<item quantity="one">%s hour ago</item>
<item quantity="other">%s hours ago</item>
</plurals>

<plurals name="days_ago">
<item quantity="one">%s day ago</item>
<item quantity="other">%s days ago</item>
</plurals>

<plurals name="weeks_ago">
<item quantity="one">%s week ago</item>
<item quantity="other">%s weeks ago</item>
</plurals>

<plurals name="months_ago">
<item quantity="one">%s month ago</item>
<item quantity="other">%s months ago</item>
</plurals>

<plurals name="years_ago">
<item quantity="one">%s year ago</item>
<item quantity="other">%s years ago</item>
</plurals>

</resources>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: all files should end with a single EOF newline.

11 changes: 11 additions & 0 deletions app/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@
<string name="profile_edit_delete_dialog_title">Permanently delete this profile?</string>
<string name="profile_edit_delete_dialog_message">All progress will be deleted and cannot be recovered.</string>
<string name="profile_edit_delete_dialog_positive">Delete</string>
<string name="profile_edit_delete_successful_message">Profile deleted successfully</string>
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated to your change--why is it needed?

<string name="profile_edit_delete_dialog_negative">Cancel</string>
<string name="profile_edit_allow_download_heading">Allow Download Access</string>
<string name="profile_edit_allow_download_sub">User is able to download and delete content without Administrator password</string>
Expand Down Expand Up @@ -467,6 +468,16 @@
<item quantity="one">a day</item>
<item quantity="other">%s days</item>
</plurals>

<string name="minute_ago">a minute ago</string>
<string name="minutes_ago">%s minutes ago</string>
<string name="hour_ago">an hour ago</string>
<string name="hours_ago">%s hours ago</string>
<string name="days_ago">%s days ago</string>
<string name="month_ago">a month ago</string>
<string name="months_ago">%s months ago</string>
<string name="year_ago">a year ago</string>
<string name="years_ago">%s years ago</string>
Comment on lines +472 to +480
Copy link
Member

Choose a reason for hiding this comment

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

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?

<!-- ViewTags -->
<string name="topic_revision_recyclerview_tag">topic_revision_recyclerview_tag</string>
<string name="ongoing_recycler_view_tag">ongoing_recycler_view_tag</string>
Expand Down
Loading