Fixes #2824: [A11y] Add label for RecentlyPlayedActivity #3065

rt4914 · 2021-04-09T00:52:42Z

Explanation

For RecentlyPlayedActivity the label can be either Stories for you or Recently Played Stories.
This activity can be visited via two screens: (a.) Click on View All in HomeActivity and (b.) Click on View All in ProfileProgressActivity.

The main problem is that we need to send the string in advance via intent as we need it for screen readers. To do this I have created RecentlyPlayedTitleEnum enum which will send the correct title. But this was not working and even I found one bug with Toolbar too in current develop. The toolbar title fluctuates. Check this below gif and focus on toolbar.

Earlier we were using toolbar in fragment and we were fetching the content from domain layer and based on the content received we were setting the toolbar. Now because in the new implementation we already have the title we can shift this toolbar to activity so that the flakiness is removed and the screen readers can work properly.

Screenreader output on this branch

current_recently_played.mp4

Note to Reviewer

In this draft PR I am just confirming the approach and therefore I done following things which will be fixed in final PR:

Removed all other layout files related to recently_played so that I don't have to worry about them.
While calling the intent for RecentlyPlayed I am passing the hard-coded/fixed enum value if the approach is finalised this will be sent correctly.
I am not updated the test cases which might be failing.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

rt4914 · 2021-04-09T00:53:20Z

@BenHenning PTAL at approach.

BenHenning · 2021-04-09T02:46:08Z

Apologies--will need to look at this tomorrow.

BenHenning

Thanks @rt4914. Left some thoughts, but the overall approach seems good.

BenHenning · 2021-04-09T21:34:02Z

app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt

      val intent = Intent(context, RecentlyPlayedActivity::class.java)
      intent.putExtra(RECENTLY_PLAYED_ACTIVITY_INTERNAL_PROFILE_ID_KEY, internalProfileId)
+      intent.putExtra(RECENTLY_PLAYED_ACTIVITY_TITLE_ENUM_KEY, recentlyPlayedTitleEnum)


This seems reasonable to me, but I think we want to do a push toward using protos more aggressively for sending arguments & intent extras. Maybe just go ahead and define an intent extra proto for this activity & use that, instead? It means a less-clean enum but you could still create an extension function for getTitleFromStringRes to keep dependent code clean.

I have tried to use protos. I am not sure if its correct so do let me know WDYT.

DestinationScreen will helps to identify which activity to open and also with what intent extra values.

For example for this implementation, RecentlyPlayedActivityIntentExtras is a value inside DestinationScreen which contains information about the destination activity as well as its intent extras.

In activity package I have created ActivityRouter which will be injected to all activities / fragments from where we want to start the next activity. In this we will call routeToScreen with correct DestinationScreen and then this will decide which activity should be started.

One added code clarity point: With this approach we can change RecentlyPlayedActivityIntentExtras proto anytime it will be very easy to make changes to code. For example in current develop as I had to send an enum I had to introduced a new key and change the createRecentlyPlayedActivityIntent function by introducing a new argument which in turn resulted in changes in all the instances where that function was getting called. But with this new approach these changes will be comparatively less.

Ah, I was just thinking that you could pass a RecentlyPlayedActivityIntentExtras proto here & proceed similarly. The router approach seems much nicer. Do you want to do that change as part of this PR or separately?

BenHenning · 2021-04-09T21:43:19Z

app/src/main/res/layout-land/recently_played_fragment.xml

-<layout xmlns:android="http://schemas.android.com/apk/res/android"
-  xmlns:app="http://schemas.android.com/apk/res-auto">
-
-  <androidx.constraintlayout.widget.ConstraintLayout


Did we ever need landscape-specific specialization for this layout or did you just noticed it wasn't needed?

Actually we do need other xml files too but if this approach gets approved the xml files will change a lot and therefore I thought to remove them directly for now, finish the implementation, then again add those files back with all the changes. That way i won't have to make changes to various versions of xml files again and again.

BenHenning · 2021-04-09T21:44:48Z

app/src/main/res/layout/recently_played_activity.xml

+        app:layout_constraintBottom_toBottomOf="parent"
+        app:layout_constraintEnd_toEndOf="parent"
+        app:layout_constraintStart_toStartOf="parent"
+        app:layout_constraintTop_toBottomOf="@id/recently_played_app_bar_layout" />


Is it value to have constraints when within a layout being constrained? Do we eve need these?

These constraints are incorrect. Removed now.

BenHenning · 2021-04-09T21:46:02Z

app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedTitleEnum.kt

+  fun getTitleFromStringRes(context: Context): String? {
+    return title.let(context::getString)
+  }


Suggested change

fun getTitleFromStringRes(context: Context): String? {

return title.let(context::getString)

}

fun getTitleFromStringRes(context: Context): String? = context.getString(title)

The scope function way seems less readable to me, but YMMV. I also changed this to an optional single-assignment syntax.

Updated as per suggestion.

BenHenning · 2021-04-09T21:47:27Z

app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedTitleEnum.kt

