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

Marquee restarts automatically in certain scenarios. #2581

Closed
Luffy18346 opened this issue Jan 29, 2021 · 9 comments · Fixed by #4392
Closed

Marquee restarts automatically in certain scenarios. #2581

Luffy18346 opened this issue Jan 29, 2021 · 9 comments · Fixed by #4392
Assignees
Labels
Impact: Low Low perceived user impact (e.g. edge cases). Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@Luffy18346
Copy link
Contributor

Luffy18346 commented Jan 29, 2021

Describe the bug
In ideal case, Marquee should only start when user click on toolbar but Marquee restarts automatically in certain scenarios after Marquee stops for the first time.

To Reproduce
Steps to reproduce the behavior:

General Steps for all below mentioned cases:

  1. Go to The Meaning of Equal Parts exploration
  2. Go to Second state. Note that the title is not moving.
  3. Click on Toolbar and Marquee starts moving.

Case 1:
4) Once marquee stops, pull down notification bar of your mobile from top to bottom and then pull up, you will see Marquee moving again without clicking the toolbar.

Case 2:
4) Once marquee stops, click on EditText, Marquee starts moving again. Now wait for it to stop don't do anything.
Also after the marquee stops, then press back button to close the keyboard and you will see Marquee starts moving again

Case 3:
4) Once marquee stops, click on hint button, Marquee starts moving again.

Case 4:
4) Once marquee stops, click on close button at top-left, Marquee starts moving again.

Case 5:
4) Once marquee stops, on clicking it again, Marquee won't start no matter how many times user clicks on the toolbar.

Expected behavior
Marquess effect should start only on click on the toolbar and not automatically & it should starts every time user clicks on the toolbar after marquee stops moving.

For reference please refer to this issue: #1770

@Luffy18346 Luffy18346 added Status: Not started Priority: Essential This work item must be completed for its milestone. labels Jan 29, 2021
@Luffy18346 Luffy18346 added this to the Backlog milestone Jan 29, 2021
@Prince-kushwaha
Copy link
Contributor

can is work on this issue

@Prince-kushwaha
Copy link
Contributor

@FareesHussain @Luffy18346 can i work on this issue

@Sparsh1212
Copy link
Contributor

If this is still up for grab then please assign it to me @FareesHussain .

@FareesHussain
Copy link
Contributor

@Prince-kushwaha any updates on this?

@FareesHussain
Copy link
Contributor

If this is still up for grab then please assign it to me @FareesHussain .

@Sparsh1212 as you are assigned to 4 other issues. I would recommend you to wait for those to get merged first.

@Sparsh1212
Copy link
Contributor

Sparsh1212 commented Feb 23, 2021

If this is still up for grab then please assign it to me @FareesHussain .

@Sparsh1212 as you are assigned to 4 other issues. I would recommend you to wait for those to get merged first.

Actually, I have initiated the PRs for the first two issues. @fsharpasharp has asked to wait for a while (comment) for these Bazel-related issues because some more research has to be done on them, so I think that will take some time to get reviewed. And I have unassigned myself from the 3rd issue. So, I currently have only 1 active issue of which I have already initiated the PR.

@Sparsh1212
Copy link
Contributor

Sparsh1212 commented Feb 25, 2021

@rt4914
https://stackoverflow.com/a/44795525
As per the above StackOverflow answer, it seems that the Marquee effect is restarting because TextView loses its focus.
There were two approaches to fix this and I tried both of them:

  1. By wrapping the TextView inside a Linear Layout (Ref)
  2. By setting fixed dimensions (instead of wrap_content or fill_parent) on the layout_width and layout_height. (Ref)
    But none of them seems to fix the problem that we are facing in this issue. The Marquee effect restarts again in the 5 cases.
    Can you please take a look at this and please guide me on how should I approach this now? because none of the two fixes seems to work.

@rt4914
Copy link
Contributor

rt4914 commented Mar 1, 2021

@Sparsh1212 That's exactly the issue. I tried to find solution but nothing seems to work for this. Me, Akash and few more contributors have tried solving this issue in past too and we haven't been able to fix it yet. I am afraid this might need more research from your side. Also, if this really becomes hard its completely fine to un-assign yourself and move on to some other issue.

@Sparsh1212
Copy link
Contributor

@Sparsh1212 That's exactly the issue. I tried to find solution but nothing seems to work for this. Me, Akash and few more contributors have tried solving this issue in past too and we haven't been able to fix it yet. I am afraid this might need more research from your side. Also, if this really becomes hard its completely fine to un-assign yourself and move on to some other issue.

Thank you. I will try to do a bit more research on this once again and if I will still not be able to find any fix for this, then will unassign myself.

