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

Fragment returns null view on config change in submit time error #4708

Closed
vrajdesai78 opened this issue Nov 12, 2022 · 9 comments · Fixed by #5205
Closed

Fragment returns null view on config change in submit time error #4708

vrajdesai78 opened this issue Nov 12, 2022 · 9 comments · Fixed by #5205
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. good first issue This item is good for new contributors to make their pull request. Impact: Low Low perceived user impact (e.g. edge cases). Work: Low Solution is clear and broken into good-first-issue-sized chunks.

Comments

@vrajdesai78
Copy link
Contributor

Describe the bug
Whenever we compute submit time error, rotates screen and again submits answer [without error] the app crashes and in log it shows that fragment is returning null view.

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'MathInputInteraction'
  2. Submit answer with error
  3. Rotate screen
  4. Remove '?' from fragment.view? in hideKeyboard method in StateFragmentPresenter.kt
  5. Submit answer without error.
  6. Observe error log

Expected behavior
App should not return null view, it should return a non-null view when we submits answer without error.

Screen Recording

rotate-screen-error.mp4

Environment

  • Device/emulator being used: Realme GT Master Edition
  • Android or SDK version (e.g. Android 5 or SDK 21): Android 12
  • App version (you can get this through system app settings or via the admin controls menu in-app): 1.0
@BenHenning
Copy link
Member

This can also be seen by playing up to fractions in the test prototype exploration and introducing a submit-time error (e.g. by typing "1/" and tapping 'Submit'), then rotating the screen.

@adhiamboperes adhiamboperes added Impact: Low Low perceived user impact (e.g. edge cases). bug End user-perceivable behaviors which are not desirable. Work: Low Solution is clear and broken into good-first-issue-sized chunks. good first issue This item is good for new contributors to make their pull request. labels Aug 14, 2023
@mukulasija
Copy link
Contributor

i would like to work on this issue

@adhiamboperes
Copy link
Collaborator

i would like to work on this issue

Could you please give a brief overview of your proposed solution?

@EmmanuelDav
Copy link

Could you please give a brief overview of your proposed solution?

is this issue still available, if so to answer the question i would consider, I would consider using ViewModel to store and manage the data associated with the Fragment. ViewModel survives configuration changes and it can help maintain the state of the view making it not to return a null view.

Alternatively, I could use setRetainInstance(true) to retain the configuration changes after screen rotation. I haven't viewed the source code, I don't know which would work

@Bontrey
Copy link
Contributor

Bontrey commented Oct 10, 2023

Since there's some interest here and I've also been looking at this, thought that I should share my current findings:

The current problem reproduced by the initial description (causing an input error, then rotating) seems to be that the recycler view becomes empty.

This happens because the answer is submitted even if it's an empty default answer.

The submit runs into an error, and this tryOperation ends up in the catch block, which does not run recomputeCurrentStateAndNotifySync to emit a new EphemeralState. The last emitted state result was Pending from SUBMITTING_ANSWER, so the itemList is never repopulated (Pending result does not cause processEphemeralState to run) after it's cleared out from the Fragment recreation on configuration change.