+
+/** Represents different titles for RecentlyPlayedActivity used by screen readers. */
+enum class RecentlyPlayedTitleEnum(@StringRes private var title: Int) {
+  RECENTLY_PLAYED_STORIES(R.string.recently_played_stories),


Why isn't this using recently_played_activity_title?

Correct. Used recently_played_activity_title

BenHenning · 2021-04-09T21:49:05Z

Also just to check: is the bug in the first video with or without the intent approach? I wouldn't expect the flicker to happen with your current implementation--is that the case?

rt4914 · 2021-04-12T06:43:09Z

Also just to check: is the bug in the first video with or without the intent approach? I wouldn't expect the flicker to happen with your current implementation--is that the case?

The bug is without this approach, in current develop. With current implementation is it fixed.

BenHenning · 2021-04-13T02:16:46Z

@rt4914 I think the approach seems fine, though I do suggest going with protos instead (one way or another with regards to introducing a router or not--I'll defer to you on how/when you want to introduce that). I think the overall solution is actually quite nice, and am happy to see the bug get fixed.

BenHenning · 2021-04-14T16:38:09Z

app/src/main/java/org/oppia/android/app/activity/ActivityRouter.kt

+
+  /** Checks the value of [DestinationScreen] and routes to different activities accordingly. */
+  fun routeToScreen(destinationScreen: DestinationScreen) {
+    if (destinationScreen.destinationScreenCase ==


Should this be a when statement, instead? Maybe also use 'return' to force it to be exhaustive.

BenHenning · 2021-04-14T16:39:23Z

app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt

      val intent = Intent(context, RecentlyPlayedActivity::class.java)
-      intent.putExtra(RECENTLY_PLAYED_ACTIVITY_INTERNAL_PROFILE_ID_KEY, internalProfileId)
+      intent.putExtra(


Prefer using this utility instead: https://github.com/oppia/oppia-android/blob/develop/utility/src/main/java/org/oppia/android/util/extensions/BundleExtensions.kt#L13.

BenHenning · 2021-04-14T16:41:29Z

model/src/main/proto/route.proto

+}
+
+// Intent extras for RecentlyPlayedActivity.
+message RecentlyPlayedActivityIntentExtras{


Nit: space before {

oppiabot · 2021-08-14T01:09:42Z

Hi @rt4914, 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 3 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!

Approach for RecentlyPlayed label

7addaed

rt4914 assigned BenHenning Apr 9, 2021

Nit fix

ff88c01

BenHenning reviewed Apr 9, 2021

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Apr 9, 2021

Rajat Talesra added 4 commits April 12, 2021 08:44

Merge remote-tracking branch 'upstream/develop' into a11y-recenly-played

77e122d

Nit fix

8e75c7f

Draft impl of proto based intents

77be534

Nit fix and documentation

804c7db

Rajat Talesra added 2 commits April 12, 2021 12:35

Proto fix

205bfe3

Nit fix

9dfb3e4

rt4914 assigned BenHenning and unassigned rt4914 Apr 12, 2021

BenHenning assigned rt4914 and unassigned BenHenning Apr 13, 2021

Merge remote-tracking branch 'upstream/develop' into a11y-recenly-played

c5c763a

BenHenning reviewed Apr 14, 2021

View reviewed changes

rt4914 mentioned this pull request Apr 15, 2021

Use protos with intent extras in RecenlyPlayedActivity #3095

Closed

anandwana001 added the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label May 7, 2021

Rajat Talesra added 2 commits June 28, 2021 17:48

Merged with develop

3c5dc75

Merge remote-tracking branch 'upstream/develop' into a11y-recenly-played

15c6066

rt4914 removed the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Jul 5, 2021

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 14, 2021

oppiabot bot closed this Aug 21, 2021

rt4914 mentioned this pull request Aug 24, 2021

Fix #3095: Use protos with intent extras in RecentlyPlayedActivity #3721

Closed

8 tasks

rt4914 mentioned this pull request Feb 14, 2022

[A11y] Add label for RecentlyPlayedActivity [Blocked on #3095] #2824

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2824: [A11y] Add label for RecentlyPlayedActivity #3065

Fixes #2824: [A11y] Add label for RecentlyPlayedActivity #3065

rt4914 commented Apr 9, 2021

rt4914 commented Apr 9, 2021

BenHenning commented Apr 9, 2021

BenHenning left a comment

BenHenning Apr 9, 2021

rt4914 Apr 12, 2021

BenHenning Apr 13, 2021

BenHenning Apr 9, 2021

rt4914 Apr 12, 2021

BenHenning Apr 9, 2021

rt4914 Apr 12, 2021

BenHenning Apr 9, 2021

rt4914 Apr 12, 2021

BenHenning Apr 9, 2021

rt4914 Apr 12, 2021

BenHenning commented Apr 9, 2021

rt4914 commented Apr 12, 2021

BenHenning commented Apr 13, 2021

BenHenning Apr 14, 2021

BenHenning Apr 14, 2021

BenHenning Apr 14, 2021

oppiabot bot commented Aug 14, 2021

Fixes #2824: [A11y] Add label for RecentlyPlayedActivity #3065

Fixes #2824: [A11y] Add label for RecentlyPlayedActivity #3065

Conversation

rt4914 commented Apr 9, 2021

Explanation

Screenreader output on this branch

Note to Reviewer

Checklist

rt4914 commented Apr 9, 2021

BenHenning commented Apr 9, 2021

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning commented Apr 9, 2021

rt4914 commented Apr 12, 2021

BenHenning commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Aug 14, 2021