@Sparsh1212 Sparsh1212 removed their assignment Mar 2, 2021
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jun 13, 2022
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jun 13, 2022
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jun 14, 2022
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jun 14, 2022
@BenHenning BenHenning moved this from Needs Triage to In Progress: Needs clarification in [Team] Core Learner and Mastery flows & UI Frontend - Android Jul 11, 2022
@BenHenning BenHenning moved this from In Progress: Needs clarification to Needs Triage in [Team] Core Learner and Mastery flows & UI Frontend - Android Jul 11, 2022
@Broppia Broppia added Impact: Low Low perceived user impact (e.g. edge cases). user_team Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). and removed Impact: Low Low perceived user impact (e.g. edge cases). labels Jul 29, 2022
@BenHenning BenHenning added Impact: Low Low perceived user impact (e.g. edge cases). Z-ibt Temporary label for Ben to keep track of issues he's triaged. issue_user_learner and removed good second issue Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). labels Sep 14, 2022
@BenHenning BenHenning removed this from the Backlog milestone Sep 16, 2022
@BenHenning BenHenning moved this from Needs Triage to In Progress: Being implemented in [Team] Core Learner and Mastery flows & UI Frontend - Android Sep 20, 2022
BenHenning pushed a commit that referenced this issue Sep 29, 2022
* Fix #4186 Uncheck all selection on developer options not working

* Fix #4186 Add changes to code as suggested on code review.

* Fix #4186 Undo changes on line wrap as advised by code reviewer.

* Fix #2581 Marquee restarts automatically in certain scenarios.

* Fix #2581 Marquee restarts automatically in certain scenarios.

* Fix #2581 Marquee restarts automatically in certain scenarios.

* Fix #2581 Marquee restarts automatically in certain scenarios.

* Revert "Fix #4186 Undo changes on line wrap as advised by code reviewer."

This reverts commit ca5bca5.

* Revert "Fix #4186 Add changes to code as suggested on code review."

This reverts commit 5295364.

* Revert "Fix #4186 Uncheck all selection on developer options not working"

This reverts commit 0cff83a.

* Insert timer in OnClick listener to enable unselect during subsequent clicks.

* Introduce custom translation animation logic to fix issue with the core marquee api.

* Introduce custom translation animation logic to fix issue with the core marquee api.

* Fix some of the failing tests.

* Fix some of the failing tests.

* Fix some of the failing tests, exclude MarqueeView from tests.

* Revert some of the changes.

* Add MarqueeView to bazel list of custom views that import resources.

* Remove Marquee tests to fix some failures.

* Update exploration_activity.xml to fix failing tests.

* Update exploration_activity.xml to fix failing tests.

* Fix failing tests.

* Fix some of the failing tests.

* Fix some of the failing tests.

* Fix some of the failing tests.

* Fix some of failing test cases.

* Update magic value textViewVirtualWidth to use the device width.

* Fix Buildifier issue with bazel.

* Update to use Library forked to oppia as advised.

* Fix custom res import issues.

* Add binding adapters for "app:speed" and "app:pause".

* Revert to "app" from "marquee".

* Revert unnecessary file exemption.

* Fix Nit.

* Fix failing tests and revert from previous fix, which could not be used in alpha.

* Change to pass "app" and "speed" values in code.

* Fix import NIT.

* Fix failing tests by updating Bazel config files to add Android-MarqueeView as external dependency.

* Fix buildifier issues after bazel file changes.

* Revert tests for this feature and fix NITs.

* Migrate to Oppia's fork of AndroidMarquee-View remove ellipsize assertion on some tests.

* Change width to matchParent on exploration_activity.xml.

* Update speed and pause to animationSpeed and animationDelay respectively to make it clear.

* Change Bazel workspace config to point to Oppia's official fork.

* Change to click on MarqueeView as it is parent to the ToolbarTitle on ExplorationActivityTest.kt.

* Add indentations to layouts and shallow_since from CI logs.

* Revert shallow since.

* Update shallow since.

* Add class assertion tests.

* Fix spacing NIT.
Repository owner moved this from In Progress: Being implemented to Done in [Team] Core Learner and Mastery flows & UI Frontend - Android Sep 29, 2022
BenHenning added a commit that referenced this issue Nov 17, 2022
This reverts commit 846657e.

Conflicts:
	app/src/main/res/layout/license_text_viewer_activity.xml
	app/src/main/res/layout/revision_card_activity.xml
BenHenning added a commit that referenced this issue Nov 18, 2022
)

## 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).
BenHenning added a commit that referenced this issue Nov 18, 2022
)

## 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact: Low Low perceived user impact (e.g. edge cases). Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

Successfully merging a pull request may close this issue.

8 participants