I think the solution might just be to return null instead of a default answer and to not attempt the submit in that case, but the testing instructions are outdated (and I'm new to real world android development) so I'm having trouble adding a unit test. Maybe I'll ask in the discussions.

EDIT: asked at #5184

@adhiamboperes
Copy link
Collaborator

@EmmanuelDav setRetainedInstance can be risky, because it can lead to memory leaks. I think these answers offer a pretty good explanation.

@adhiamboperes
Copy link
Collaborator

@Bontrey, Could you share a draft PR with your fix?

Bontrey pushed a commit to Bontrey/oppia-android that referenced this issue Oct 16, 2023
@Bontrey
Copy link
Contributor

Bontrey commented Oct 16, 2023

Bontrey@1560426

But satisfying "Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing" will take some time for StateFragmentTest since there are many tests and some of them seem to fail for unrelated reasons (example).

I'll keep going on it though

@adhiamboperes
Copy link
Collaborator

adhiamboperes commented Oct 16, 2023

Bontrey@1560426

But satisfying "Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing" will take some time for StateFragmentTest since there are many tests and some of them seem to fail for unrelated reasons (example).

I'll keep going on it though

Can you push the code so that the tests run on our CI?

Bontrey pushed a commit to Bontrey/oppia-android that referenced this issue Oct 20, 2023
Bontrey pushed a commit to Bontrey/oppia-android that referenced this issue Oct 23, 2023
Bontrey pushed a commit to Bontrey/oppia-android that referenced this issue Oct 24, 2023
Bontrey pushed a commit to Bontrey/oppia-android that referenced this issue Oct 24, 2023
Bontrey pushed a commit to Bontrey/oppia-android that referenced this issue Oct 24, 2023
adhiamboperes added a commit that referenced this issue Oct 28, 2023
…ut (#5205)

## Explanation
Fixes #4708: Don't submit answer if it's invalid according to the input
or else after submitting the recycler view will not restore items on
configuration change. See #4708 for more details

Video demo:

[before](https://drive.google.com/file/d/1bLgo-AYro0UbffR6X8nWo8Xv7oUuNJFa/view?usp=sharing)

[after](https://drive.google.com/file/d/1Oek7j6dgjJmgasyyd9FHtQo4E7zTvEBd/view?usp=sharing)

Continuation of PR #5202 since that one's no longer viable due to being
force pushed in a repo sync through GitHub UI.

## 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: ...".)
- [ ] 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
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

Change unrelated to concerns like dark mode, RTL, accessibility. PR
#5202 had passing test checks for these changes, so that'll be my
demonstration of tests passing.

---------

Co-authored-by: Long Wei <[email protected]>
Co-authored-by: Adhiambo Peres <[email protected]>
Rd4dev pushed a commit to Rd4dev/oppia-android that referenced this issue Oct 31, 2023
…e input (oppia#5205)

## Explanation
Fixes oppia#4708: Don't submit answer if it's invalid according to the input
or else after submitting the recycler view will not restore items on
configuration change. See oppia#4708 for more details

Video demo:

[before](https://drive.google.com/file/d/1bLgo-AYro0UbffR6X8nWo8Xv7oUuNJFa/view?usp=sharing)

[after](https://drive.google.com/file/d/1Oek7j6dgjJmgasyyd9FHtQo4E7zTvEBd/view?usp=sharing)

Continuation of PR oppia#5202 since that one's no longer viable due to being
force pushed in a repo sync through GitHub UI.

## 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: ...".)
- [ ] 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
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

Change unrelated to concerns like dark mode, RTL, accessibility. PR
oppia#5202 had passing test checks for these changes, so that'll be my
demonstration of tests passing.

---------

Co-authored-by: Long Wei <[email protected]>
Co-authored-by: Adhiambo Peres <[email protected]>
adhiamboperes added a commit that referenced this issue Jan 11, 2024
Refactor firebase auth wrapper

Refactor dagger bindings for firestore dependencies

Ensure firestore logs show in dev event logs view.

Removed redundant bindings.

Localisation updates from https://translatewiki.net. (#5209)

Translation updates

Fixes #4708: Don't submit answer if it's invalid according to the input (#5205)

Fixes #4708: Don't submit answer if it's invalid according to the input
or else after submitting the recycler view will not restore items on
configuration change. See #4708 for more details

Video demo:

[before](https://drive.google.com/file/d/1bLgo-AYro0UbffR6X8nWo8Xv7oUuNJFa/view?usp=sharing)

[after](https://drive.google.com/file/d/1Oek7j6dgjJmgasyyd9FHtQo4E7zTvEBd/view?usp=sharing)

Continuation of PR #5202 since that one's no longer viable due to being
force pushed in a repo sync through GitHub UI.

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

If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

Change unrelated to concerns like dark mode, RTL, accessibility. PR
demonstration of tests passing.

---------

Co-authored-by: Long Wei <[email protected]>
Co-authored-by: Adhiambo Peres <[email protected]>

Fix part of #5195: using viewLifecycleOwner (#5207)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
It is not a good idea to use a fragment as a lifecycle owner when
subscribing to liveData objects. It would be correct to use
viewLifecycleOwner.

Lint report before:
<img
src="https://github.com/oppia/oppia-android/assets/71126152/ad482fd4-d3b0-46c7-b7de-febe7a48c07b"
width="70%" height="70%" />

Lint report after
<img
src="https://github.com/oppia/oppia-android/assets/71126152/fe7a26cf-1fcd-4cd5-ae3a-3cb7dcf2659c"
width="70%" height="70%" />

<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [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)).

<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

Fix #5032 Images in Arabic (RTL) lessons are right-aligned rather than center-aligned. (#5212)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
Fixes #5032, To make the image center-aligned, we are adding styling to
the HTML content image and removing the left margin by setting
drawableLeft bound to 0 in RTL.
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [ ] 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)).

LTR Potrait

![beforeLTRpotrait](https://github.com/oppia/oppia-android/assets/76042077/298fc2c0-7512-40dc-8d56-444fe55f4998)

LTR Landscape

![LTRlandscape](https://github.com/oppia/oppia-android/assets/76042077/94e16ccb-052c-4723-b26e-00fa0591270d)

RTL Potrait

![beforeRTLpotrait](https://github.com/oppia/oppia-android/assets/76042077/59efc1c5-c695-442a-9549-8aa353d61aed)

RTL Landscape

![RTLlandscape](https://github.com/oppia/oppia-android/assets/76042077/dae3c7b8-ce4e-4968-8dea-c4bc23eaf75f)

RTL Potrait

![Potrait_after](https://github.com/oppia/oppia-android/assets/76042077/d5115f63-f71c-44fa-bb60-639e8f8abcef)

RTL Landscape

![landscapeafter](https://github.com/oppia/oppia-android/assets/76042077/9a37d6d1-dc80-489e-827c-8b208fa33d67)

LTR Potrait

![LTRpotraitafter](https://github.com/oppia/oppia-android/assets/76042077/4d51cf7c-68eb-4924-aaaa-64cb44391081)

LTR Landscape

![LTRLandscape](https://github.com/oppia/oppia-android/assets/76042077/802dcd37-5adb-4f5d-8a30-cfa110d76885)

<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

fix: Fix Translate Wiki Issues with Splash Activity Screen Strings (#5219)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->
When merged, this PR will:
- Remove white spaces and new lines that were added to Splash Activity
Screen strings by the [App/OS Deprecation Milestone 3
PR](#5096). This will allow
TranslateWiki to translate them. See related [comment on the
PR](d471d79#r130759117).

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [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)).

<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

Co-authored-by: Adhiambo Peres <[email protected]>

Fix #5214, Fix Part of #1723 : Fix onboarding issue faced by new contributors (#5210)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
Fix #5214 : Wiki link not redirect to correct destination (
good-first-issue link)
Fix Part of #1723 : Fix onboarding issue faced by new contributors.

1. Tell contributors firmly to use "Android Studio Bumblebee | 2021.1.1
Patch 3."
2. In the wiki, make sure to mention that you need to have JDK 11 to use
oppia-android.
3. Concentrate on using robolectric instead of espresso and provide
clear instructions for setting up tests.

<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [ ] 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)).

Fix #5204 - Inconsistent Layout Lint Warning (#5218)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->
Fix #5204 - [Part of #5169]

To address the lint warnings, two potential solutions were considered.
1. Initially, there was an option to include the missing IDs, ensuring
consistency across different layout configurations
[[**Link**](Rd4dev@f8977e6)].
However, it was observed that implementing this solution led to
functional disruptions within the current working application, as
confirmed through comprehensive testing procedures (refer to the
attached assets for a detailed overview).

https://github.com/oppia/oppia-android/assets/122200035/82172eaf-a555-4e76-b1ae-8f0db7ddb924

2. The second option was to suppress the lint warnings for the specific
views in the layout-sw600dp configuration files
[[**Link**](Rd4dev@a0f5b1e)]
to maintain the seamless functionality of the application . This was
achieved by incorporating the attribute
**`tools:ignore="InconsistentLayout"`** for relevant views, without
impacting the current operational stability of the application.

https://github.com/oppia/oppia-android/assets/122200035/93a65422-3526-4d76-873f-c08d24abc3ee

Taking the necessary action, I proceeded to implement the suppression of
lint warnings by submitting a pull request.

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [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".
- [ ] 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)).

<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

Co-authored-by: Adhiambo Peres <[email protected]>

Fix #5112: Rendering Inline SVGs (#5208)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
Fixes #5112

We discovered that `mathImg_` dimesions are in **ex**, and not **px**,
and this unit is not natively rendered on android.

This PR introduces a method that will convert ex to px and use the
resulting dimesions to compute the rendering size of a math image to be
rendered inline. The formula is based on the [androidSvg
library](https://github.com/oppia/androidsvg/blob/5bc9c7553e94c3476e8ea32baea3c77567228fcd/androidsvg/src/main/java/com/caverock/androidsvg/androidrendering/SVGAndroidRenderer.java#L5018)
that is also based on CSS3 specs.

To preserve the size computation of block SVGs, we have created a
seperate method that handles the calculation of rendering dimensions for
inline SVGs.

We have only tested these changes through visual observation, and
confirming that the images scale correctly when reading text size is
changed.

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [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)).

|Before|After|
|--|--|

|![IMG_0705](https://github.com/oppia/oppia-android/assets/99066793/62e00542-091c-48b1-9e63-aa1ca3c9abff)|![Screenshot_1698283405](https://github.com/oppia/oppia-android/assets/59600948/e1c534ee-b379-461c-bc39-7854fe57dc70)|

|![Screenshot_1698284125](https://github.com/oppia/oppia-android/assets/59600948/3f47014d-f4e3-4430-9e1e-d4b4609014e7)|![Screenshot_1698283411](https://github.com/oppia/oppia-android/assets/59600948/786445ec-0b75-41af-882b-ae93e2da9b4d)|

|![Screenshot_1697714168](https://github.com/oppia/oppia-android/assets/59600948/9fa8daeb-25ca-4b90-ac66-a6228c81f120)|![Screenshot_1698284519](https://github.com/oppia/oppia-android/assets/59600948/0b39d37d-1471-4ecf-8626-13a01ec35c3e)|

|![Screenshot_1697745323](https://github.com/oppia/oppia-android/assets/59600948/8152c286-8845-4f3b-8ecf-93acd03dbc3e)|![Screenshot_1698284666](https://github.com/oppia/oppia-android/assets/59600948/dbff75ba-bb08-4855-b49a-0e4d337c478b)|

|![Screenshot_1697714820](https://github.com/oppia/oppia-android/assets/59600948/65b7eadc-c8b0-4963-8dbf-d4cd19590e98)|![Screenshot_1698284933](https://github.com/oppia/oppia-android/assets/59600948/9b517dc5-fc75-43f8-b6af-a4e24b1cde28)|

Images scaled to Largest text and display size, and dark mode
||||
|--|--|--|

|![image](https://github.com/oppia/oppia-android/assets/59600948/cedd58b2-a16f-4ed7-9856-df9371242d3a)|![image](https://github.com/oppia/oppia-android/assets/59600948/acf79348-72cb-4732-b91a-1f934861d9f0)|![image](https://github.com/oppia/oppia-android/assets/59600948/fa07209d-fb54-4f21-a267-cc9bd7d26177)|

|![image](https://github.com/oppia/oppia-android/assets/59600948/241f8501-21ce-488f-be6e-a6f2b4bde198)|![image](https://github.com/oppia/oppia-android/assets/59600948/82d0a270-e486-4553-96ae-559ad7b784a2)|![image](https://github.com/oppia/oppia-android/assets/59600948/6080aff6-aff0-46ad-b30c-88e086aeeac7)|

---------

Co-authored-by: Ben Henning <[email protected]>

Fix #3596: Adds Audio Loading UI (#5179)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

Fixes #3596

This PR adds an indeterminate progress bar when the audio is loading,
before it starts playing.

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [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)).

<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

https://github.com/oppia/oppia-android/assets/84731134/5c6fe393-5d89-4bda-a144-57bdc42a9c57

---------

Co-authored-by: Adhiambo Peres <[email protected]>

Fix #5226: EnableContinueButtonAnimation Feature Flag  (#5228)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
Fix #5226

Remove declarations and usages of the enableContinueButtonAnimation
PlatformParameter.

(A) Removed declaration of
TestPlatformParameterModule.forceEnableContinueButtonAnimation() from
all test functions namely:

testContinueInteractionAnim_openPrototypeExp_checkContinueButtonAnimatesAfter45Seconds(),
testConIntAnim_openProtExp_orientLandscapeAfter30Sec_checkAnimHasNotStarted(),
testConIntAnim_openProtExp_orientLandAfter30Sec_checkAnimStartsIn15SecAfterOrientChange(),
testContNavBtnAnim_openMathExp_checkContNavBtnAnimatesAfter45Seconds(),
testContNavBtnAnim_openMathExp_playThroughSecondState_checkContBtnDoesNotAnimateAfter45Sec(),
testConIntAnim_openFractions_expId1_checkButtonDoesNotAnimate()
in StateFragmentLocalTest.kt file

(B) Removed function declaration
TestPlatformParameterModule.forceEnableContinueButtonAnimation(false)
from setup() function
in StateFragmentTest.kt


(C) Removed function declaration
TestPlatformParameterModule.forceEnableContinueButtonAnimation(false)
from setup() function
in ExploreActivityTest.kt

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [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)).

Fix #5230: VectorDrawableCompat error lint (#5237)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
Fix #5230: VectorDrawableCompat Lint error

Warning: VectorDrawableCompat
Fix: opened the app's build.gradle file.
Inside the android block, added the vectorDrawables section under
defaultConfig with the code below:
android { ... defaultConfig { ... vectorDrawables { useSupportLibrary
true } } }

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [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)).

Fix #3345 Images are not fully displayed  (#5234)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
Fixes #3345, substracting drawableWidth from maxContentItemPadding only
if drawableWidth >= (maxcontentWidth -
 maxContentItemPadding) would solve the issue.
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [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)).

https://user-images.githubusercontent.com/82181327/122643554-54f1ac00-d108-11eb-906f-6748d2facf2f.jpg

https://user-images.githubusercontent.com/101634267/245153650-cd8335df-01ec-47e6-97b2-7f7244d95efb.jpg

https://user-images.githubusercontent.com/101634267/245153665-cb9506d2-6d0d-4231-8a42-5f8ab15cf4d4.jpg

![after_image_cut](https://github.com/oppia/oppia-android/assets/76042077/babacf98-1ef6-4f29-98a4-cc46c5f53303)

![after_imagecut](https://github.com/oppia/oppia-android/assets/76042077/e08cf8d0-5f25-46bb-afbe-124952d19bfc)

![after_image_cut2](https://github.com/oppia/oppia-android/assets/76042077/85e950df-1ae9-48f2-b46c-c610b838593a)

![After_RTL](https://github.com/oppia/oppia-android/assets/76042077/586ade28-a45e-4f0a-9ac8-51457ccf8b5a)

![after_no_effect_RTL](https://github.com/oppia/oppia-android/assets/76042077/75820a6d-cc76-4ec0-b90e-ede7e5a72643)

<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

Localisation updates from https://translatewiki.net. (#5242)

Translation updates

Fix #5137: Upgrade builds to target SDK 33 (#5222)

Fixes #5137
Partially mitigates #5233

This updates all build & target SDKs for Gradle & Bazel builds to now
target SDK 33, per the new Play Store mandate (see
https://support.google.com/googleplay/android-developer/answer/11926878?hl=en).
The analysis of SDK 33 features, potential issues, and potential
mitigations are described in #5137. It was determined that there are no
obvious code changes needed beyond the minimum to target SDK 33. We'll
be relying on some platform-level regression testing plus the Play
Console's pre-submit app analysis report for finalizing the go/no-go
decision for SDK 33 support.

Testing consisted of manually playing through the app and seeing if
there were any issues using emulators both with SDK 33 and lower
versions. #5137 documents the issues found and their ultimate causes.
Since no functionality changes were needed for SDK 33, no tests needed
updating. Separately, tests are still running on SDK 30 due to being
stuck (see #4748).

One issue unrelated to SDK 33 was observed: when playing through the
prototype lesson, I once again brought the app into a broken state
wherein I couldn't leave the lesson via navigation. We've hit this issue
a few times, but still don't know the root cause. #5233 was filed and a
mitigation was introduced in this PR (+ a test for it): if the
exploration player observes a failure when trying to stop the player
session (for whatever reason), it still proceeds with its "end of
activity" flow (e.g. finishing the activity). This provides at least an
escape hatch for users who somehow hit this state. Note that this isn't
a proper fix for #5233 given the lack of a known root cause, so this is
only considered a mitigation.

The new test was verified as failing if the mitigation is omitted:

![image](https://github.com/oppia/oppia-android/assets/12983742/10ddbf85-332c-4469-8efa-483a967170f9)

Note that ``ExplorationActivityTest``'s setup was tweaked to change the
parent screen since this affects back navigation routing. I opted to
using lessons tab as the parent screen since it's the most common route
for users to hit, so it makes sense for our tests to default to that if
they don't specifically need to use a different route (and none of the
existing tests seemed to have an issue with this change).

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

N/A -- This isn't changing any UIs directly, and per the analysis in
33 that would affect Oppia.

Technical Analytics: Milestone 1 - Add Feature Flag Statuses and Ability To Sync Them to Cache Store (#5203)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->
When merged, this PR will:
- Create a new `FeatureFlagConstants.kt` file to contain all feature
flags.
- Create feature flag sync status trackers for each existing feature
flag.
- Modify the `PlatformParameterSyncUpWorker` to allow status trackers of
all feature flags to be synced and cached in the PersistentCacheStore.
- Merge the `TestBooleanPlatformParameter`,
`TestStringPlatformParameter` and the `TestIntegerPlatformParameter`
files into the `TestPlatformParameterConstants` file.
- Add syncing with the web for the `EnableDownloadsSupport`,
`EnableEditAccountsOptionsUi`, `EnableSpotlightUi`,
`EnableExtraTopicTabsUi`, `EnableInteractionConfigChangeStateRetention`,
and `EnableAppAndOsDeprecation` feature flags.
- Write tests for the changes made.

<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
- [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)).

<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

---------

Co-authored-by: Adhiambo Peres <[email protected]>
Co-authored-by: Ben Henning <[email protected]>

Refactor TestAuthenticationModule.kt to provide FirebaseAuthWrapper

Ensure firestore logs show in dev event logs view.

Removed redundant bindings.

Remove FakeAuthenticationController and refactored its usages to the new FakeFirebaseWrapperImpl

Remove DebugFirestoreEventLogger.kt and usages.

Update AuthenticationControllerTest tests

Fix static analysis checks

Fix bazel build errors

Reformat auth/BUILD.bazel

Create wrapper for firebase auth instance

Fix lint errors

Fix failing tests

Fix failing lint checks

Fix test failures

Fix BUILD file formatting

Fix DebugFirestoreEventLoggerImplTest

Fake a firebase instance.

Fix build failures

Fix test file exemption warnings

Fix bazel build input file

Fix bazel build input file

Fix failing tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. good first issue This item is good for new contributors to make their pull request. Impact: Low Low perceived user impact (e.g. edge cases). Work: Low Solution is clear and broken into good-first-issue-sized chunks.
6 participants