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 part of #161 : Introduce html parser #226

Closed
wants to merge 62 commits into from

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Oct 11, 2019

Explanation

This PR supports rich-text. It processes Oppia's custom HTML tags and displays the styled text in the textview.
Note: It doesn't support latex.

Reference Design Doc:
https://docs.google.com/document/d/1Knx8KIPN7Amqav2wu-jvSKd5RjolS-taqRF3eUNSDu8/edit?usp=sharing

BenHenning and others added 27 commits September 24, 2019 23:47
tests to verify correctness.

Also, added a method to facilitate notifying of DataProvider changes on
the UI thread.
… it up to

the data controller, and start adding tests.
Conflicts:
	domain/build.gradle
	domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt

Also, migrate the data controller to the retriever.
AnswerClassificationController, and attempted to make
ExplorationProgressController thread-safe.

The thread-safety led to significant interface changes in the progress
controller, and led to discovering some issues with the mediator live data
approach to interop coroutines and LiveData. This locking mechanism will
need to change since the optimal solution requires resolving #90.
with the current MediatorLiveData implementation (see #90 for more
context). Fix existing progress controller tests and add a few more. All
current progress controller tests are passing.
…ation

support for the second test exploration (about_oppia).
…-controller' into introduce-load-exploration-part-1
Separated this in its own PR.
@veena14cs veena14cs changed the base branch from develop to exploration-player-content-interaction October 11, 2019 12:19
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

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

Please update the test case

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

Generally LGTM.

@BenHenning PTAL

@rt4914 rt4914 removed their assignment Oct 17, 2019
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @veena14cs. I had some confusion about the test logic itself, but the overall approach LGTM. Feel free to submit once my comments are addressed, or re-assign to me if you'd like me to do one more pass.

@BenHenning BenHenning assigned veena14cs and unassigned BenHenning Oct 18, 2019
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @veena14cs--there appear to be a few test files here that are being fully commented out. I understand that's needed to unblock running them locally, but please don't check them in like that.

Also, I think there are some unresolved comment threads that I left earlier regarding the need for the requestedOrientation bits. Please address those comments.

@BenHenning BenHenning assigned veena14cs and unassigned BenHenning Oct 18, 2019
@veena14cs
Copy link
Contributor Author

Dagger implementation of this is done in PR #205 , hence closing this PR.

@veena14cs veena14cs closed this Oct 22, 2019
@veena14cs veena14cs deleted the introduce-html-parser branch October 22, 2019 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants