-
Notifications
You must be signed in to change notification settings - Fork 73
Fixes On screen rotation selected forms get unselected bug #124
base: master
Are you sure you want to change the base?
Conversation
This PR is not ready yet. I needed some help, that's the reason I've posted this PR to show you the work that has been done yet. Also, it'd be good to know if I'm going in the right direction or not. @lakshyagupta21 @shobhitagarwal1612 |
share_app/build.gradle
Outdated
def lifecycle_version = "2.0.0" | ||
|
||
// ViewModel and LiveData | ||
implementation "androidx.lifecycle:lifecycle-extensions:$lifecycle_version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this issue can be resolved without using this, any other use case for which you want to use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to use ViewModel for this specific issue. What alternate method you think could be implemented? Any hints would be really appreciated.
@iadeelzafar Did you try to save the selected list of items and restore it after configuration changes via the |
I tried that, here's how the code looked like:
|
So the above snippet didn't work? |
This line was causing the app to crash.
I think the problem here was that I was not getting the right view and accordingly the checkbox. Have a look at this. |
You can check this code here: Line no. 88 and 89 are the ones causing the problem and I'm having a hard time getting a fix for these two lines. |
@iadeelzafar Try this
We need to update selectedInstances value with the selected items, and when |
5585d95
to
09cb7c8
Compare
@lakshyagupta21 Thanks a lot! This worked and it has also less boiler plate code. I've updated the PR. |
09cb7c8
to
f8dd7b6
Compare
@shobhitagarwal1612 @lakshyagupta21 Could you please review this? Required changes have been made. |
@iadeelzafar Looks like the Circle CI build is failing (link) |
@shobhitagarwal1612 I've rebased it into master so now, there are no merge conflicts. I'm unsure why the CircleCI "build_debug" is failing.
I'm unable to open this report. |
Here you go https://266-133386452-gh.circle-artifacts.com/0/reports/pmd/pmd.html Alternatively, you can also run the check locally. |
@shobhitagarwal1612 I've resolved the issues that were in PMD file earlier. But the CI Build is failing again and here's how the PMD file looks now. The problem page it shows is unclickable. |
@iadeelzafar Have tried running this command locally |
Got it. Reformatting is needed. |
4f3f0f9
to
03ae0a8
Compare
@lakshyagupta21 @shobhitagarwal1612 All checks have passed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see in the attached screenshot, the form is selected but the buttons aren't proper. Since this change selects the forms then the buttons must be fixed along with it as well
Thanks for your patience and nice work!
@shobhitagarwal1612 Oh! Thanks for pointing this out. I've made these changes. |
It is still showing the same results. Please check again |
skunkworks_crow/src/main/java/org/odk/share/fragments/BlankFormsFragment.java
Outdated
Show resolved
Hide resolved
skunkworks_crow/src/main/java/org/odk/share/fragments/FilledFormsFragment.java
Outdated
Show resolved
Hide resolved
@shobhitagarwal1612 Did you review the changes? |
Sorry for the delayed response. I still noticed a bug when rotating the screen. |
d3a2d36
to
994e521
Compare
@shobhitagarwal1612 I'm looking into it. I'll update soon. |
You can just leave a comment when it's ready for another round of review |
Any updates on this? |
@lakshyagupta21 @shobhitagarwal1612 Review the recent commit. I made changes to save state of the |
Don't save the state of toggle button and it's text. Regenerate that based on selected forms and total forms |
Alright. I'll look into this in a few days and make the changes soon. |
Apologies for being inactive on this issue. |
Build is failing because of using Log instead of Timber. I'll remove the log as soon as the logic works. |
@iadeelzafar Are you still working on this? If you're facing any difficulty let us know we'll help you out. |
@lakshyagupta21 read my last comment. I wrote what I need assistance with. |
Check this code
Here it's checking the item count value received when the cursor has finished loading. So you may have to change the logic here to fix the issue. |
Also resolve the conflicts. |
@lakshyagupta21 This code is causing the app to crash. I think you should try cloning my branch and have a look into the code that I've written. |
Which code you're talking about here? And if it's crashing on master then please report this as a new issue. And I checked out your branch only and then saw that the above code is the root cause of the issue that you're facing. |
Currently, if we click on |
Any updates on this PR? |
@lakshyagupta21 I think we should break it in two parts: First part has been completed. The code for the second part is a lil messy. I'll remove that. And then anyone who'd like to work on the second part can complete it. What you think? |
We can't split that into two parts because if first thing is done and merged, and other stays in a pending state then it would be in an inconsistent state, it's better to do that in one PR. |
Fixes: #120
Changes Introduced:
GIF Showing The Changes: