Fix #153/#154 : MultipleChoice/ItemSelection Interaction View #188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
veena14cs
requested review from
BenHenning,
jamesxu0,
nikitamarysolomanpvt and
rt4914
and removed request for
jamesxu0
veena14cs
changed the title
veena14cs
changed the title
|
Your code for checkbox and radio button works correct. Great job. I have left some comments which you can resolve. Also, you are using Also, create a customview which handles your two views, checkout @nikitamarysolomanpvt PR on FractionInput, she has created a separate Write test-cases for this code. |
rt4914
assigned veena14cs and nikitamarysolomanpvt and unassigned rt4914, BenHenning and nikitamarysolomanpvt
|
@veena14cs your branch name contains '()' brackets, do not add brackets to a branch name, it is wrong convention. Also, if the branch name contains |
rt4914
reviewed
Sep 27, 2019
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragment.kt
Outdated
Show resolved
Hide resolved
...rc/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
...rc/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
...rc/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
rt4914
reviewed
Sep 27, 2019
|
Moved Interaction implementation to statefragment.
…On Fri, Sep 27, 2019 at 8:52 AM Rajat Talesra ***@***.***> wrote:
@veena14cs <https://github.com/veena14cs>
Your code for checkbox and radio button works correct. Great job.
I have left some comments which you can resolve.
Also, you are using ContentInteractionFragment just for showing this demo
and it actually serves no other purpose. And we know that we have to use
these two views in the StateFragment, so I suggest you remove
ContentInteractionFragment and shift your implementation in StateFragment.
Write test-cases for this code.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#188?email_source=notifications&email_token=AD4LXZGJTKBOLDDPRXRFAF3QLV4ABA5CNFSM4I2WFUPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7XTKUA#issuecomment-535770448>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD4LXZB4UFTSLCINF46ZV53QLV4ABANCNFSM4I2WFUPA>
.
|
|
used the terms MultipleChoiceViewInteraction and ItemSelectionViewInteraction
for interactions.
But these terms are really creating confusion though it is according to
webcode base, in my opinion we have to change these terms according to
their definations
…On Fri, Sep 27, 2019 at 8:55 AM Rajat Talesra ***@***.***> wrote:
***@***.**** commented on this pull request.
PTAL
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragment.kt
<#188 (comment)>:
> @@ -0,0 +1,26 @@
+package org.oppia.app.player.content_interaction
+
+import android.content.Context
+import android.os.Bundle
+import android.view.LayoutInflater
+import android.view.View
+import android.view.ViewGroup
+import org.oppia.app.fragment.InjectableFragment
+import org.oppia.app.player.content_interaction.ContentInteractionFragmentPresenter
+import javax.inject.Inject
+
+/** Fragment displays single/multiplechoice input interaction. */
Do not you single or singlechoice or singlechoiceinput anywhere, in our
discussion that was just for communication.
If you are using SingleChoiceInput -> that by default developer will
understand that in this radio buttons should be visible and leaner can
select only one correct answer. This then drives the assumption that
MultipleChoiceInput will show checkboxes and learner can select multiple
answers.
This is incorrect in terms of definition as well as oppia-web codebase.
Use ItemSelectionInput or ItemSelectionViewInteraction whichever fits the
name, this means that learner can select multiple answers at a time in
backend code.
Use MultipleChoiceInput or MultipleChoiceViewInteraction, this means that
learner can select one answer among multiple options, as per backend code.
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragment.kt
<#188 (comment)>:
> @@ -0,0 +1,26 @@
+package org.oppia.app.player.content_interaction
+
+import android.content.Context
+import android.os.Bundle
+import android.view.LayoutInflater
+import android.view.View
+import android.view.ViewGroup
+import org.oppia.app.fragment.InjectableFragment
+import org.oppia.app.player.content_interaction.ContentInteractionFragmentPresenter
+import javax.inject.Inject
+
+/** Fragment displays single/multiplechoice input interaction. */
+class ContentInteractionFragment : InjectableFragment() {
+
Nit: Remove this empty line.
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> +import org.oppia.app.databinding.ContentInteractionFragmentBinding
+import javax.inject.Inject
+import android.widget.RadioButton
+import android.widget.LinearLayout
+import android.widget.RadioGroup
+import android.widget.TextView
+import android.widget.Toast
+import kotlinx.android.synthetic.main.content_interaction_fragment.view.*
+import org.oppia.util.data.UrlImageParser
+import android.widget.CompoundButton
+
+/** Presenter for [ContentInteractionFragment]. */
+class ContentInteractionFragmentPresenter @Inject constructor(
+ @ApplicationContext private val context: Context,
+ private val fragment: Fragment
+// private val interactionInstanceId: String, val gaeCustomizationArgs: Map<String, GaeCustomizationArgs>
Remove this comment and unused variable
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> +/** Presenter for [ContentInteractionFragment]. */
+class ContentInteractionFragmentPresenter @Inject constructor(
+ @ApplicationContext private val context: Context,
+ private val fragment: Fragment
+// private val interactionInstanceId: String, val gaeCustomizationArgs: Map<String, GaeCustomizationArgs>
+) {
+
+ private lateinit var binding: ContentInteractionFragmentBinding
+
+ fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? {
+
+ binding = ContentInteractionFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false)
+
+ showInputInteractions()
+
+ return binding.root
Remove extra empty spaces.
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> +// private val interactionInstanceId: String, val gaeCustomizationArgs: Map<String, GaeCustomizationArgs>
+) {
+
+ private lateinit var binding: ContentInteractionFragmentBinding
+
+ fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? {
+
+ binding = ContentInteractionFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false)
+
+ showInputInteractions()
+
+ return binding.root
+ }
+
+ private fun showInputInteractions() {
+
Remove extra empty space
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> +) {
+
+ private lateinit var binding: ContentInteractionFragmentBinding
+
+ fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? {
+
+ binding = ContentInteractionFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false)
+
+ showInputInteractions()
+
+ return binding.root
+ }
+
+ private fun showInputInteractions() {
+
+ //Todo(Veena): Remove static initialization and use values from constructor for gaeCustomizationArgsMap and interactionInstanceId
Space just before TODO.
This should be
// Todo(Veena): Remove static ...
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> + private val fragment: Fragment
+// private val interactionInstanceId: String, val gaeCustomizationArgs: Map<String, GaeCustomizationArgs>
+) {
+
+ private lateinit var binding: ContentInteractionFragmentBinding
+
+ fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? {
+
+ binding = ContentInteractionFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false)
+
+ showInputInteractions()
+
+ return binding.root
+ }
+
+ private fun showInputInteractions() {
It would be good, if your method accepts whatever parameters it needs and
then create the UI. So that, when we work on final implementation we can
directly use your method.
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> + if (interactionInstanceId.equals("MultipleChoiceInput")) {
+ val gaeCustomArgsInString: String = gaeCustomizationArgs.toString().replace("[", "").replace("]", "")
+ var items = gaeCustomArgsInString.split(",").toTypedArray()
+ addRadioButtons(items)
+ } else if (interactionInstanceId.equals("ItemSelectionInput") || interactionInstanceId.equals("SingleChoiceInput")) {
+ val gaeCustomArgsInString: String = gaeCustomizationArgs.toString().replace("[", "").replace("]", "")
+ var items = gaeCustomArgsInString.split(",").toTypedArray()
+ addCheckbox(items)
+ } else {
+ //Do no show any view
+ }
+ }
+
+ fun addCheckbox(optionsArray: Array<String>) {
+ for (row in 0..0) {
+ val ll = LinearLayout(context)
Rather than using ll using proper variable names like checkboxLinearLayout
or rootLinearLayout or simply linearLayout
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> + } else if (interactionInstanceId.equals("ItemSelectionInput") || interactionInstanceId.equals("SingleChoiceInput")) {
+ val gaeCustomArgsInString: String = gaeCustomizationArgs.toString().replace("[", "").replace("]", "")
+ var items = gaeCustomArgsInString.split(",").toTypedArray()
+ addCheckbox(items)
+ } else {
+ //Do no show any view
+ }
+ }
+
+ fun addCheckbox(optionsArray: Array<String>) {
+ for (row in 0..0) {
+ val ll = LinearLayout(context)
+ ll.orientation = LinearLayout.VERTICAL
+
+ for (i in 0..optionsArray.size - 1) {
+ val cb = CheckBox(context)
Change variable name as per above comment.
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> + cb.text = convertHtmlToString(optionsArray[i], cb).toString()
+
+ cb.setOnCheckedChangeListener(CompoundButton.OnCheckedChangeListener { buttonView, isChecked ->
+ val msg = "You have " + (if (isChecked) "checked" else "unchecked") + " this Check it Checkbox."
+ Toast.makeText(context, msg, Toast.LENGTH_SHORT).show()
+ })
+
+ ll.addView(cb)
+ }
+ binding.root.radioGroup.addView(ll)
+ }
+ }
+
+ fun addRadioButtons(optionsArray: Array<String>) {
+ for (row in 0..0) {
+ val rg = RadioGroup(context)
Nit: Variable name
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> + Toast.makeText(context, msg, Toast.LENGTH_SHORT).show()
+ })
+
+ ll.addView(cb)
+ }
+ binding.root.radioGroup.addView(ll)
+ }
+ }
+
+ fun addRadioButtons(optionsArray: Array<String>) {
+ for (row in 0..0) {
+ val rg = RadioGroup(context)
+ rg.orientation = LinearLayout.VERTICAL
+
+ for (i in 0..optionsArray.size - 1) {
+ val rdbtn = RadioButton(context)
Nit: Variable name
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> +
+ ll.addView(cb)
+ }
+ binding.root.radioGroup.addView(ll)
+ }
+ }
+
+ fun addRadioButtons(optionsArray: Array<String>) {
+ for (row in 0..0) {
+ val rg = RadioGroup(context)
+ rg.orientation = LinearLayout.VERTICAL
+
+ for (i in 0..optionsArray.size - 1) {
+ val rdbtn = RadioButton(context)
+ rdbtn.id = View.generateViewId()
+ rdbtn.text = convertHtmlToString(optionsArray[i], rdbtn).toString()// + rdbtn.id
Remove comment if not needed
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> + val rdbtn = RadioButton(context)
+ rdbtn.id = View.generateViewId()
+ rdbtn.text = convertHtmlToString(optionsArray[i], rdbtn).toString()// + rdbtn.id
+ rdbtn.gravity = (Gravity.RIGHT); (Gravity.CENTER)
+
+ rg.setOnCheckedChangeListener(object : RadioGroup.OnCheckedChangeListener {
+ override fun onCheckedChanged(group: RadioGroup, checkedId: Int) {
+ for (i in 0 until group.childCount) {
+ val btn = group.getChildAt(i) as RadioButton
+ if (btn.id == checkedId) {
+ val text = btn.text
+ Toast.makeText(context, "" + text, Toast.LENGTH_LONG).show()
+ return
+ }
+ }
+
Remove this extra space
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> + val text = btn.text
+ Toast.makeText(context, "" + text, Toast.LENGTH_LONG).show()
+ return
+ }
+ }
+
+ }
+ })
+ rg.addView(rdbtn)
+ }
+ binding.root.radioGroup.addView(rg)
+ }
+ }
+
+ fun convertHtmlToString(rawResponse: Any?, rdbtn: View): Spanned {
+
Remove this extra space
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> + }
+ })
+ rg.addView(rdbtn)
+ }
+ binding.root.radioGroup.addView(rg)
+ }
+ }
+
+ fun convertHtmlToString(rawResponse: Any?, rdbtn: View): Spanned {
+
+ var htmlContent = rawResponse as String;
+ var result: Spanned
+ var CUSTOM_TAG = "oppia-noninteractive-image"
+ var HTML_TAG = "img"
+ var CUSTOM_ATTRIBUTE = "filepath-with-value"
+ var HTML_ATTRIBUTE = "src"
I think all these variables can be converted to val. But better you define
all these as const val before above the entire-class-code
------------------------------
In
app/src/main/java/org/oppia/app/player/content_interaction/ContentInteractionFragmentPresenter.kt
<#188 (comment)>:
> + }
+ binding.root.radioGroup.addView(rg)
+ }
+ }
+
+ fun convertHtmlToString(rawResponse: Any?, rdbtn: View): Spanned {
+
+ var htmlContent = rawResponse as String;
+ var result: Spanned
+ var CUSTOM_TAG = "oppia-noninteractive-image"
+ var HTML_TAG = "img"
+ var CUSTOM_ATTRIBUTE = "filepath-with-value"
+ var HTML_ATTRIBUTE = "src"
+
+ if (htmlContent!!.contains(CUSTOM_TAG)) {
+
Remove this extra line.
------------------------------
In utility/src/main/java/org/oppia/util/data/URlImageParser.kt
<#188 (comment)>:
> +import android.content.Context
+import android.graphics.Bitmap
+import android.graphics.Canvas
+import android.graphics.Rect
+import android.graphics.drawable.BitmapDrawable
+import android.graphics.drawable.Drawable
+import android.graphics.drawable.LevelListDrawable
+import android.text.Html
+import android.widget.TextView
+import com.bumptech.glide.Glide
+import com.bumptech.glide.request.target.SimpleTarget
+import com.bumptech.glide.request.transition.Transition
+import java.net.URL
+
+/** UrlImage Parser for android TextView to load Html Image tag */
+
Remove this line. Also add . in the comment
/** UrlImage Parser for android TextView to load Html Image tag. */
------------------------------
In utility/src/main/java/org/oppia/util/data/URlImageParser.kt
<#188 (comment)>:
> + override fun getDrawable(urlString: String): Drawable {
+ val urlDrawable = UrlDrawable()
+ val load = Glide.with(context).asBitmap().load(URL(urlString))
+ val target = BitmapTarget(urlDrawable)
+ targets?.add(target)
+ load.into(target)
+ return urlDrawable
+ }
+
+ inner class BitmapTarget(private val urlDrawable: UrlDrawable) : SimpleTarget<Bitmap>() {
+ internal var drawable: Drawable? = null
+
+ override fun onResourceReady(resource: Bitmap, transition: Transition<in Bitmap>?) {
+
+ drawable = BitmapDrawable(context.resources, resource)
+
Nit: Remove extra space
------------------------------
In utility/src/main/java/org/oppia/util/data/URlImageParser.kt
<#188 (comment)>:
> + * @return
+ */
+ override fun getDrawable(urlString: String): Drawable {
+ val urlDrawable = UrlDrawable()
+ val load = Glide.with(context).asBitmap().load(URL(urlString))
+ val target = BitmapTarget(urlDrawable)
+ targets?.add(target)
+ load.into(target)
+ return urlDrawable
+ }
+
+ inner class BitmapTarget(private val urlDrawable: UrlDrawable) : SimpleTarget<Bitmap>() {
+ internal var drawable: Drawable? = null
+
+ override fun onResourceReady(resource: Bitmap, transition: Transition<in Bitmap>?) {
+
Nit: Remove extra space
------------------------------
In utility/src/main/java/org/oppia/util/data/URlImageParser.kt
<#188 (comment)>:
> + return urlDrawable
+ }
+
+ inner class BitmapTarget(private val urlDrawable: UrlDrawable) : SimpleTarget<Bitmap>() {
+ internal var drawable: Drawable? = null
+
+ override fun onResourceReady(resource: Bitmap, transition: Transition<in Bitmap>?) {
+
+ drawable = BitmapDrawable(context.resources, resource)
+
+ tvContents.post {
+ val w = tvContents.width
+ val hh = (drawable as BitmapDrawable).intrinsicHeight
+ val ww = (drawable as BitmapDrawable).intrinsicWidth
+ val newHeight = hh * w / ww
+ val rect = Rect(0, 0, w, newHeight)
Name variables correctly
------------------------------
In utility/src/main/java/org/oppia/util/data/URlImageParser.kt
<#188 (comment)>:
> +
+ drawable = BitmapDrawable(context.resources, resource)
+
+ tvContents.post {
+ val w = tvContents.width
+ val hh = (drawable as BitmapDrawable).intrinsicHeight
+ val ww = (drawable as BitmapDrawable).intrinsicWidth
+ val newHeight = hh * w / ww
+ val rect = Rect(0, 0, w, newHeight)
+ (drawable as BitmapDrawable).bounds = rect
+ urlDrawable.bounds = rect
+ urlDrawable.drawable = drawable
+ tvContents.text = tvContents.text
+ tvContents.invalidate()
+ }
+
Nit: Remove extra space
------------------------------
In utility/src/main/java/org/oppia/util/data/URlImageParser.kt
<#188 (comment)>:
> + urlDrawable.drawable = drawable
+ tvContents.text = tvContents.text
+ tvContents.invalidate()
+ }
+
+ }
+ }
+
+ inner class UrlDrawable : BitmapDrawable() {
+ var drawable: Drawable? = null
+ override fun draw(canvas: Canvas) {
+ if (drawable != null)
+ drawable!!.draw(canvas)
+ }
+ }
+
Nit: Remove extra space
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#188?email_source=notifications&email_token=AD4LXZHNFWPYZUXSAIB6LDDQLV4I5A5CNFSM4I2WFUPKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGDYYNQ#pullrequestreview-294095926>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD4LXZCR5PUOLMRIUVTK5ALQLV4I5ANCNFSM4I2WFUPA>
.
|
|
corrected indentation
…On Fri, Sep 27, 2019 at 9:04 AM Rajat Talesra ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/src/main/res/layout/content_interaction_fragment.xml
<#188 (comment)>:
> @@ -0,0 +1,14 @@
+<layout xmlns:android="http://schemas.android.com/apk/res/android">
+ <LinearLayout
+ android:id="@+id/rootContainer"
+ android:layout_width="match_parent"
+ android:layout_height="match_parent"
+ android:orientation="vertical">
+ <RadioGroup
+ android:id="@+id/radioGroup"
+ android:layout_width="match_parent"
+ android:layout_height="wrap_content"
+ android:padding="15dp">
Indentation issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#188?email_source=notifications&email_token=AD4LXZCUXZ3XOW65HYMWBJDQLV5MXA5CNFSM4I2WFUPKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGDZZOI#pullrequestreview-294100153>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD4LXZA5TYG44NNCU3OPW53QLV5MXANCNFSM4I2WFUPA>
.
|
|
i understand that this naming convention is very confusing but it better if we get used to it so that we can communicate easily with @seanlip and @BenHenning , because they are used to these terms whereas we are completely new to this project and it is easier for us to follow this and difficult for them to change this naming now. |
rt4914
reviewed
Sep 27, 2019
app/src/main/java/org/oppia/app/player/exploration/ExplorationActivityPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/app/player/state/CustomRadioButton.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateFragmentPresenterTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateFragmentPresenterTest.kt
Outdated
Show resolved
Hide resolved
|
Duplicate of this PR is been created here #196 |
veena14cs
deleted the
exploration-player-3-content-interaction(replica)
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
This PR is duplicate of #184. It contains UI structure for Multiple choice and item selection interactions, and fixes #153 and #154.
Reference Design Doc: https://docs.google.com/document/d/1HFwDFcqenAXepbYr5rfawGHvkeBZpYvNMs228AWLn24/edit?usp=sharing