Skip to content

Commit

Permalink
Fix #4709: Revert "Fix #2581: Marquee auto restart issue (#4392)" (#4730
Browse files Browse the repository at this point in the history
)

## Explanation
Fixes #4709

This reverts commit 846657e from PR
#4392. See
#4709 (comment)
for a detailed explanation for why this reversion is the correct
approach.

Note that this PR will be cherry-picked into the 0.10 release branch for
the upcoming MR2 beta release.

## Essential Checklist
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
This is a mostly clean reversion of #4392, so see that PR for an idea on
how marquees behaved prior to that PR (as this PR returns the app to
that behavior).
  • Loading branch information
BenHenning authored Nov 18, 2022
1 parent 3f553ec commit 351fd59
Show file tree
Hide file tree
Showing 20 changed files with 58 additions and 147 deletions.
9 changes: 0 additions & 9 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,6 @@ git_repository(
shallow_since = "1647554845 -0700",
)

# A custom fork of Android-MarqueeView that is updated with latest dependencies compatible with Oppia and also
# min target SDK version set to be compatible with Oppia.
git_repository(
name = "marqueeview",
commit = "a935a78c88a01958716396e6f2cb4abf4559eccc",
remote = "https://github.com/oppia/Android-MarqueeView",
shallow_since = "1663393399 -0400",
)

