oppia / oppia-android

A free, online & offline learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
315 stars 516 forks source link

Landscape support for ItemSelectionInteraction [Blocked on #1737] #1595

Closed rt4914 closed 1 year ago

rt4914 commented 4 years ago

Describe the bug If learner has selected few options in ItemSelectionInteraction and does configuration change the selection disappears.

To Reproduce Steps to reproduce the behavior:

  1. Go to Prototype Exploration

  2. Go to below mentioned state: Screenshot_1597062466

  3. Select 2-3 options

  4. Change device configuration

  5. Notice that the selection resets.

Expected behavior ItemSelectionInteraction should persist selected items on configuration change. Fix this for question and exploration player.

prayutsu commented 4 years ago

@rt4914 I want to work on this issue.

rt4914 commented 4 years ago

@prayutsu This can be a tricky task but considering that you have finished almost 3 PRs, I am assigning this to you. If you get stuck feel free to mention and we will deassign you.

prayutsu commented 4 years ago

@prayutsu This can be a tricky task but considering that you have finished almost 3 PRs, I am assigning this to you. If you get stuck feel free to mention and we will deassign you.

Thank you so much for assigning me the issue and yes, I will surely need your help @rt4914

prayutsu commented 4 years ago

@rt4914 If we could wrap the options selected in a LiveData object. Then the checkboxes will survive configuration change. I explored the directories and packages. But I need a little help in identifying the correct files (like SelectionInteractionViewModel, SelectionInteractionContentViewModel, QuestionPlayerViewModel, etc) where I should try making changes. It would be a pleasure if you could do some help

aggarwalpulkit596 commented 4 years ago

@prayutsu things won't work with live data because they cannot retain values on configuration change.

aggarwalpulkit596 commented 4 years ago

And you have to make changed ItemSelectionInteractionView

prayutsu commented 4 years ago

@prayutsu things won't work with live data because they cannot retain values on configuration change.

And you have to make changed ItemSelectionInteractionView

Thank you @aggarwalpulkit596 for your help.

prayutsu commented 4 years ago

@aggarwalpulkit596 I searched the file "ItemSelectionInteractionView" and found only this most closely matched file -

src/main/java/org/oppia/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt

Is this what you are referring to?

aggarwalpulkit596 commented 4 years ago

Yes that's correct

prayutsu commented 4 years ago

Yes that's correct

Thank you for that @aggarwalpulkit596 I want an extra help. I am trying to install ktlint and bufbuild on my ubuntu system and for ktlint my terminal says -

**error: The publisher of snap "ktlint" has indicated that they do not consider
       this revision to be of production quality and that it is only meant for
       development or testing at this point. As a consequence this snap will
       not refresh automatically and may perform arbitrary system changes
       outside of the security sandbox snaps are generally confined to, which
       may put your system at risk.
       If you understand and want to proceed repeat the command including
       --devmode; if instead you want to install the snap forcing it into
       strict confinement repeat the command including --jailmode.**

I also tried another command which is there on their official website - curl -sSLO https://github.com/pinterest/ktlint/releases/download/0.38.1/ktlint && chmod a+x ktlint

But nothing happens, there's not any logs in the terminal and the command isn't executed completely. Screenshot from 2020-08-26 23-03-32

BenHenning commented 4 years ago

@vinitamurthi or @anandwana001 is this something that you can help with?

anandwana001 commented 4 years ago

Yes that's correct

Thank you for that @aggarwalpulkit596 I want an extra help. I am trying to install ktlint and bufbuild on my ubuntu system and for ktlint my terminal says -

**error: The publisher of snap "ktlint" has indicated that they do not consider
       this revision to be of production quality and that it is only meant for
       development or testing at this point. As a consequence this snap will
       not refresh automatically and may perform arbitrary system changes
       outside of the security sandbox snaps are generally confined to, which
       may put your system at risk.
       If you understand and want to proceed repeat the command including
       --devmode; if instead you want to install the snap forcing it into
       strict confinement repeat the command including --jailmode.**

I also tried another command which is there on their official website - curl -sSLO https://github.com/pinterest/ktlint/releases/download/0.38.1/ktlint && chmod a+x ktlint

But nothing happens, there's not any logs in the terminal and the command isn't executed completely. Screenshot from 2020-08-26 23-03-32

As per the wiki mentioned, we are currently using 0.37.0 version of the ktlint. Please check with this version and let me know if the same issue occurs.

prayutsu commented 4 years ago

@aggarwalpulkit596 I am trying to solve this issue by using savedStateHandle in SelectionInterActionViewModel We can save the currently selected options in a key-value pair. And after configuration changes, when the ViewModel is created again, in the init block we can check if the state is not null, we use the stored values in selectedItems. This should work, right?

prayutsu commented 4 years ago

@anandwana001 Even after trying version 0.37.0, still the same thing is happening. Screenshot from 2020-08-27 12-05-35

anandwana001 commented 4 years ago

@anandwana001 Even after trying version 0.37.0, still the same thing is happening. Screenshot from 2020-08-27 12-05-35

Just to confirm before I look into what's the issue, apart from ktlint does curl commands working on your machine? Also, have you tried with brew - brew install ktlint

prayutsu commented 4 years ago

@anandwana001 Even after trying version 0.37.0, still the same thing is happening. Screenshot from 2020-08-27 12-05-35

Just to confirm before I look into what's the issue, apart from ktlint does curl commands working on your machine? Also, have you tried with brew - brew install ktlint

yes, they work

anandwana001 commented 4 years ago

@anandwana001 Even after trying version 0.37.0, still the same thing is happening. Screenshot from 2020-08-27 12-05-35

Just to confirm before I look into what's the issue, apart from ktlint does curl commands working on your machine? Also, have you tried with brew - brew install ktlint

yes, they work

ok let me see, The common issue always is this command sets up the file which requires stable and strong internet connection.

prayutsu commented 4 years ago

@anandwana001 Please take a look on this Screenshot from 2020-08-27 13-03-37

anandwana001 commented 4 years ago

Could you try this one - https://snapcraft.io/install/ktlint/ubuntu

prayutsu commented 4 years ago

Could you try this one - https://snapcraft.io/install/ktlint/ubuntu

Screenshot from 2020-08-27 13-10-28

anandwana001 commented 4 years ago

ah, snap doesn't have the latest version of ktlint and that's why ignoring to install the older version. This simply curl -sSLO https://github.com/pinterest/ktlint/releases/download/0.37.0/ktlint && chmod a+x ktlint should work. Might be internet speed is a bit slow!! But, after running this command this will show you nothing, just start the next line to take input of other commands. Have you checked ktlint --version?

aggarwalpulkit596 commented 4 years ago

@aggarwalpulkit596 I am trying to solve this issue by using savedStateHandle in SelectionInterActionViewModel We can save the currently selected options in a key-value pair. And after configuration changes, when the ViewModel is created again, in the init block we can check if the state is not null, we use the stored values in selectedItems. This should work, right?

this should work but i don't think so it would that straight forward because we don't have state handle inside viewmodels also these view models are different from the view models that you might bee using in daily android development.

prayutsu commented 4 years ago

ah, snap doesn't have the latest version of ktlint and that's why ignoring to install the older version. This simply curl -sSLO https://github.com/pinterest/ktlint/releases/download/0.37.0/ktlint && chmod a+x ktlint should work. Might be internet speed is a bit slow!! But, after running this command this will show you nothing, just start the next line to take input of other commands. Have you checked ktlint --version?

I tried it says, Screenshot from 2020-08-27 13-30-18

prayutsu commented 4 years ago

@aggarwalpulkit596 I am trying to solve this issue by using savedStateHandle in SelectionInterActionViewModel We can save the currently selected options in a key-value pair. And after configuration changes, when the ViewModel is created again, in the init block we can check if the state is not null, we use the stored values in selectedItems. This should work, right?

this should work but i don't think so it would that straight forward because we don't have state handle inside viewmodels also these view models are different from the view models that you might bee using in daily android development.

So, could you suggest some other better way @aggarwalpulkit596 ?

anandwana001 commented 4 years ago

@prayutsu Give this a try, I tried in one of the ubuntu setups and works fine. if this doesn't work let me check on a few other solutions.

curl -sSLO https://github.com/pinterest/ktlint/releases/download/0.37.0/ktlint && chmod a+x ktlint && sudo mv ktlint /usr/local/bin/
aggarwalpulkit596 commented 4 years ago

@aggarwalpulkit596 I am trying to solve this issue by using savedStateHandle in SelectionInterActionViewModel We can save the currently selected options in a key-value pair. And after configuration changes, when the ViewModel is created again, in the init block we can check if the state is not null, we use the stored values in selectedItems. This should work, right?

this should work but i don't think so it would that straight forward because we don't have state handle inside viewmodels also these view models are different from the view models that you might bee using in daily android development.

So, could you suggest some other better way @aggarwalpulkit596 ?

That's probably the only way. There's no other way around for this problem.

prayutsu commented 4 years ago

@aggarwalpulkit596 Thanks for the help.

aggarwalpulkit596 commented 4 years ago

It would lead to certain changes and wouldn't be easy to do especially when you're not familiar with project and that's why i suggested to take some up easy issue that will make you familiar with project but still if you'd like to take this one up i will be more than happy to help you. 🙂

prayutsu commented 4 years ago

@prayutsu Give this a try, I tried in one of the ubuntu setups and works fine. if this doesn't work let me check on a few other solutions.

curl -sSLO https://github.com/pinterest/ktlint/releases/download/0.37.0/ktlint && chmod a+x ktlint && sudo mv ktlint /usr/local/bin/

Thank you so much @anandwana001. It worked.

prayutsu commented 4 years ago

It would lead to certain changes and wouldn't be easy to do especially when you're not familiar with project and that's why i suggested to take some up easy issue that will make you familiar with project but still if you'd like to take this one up i will be more than happy to help you.

@aggarwalpulkit596 Thank you very much. So, should I push my changes once? So, you can have a look at it once.

aggarwalpulkit596 commented 4 years ago

@prayutsu sure. https://proandroiddev.com/connecting-the-dots-44d8fa79f14 you could also read this thing if you want some context.

aggarwalpulkit596 commented 4 years ago

@prayutsu https://github.com/hansenji/ViewModelInject this might be more appropriate but as we would be moving to dagger hilt idk doing this would be feasible or not. @rt4914 your thoughts?

prayutsu commented 4 years ago

@aggarwalpulkit596 Can you please take a look #1731

aggarwalpulkit596 commented 4 years ago

Done.

On Thu, Aug 27, 2020, 2:18 PM Abhay Garg notifications@github.com wrote:

@aggarwalpulkit596 https://github.com/aggarwalpulkit596 Can you please take a look #1731 https://github.com/oppia/oppia-android/pull/1731

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oppia/oppia-android/issues/1595#issuecomment-681808500, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG6KGSRB4BLWZXDUZP2XKPLSCYMUZANCNFSM4PZ5C6QA .

rt4914 commented 4 years ago

@anandwana001 @aggarwalpulkit596 @prayutsu

Please avoid having conversations on issues, instead you can chat in gitter channel or on email. Thanks

prayutsu commented 4 years ago

@rt4914 I am removing my assignment for this issue, so can you please assign me new issues?

rt4914 commented 1 year ago

Duplicate #4472