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

Fix #4709: Revert "Fix #2581: Marquee auto restart issue (#4392)" #4730

Merged
merged 3 commits into from
Nov 18, 2022

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Nov 17, 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

  • 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

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

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
Copy link
Sponsor Member Author

Note that I've just self-reviewed this PR to make sure it all makes sense. There are only a few major differences in the layout structure, and I've manually verified that the marquee behaviors seem correct for all affected screens (plus, it's reverting back to a state that has been used for years prior in the app).

@BenHenning
Copy link
Sponsor Member Author

/cc @KevinGitonga as well as an FYI. Unfortunately, we need to revert your marquee view PR since I don't think we're going to be able to make it work (see #4709 (comment) for more specifics). I'll reopen #2581 once this is merged and then close it as infeasible.

@BenHenning BenHenning added the PR: Cherrypick requested Indicates that a PR is being requested for being cherrypicked into the ongoing release branch. label Nov 17, 2022
@BenHenning BenHenning marked this pull request as ready for review November 18, 2022 04:24
@BenHenning
Copy link
Sponsor Member Author

@seanlip PTAL. Note that I'll make the one edit I mentioned in my comment above & update to the latest develop a bit later since both are innocuous and unlikely to cause issues.

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@seanlip seanlip assigned BenHenning and unassigned seanlip Nov 18, 2022
@oppiabot
Copy link

oppiabot bot commented Nov 18, 2022

Assigning @rt4914 for code owner reviews. Thanks!

@BenHenning
Copy link
Sponsor Member Author

BenHenning commented Nov 18, 2022

Going ahead and merging this without your review @rt4914 since it's mainly a reversion, and it's release blocking. Feel free to post-submit review if you think you should.

@BenHenning BenHenning merged commit 351fd59 into develop Nov 18, 2022
@BenHenning BenHenning deleted the revert-4392 branch November 18, 2022 09:28
BenHenning added a commit that referenced this pull request 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 BenHenning added the PR: Cherrypick completed Indicates a cherrypick request was approved & completed for a PR. label Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Cherrypick completed Indicates a cherrypick request was approved & completed for a PR. PR: Cherrypick requested Indicates that a PR is being requested for being cherrypicked into the ongoing release branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exploration titles aren't showing up
3 participants