bind(
name = "databinding_annotation_processor",
actual = "//tools/android:compiler_annotation_processor",
Expand Down
1 change: 0 additions & 1 deletion app/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,6 @@ android_library(
"//third_party:androidx_recyclerview_recyclerview",
"//third_party:androidx_viewpager2_viewpager2",
"//third_party:androidx_viewpager_viewpager",
"//third_party:asia_ivity_android_marqueeview",
"//third_party:circularimageview_circular_image_view",
"//third_party:com_google_android_flexbox_flexbox",
"//third_party:com_google_android_material_material",
Expand Down
2 changes: 0 additions & 2 deletions app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ dependencies {
'com.google.firebase:firebase-crashlytics:17.0.0',
'com.google.guava:guava:28.1-android',
'com.google.protobuf:protobuf-javalite:3.17.3',
'com.github.oppia:Android-MarqueeView:a935a78c88',
'com.github.oppia:CircularImageview:35d08ba88a',
'de.hdodenhof:circleimageview:3.0.1',
'nl.dionsegijn:konfetti:1.2.5',
Expand Down Expand Up @@ -210,7 +209,6 @@ dependencies {
'androidx.test.espresso:espresso-intents:3.1.0',
'androidx.test.ext:junit:1.1.1',
'com.github.bumptech.glide:mocks:4.11.0',
'com.github.oppia:Android-MarqueeView:a935a78c88',
'com.google.truth:truth:1.1.3',
'androidx.work:work-testing:2.4.0',
'com.google.truth.extensions:truth-liteproto-extension:1.1.3',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class LicenseTextViewerActivityPresenter @Inject constructor(
}

binding.licenseTextViewerActivityToolbarTitle.setOnClickListener {
binding.licenseTextViewerActivityMarqueeView.startMarquee()
binding.licenseTextViewerActivityToolbarTitle.isSelected = true
}

if (getLicenseTextViewerFragment() == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class ExplorationActivityPresenter @Inject constructor(
activity.setSupportActionBar(explorationToolbar)

binding.explorationToolbarTitle.setOnClickListener {
binding.explorationMarqueeView.startMarquee()
binding.explorationToolbarTitle.isSelected = true
}

binding.explorationToolbar.setNavigationOnClickListener {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class StoryFragmentPresenter @Inject constructor(
}

binding.storyToolbarTitle.setOnClickListener {
binding.storyMarqueeView?.startMarquee()
binding.storyToolbarTitle.isSelected = true
}

linearLayoutManager = LinearLayoutManager(activity.applicationContext)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ class TopicFragmentPresenter @Inject constructor(
(activity as TopicActivity).finish()
}

binding.topicToolbarTitle.setOnClickListener {
binding.topicMarqueeView.startMarquee()
binding.topicToolbar.setOnClickListener {
binding.topicToolbarTitle.isSelected = true
}

viewModel.setInternalProfileId(internalProfileId)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class RevisionCardActivityPresenter @Inject constructor(
(activity as RevisionCardActivity).finish()
}
binding.revisionCardToolbarTitle.setOnClickListener {
binding.revisionCardMarqueeView.startMarquee()
binding.revisionCardToolbarTitle.isSelected = true
}
subscribeToSubtopicTitle()

Expand Down
21 changes: 6 additions & 15 deletions app/src/main/res/layout-sw600dp/story_fragment.xml
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>
<layout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto"
xmlns:marquee="http://schemas.android.com/apk/res-auto"
xmlns:tools="http://schemas.android.com/tools">

<data>
Expand Down Expand Up @@ -37,21 +36,13 @@
app:navigationContentDescription="@string/navigate_up"
app:navigationIcon="?attr/homeAsUpIndicator">

<asia.ivity.android.marqueeview.MarqueeView
android:id="@+id/story_marquee_view"
android:layout_width="match_parent"
<TextView
android:id="@+id/story_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
app:animationDelay="500"
app:animationSpeed="10">

<TextView
android:id="@+id/story_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginEnd="20dp"
android:text="@{viewModel.storyNameLiveData}" />
</asia.ivity.android.marqueeview.MarqueeView>
android:layout_marginEnd="20dp"
android:text="@{viewModel.storyNameLiveData}" />
</androidx.appcompat.widget.Toolbar>
</com.google.android.material.appbar.AppBarLayout>

Expand Down
18 changes: 5 additions & 13 deletions app/src/main/res/layout/exploration_activity.xml
Original file line number Diff line number Diff line change
Expand Up @@ -38,21 +38,13 @@
android:layout_height="wrap_content"
android:orientation="horizontal">

<asia.ivity.android.marqueeview.MarqueeView
android:id="@+id/exploration_marquee_view"
android:layout_width="match_parent"
<TextView
android:id="@+id/exploration_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginEnd="20dp"
android:layout_weight="1"
app:animationDelay="500"
app:animationSpeed="10">

<TextView
android:id="@+id/exploration_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="match_parent"
android:layout_height="wrap_content" />
</asia.ivity.android.marqueeview.MarqueeView>
android:layout_weight="1" />

<ImageView
android:id="@+id/action_audio_player"
Expand Down
17 changes: 4 additions & 13 deletions app/src/main/res/layout/license_text_viewer_activity.xml
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,13 @@
app:navigationContentDescription="@string/navigate_up"
app:navigationIcon="?attr/homeAsUpIndicator">

<asia.ivity.android.marqueeview.MarqueeView
android:id="@+id/license_text_viewer_activity_marquee_view"
<TextView
android:id="@+id/license_text_viewer_activity_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_marginEnd="20dp"
android:minHeight="48dp"
app:animationDelay="500"
app:animationSpeed="10">

<TextView
android:id="@+id/license_text_viewer_activity_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_marginEnd="20dp" />
</asia.ivity.android.marqueeview.MarqueeView>
android:minHeight="48dp" />
</androidx.appcompat.widget.Toolbar>
</com.google.android.material.appbar.AppBarLayout>

Expand Down
18 changes: 5 additions & 13 deletions app/src/main/res/layout/revision_card_activity.xml
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,12 @@
android:minHeight="48dp"
android:scaleType="center" />

<asia.ivity.android.marqueeview.MarqueeView
android:id="@+id/revision_card_marquee_view"
android:layout_width="match_parent"
<TextView
android:id="@+id/revision_card_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
app:animationDelay="500"
app:animationSpeed="10">

<TextView
android:id="@+id/revision_card_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginEnd="20dp" />
</asia.ivity.android.marqueeview.MarqueeView>
android:layout_marginEnd="20dp"/>
</androidx.appcompat.widget.Toolbar>
</com.google.android.material.appbar.AppBarLayout>

Expand Down
20 changes: 6 additions & 14 deletions app/src/main/res/layout/story_fragment.xml
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,13 @@
app:navigationContentDescription="@string/navigate_up"
app:navigationIcon="?attr/homeAsUpIndicator">

<asia.ivity.android.marqueeview.MarqueeView
android:id="@+id/story_marquee_view"
android:layout_width="match_parent"
<TextView
android:id="@+id/story_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
app:animationDelay="500"
app:animationSpeed="10">

<TextView
android:id="@+id/story_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_marginEnd="20dp"
android:text="@{viewModel.storyNameLiveData}" />
</asia.ivity.android.marqueeview.MarqueeView>
android:layout_marginEnd="20dp"
android:text="@{viewModel.storyNameLiveData}"/>
</androidx.appcompat.widget.Toolbar>
</com.google.android.material.appbar.AppBarLayout>

Expand Down
20 changes: 6 additions & 14 deletions app/src/main/res/layout/topic_fragment.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,13 @@
app:navigationContentDescription="@string/navigate_up"
app:navigationIcon="?attr/homeAsUpIndicator">

<asia.ivity.android.marqueeview.MarqueeView
android:id="@+id/topic_marquee_view"
android:layout_width="match_parent"
<TextView
android:id="@+id/topic_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
app:animationDelay="500"
app:animationSpeed="10">

<TextView
android:id="@+id/topic_toolbar_title"
style="@style/MarqueeToolbarTitleTextView"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_marginEnd="20dp"
android:text="@{viewModel.topicToolbarTitleLiveData}" />
</asia.ivity.android.marqueeview.MarqueeView>
android:layout_marginEnd="20dp"
android:text="@{viewModel.topicToolbarTitleLiveData}" />
</androidx.appcompat.widget.Toolbar>

<com.google.android.material.tabs.TabLayout
Expand Down
6 changes: 6 additions & 0 deletions app/src/main/res/values/styles.xml
Original file line number Diff line number Diff line change
Expand Up @@ -445,13 +445,19 @@
<style name="BorderlessMaterialButton" parent="Widget.AppCompat.Button.Borderless" />

<style name="MarqueeToolbarTitleTextView" parent="TextView.Common1">
<item name="android:ellipsize">marquee</item>
<item name="android:fadingEdge">horizontal</item>
<item name="android:fadingEdgeLength">20dp</item>
<item name="android:focusable">false</item>
<item name="android:focusableInTouchMode">true</item>
<item name="android:fontFamily">sans-serif</item>
<item name="android:marqueeRepeatLimit">1</item>
<item name="android:minHeight">48dp</item>
<item name="android:textAlignment">viewStart</item>
<item name="android:textColor">@color/color_def_white</item>
<item name="android:textSize">20sp</item>
<item name="android:requiresFadingEdge">horizontal</item>
<item name="android:scrollHorizontally">true</item>
<item name="android:singleLine">true</item>
<item name="fontFamily">sans-serif</item>
<item name="textAllCaps">false</item>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package org.oppia.android.app.player.exploration
import android.app.Application
import android.content.Context
import android.content.Intent
import android.text.TextUtils
import android.view.View
import android.widget.TextView
import androidx.appcompat.app.AppCompatActivity
Expand All @@ -28,7 +29,6 @@ import androidx.test.espresso.intent.matcher.IntentMatchers.hasComponent
import androidx.test.espresso.intent.matcher.IntentMatchers.hasExtra
import androidx.test.espresso.matcher.RootMatchers.isDialog
import androidx.test.espresso.matcher.ViewMatchers.Visibility
import androidx.test.espresso.matcher.ViewMatchers.assertThat
import androidx.test.espresso.matcher.ViewMatchers.isChecked
import androidx.test.espresso.matcher.ViewMatchers.isClickable
import androidx.test.espresso.matcher.ViewMatchers.isDisplayed
Expand All @@ -41,15 +41,13 @@ import androidx.test.espresso.util.HumanReadables
import androidx.test.espresso.util.TreeIterables
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.rule.ActivityTestRule
import asia.ivity.android.marqueeview.MarqueeView
import com.google.common.truth.Truth.assertThat
import dagger.Component
import org.hamcrest.BaseMatcher
import org.hamcrest.CoreMatchers.allOf
import org.hamcrest.CoreMatchers.containsString
import org.hamcrest.Description
import org.hamcrest.Matcher
import org.hamcrest.Matchers.instanceOf
import org.hamcrest.Matchers.not
import org.junit.After
import org.junit.Before
Expand Down Expand Up @@ -327,12 +325,10 @@ class ExplorationActivityTest {
val explorationToolbarTitle: TextView =
explorationActivityTestRule.activity.findViewById(R.id.exploration_toolbar_title)
ViewCompat.setLayoutDirection(explorationToolbarTitle, ViewCompat.LAYOUT_DIRECTION_RTL)
val explorationMarqueeView: MarqueeView =
explorationActivityTestRule.activity.findViewById(R.id.exploration_marquee_view)

onView(withId(R.id.exploration_marquee_view)).perform(click())
onView(withId(R.id.exploration_toolbar_title)).perform(click())
assertThat(explorationToolbarTitle.ellipsize).isEqualTo(TextUtils.TruncateAt.MARQUEE)
assertThat(explorationToolbarTitle.textAlignment).isEqualTo(View.TEXT_ALIGNMENT_VIEW_START)
assertThat(explorationMarqueeView, instanceOf(MarqueeView::class.java))
}

@Test
Expand All @@ -348,13 +344,11 @@ class ExplorationActivityTest {
)
val explorationToolbarTitle: TextView =
explorationActivityTestRule.activity.findViewById(R.id.exploration_toolbar_title)
val explorationMarqueeView: MarqueeView =
explorationActivityTestRule.activity.findViewById(R.id.exploration_marquee_view)
ViewCompat.setLayoutDirection(explorationToolbarTitle, ViewCompat.LAYOUT_DIRECTION_LTR)

onView(withId(R.id.exploration_marquee_view)).perform(click())
onView(withId(R.id.exploration_toolbar_title)).perform(click())
assertThat(explorationToolbarTitle.ellipsize).isEqualTo(TextUtils.TruncateAt.MARQUEE)
assertThat(explorationToolbarTitle.textAlignment).isEqualTo(View.TEXT_ALIGNMENT_VIEW_START)
assertThat(explorationMarqueeView, instanceOf(MarqueeView::class.java))
}

@Test
Expand Down
Loading

0 comments on commit 351fd59

Please sign in to comment.