Fixes On screen rotation selected forms get unselected bug

[iadeelzafar]

Fixes: #120

Changes Introduced:

• Override onSaveInstanceState() method in both FilledFormsFragment.java and BlankFormsFragment.java
• Adding the logic to backup selectedInstances
• Restore the selectedInstances in onCreateView method of both fragments.

GIF Showing The Changes:

[iadeelzafar]

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
I'm getting an error at this line of code.
MainViewModel viewModel= ViewModelProviders.of(this).get(MainViewModel.class);
More specifically, here:
ViewModelProviders.of(this)
I've tried to call getFragmentManager and other similar functions but that's also not working here.

[lakshyagupta21]

this issue can be resolved without using this, any other use case for which you want to use this?

[iadeelzafar]

I was trying to use ViewModel for this specific issue. What alternate method you think could be implemented? Any hints would be really appreciated.

[shobhitagarwal1612]

@iadeelzafar Did you try to save the selected list of items and restore it after configuration changes via the
actiity's lifecycle methods onSaveInstanceState() and onRestoreInstanceState()?

[iadeelzafar]

I tried that, here's how the code looked like:

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container,
                             Bundle savedInstanceState) {
        setHasOptionsMenu(true);
        View view = inflater.inflate(R.layout.fragment_forms, container, false);
        ButterKnife.bind(this, view);
        selectedInstances = new LinkedHashSet<>();
        LinearLayoutManager llm = new LinearLayoutManager(getActivity());
        llm.setOrientation(LinearLayoutManager.VERTICAL);
        recyclerView.setLayoutManager(llm);

        if(savedInstanceState!=null)
        {
                backupSelectedInstances=savedInstanceState.getIntegerArrayList(SELECTED_INSTANCES_KEY);
                CheckBox checkBox;
                for(int i=0;i<backupSelectedInstances.size();i++)
                {
                    View newView=recyclerView.getChildAt(backupSelectedInstances.get(i));
                     //I was getting error at this line.
                    checkBox = newView.findViewById(R.id.checkbox);
                    checkBox.setChecked(true);
                }
        }
        return view;

    }

    @Override public void onSaveInstanceState(@NonNull Bundle outState) {
        super.onSaveInstanceState(outState);
        outState.putIntegerArrayList(SELECTED_INSTANCES_KEY,backupSelectedInstances);
        
    }

    private void onListItemClick(View view, int position) {
        Cursor cursor = instanceAdapter.getCursor();
        cursor.moveToPosition(position);
        
        
        CheckBox checkBox = view.findViewById(R.id.checkbox);
        checkBox.setChecked(!checkBox.isChecked());

        long id = cursor.getLong(cursor.getColumnIndex(InstanceProviderAPI.InstanceColumns._ID));

        if (selectedInstances.contains(id)) {
            selectedInstances.remove(id);
        } else {
            selectedInstances.add(id);
        }
        
        //Created this integer array list to store positions of selected instances.
        if(backupSelectedInstances.contains(position)){
            backupSelectedInstances.remove(position);
        }
        else
            backupSelectedInstances.add(position);

        sendButton.setEnabled(selectedInstances.size() > 0);

        toggleButtonLabel();
    }

[shobhitagarwal1612]

So the above snippet didn't work?

[iadeelzafar]

This line was causing the app to crash.

checkBox = newView.findViewById(R.id.checkbox);

I think the problem here was that I was not getting the right view and accordingly the checkbox. Have a look at this.

[iadeelzafar]

You can check this code here:
https://github.com/opendatakit/skunkworks-crow/compare/master...iadeelzafar:rotate?expand=1

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.

[lakshyagupta21]

@iadeelzafar Try this

@Override
    public void onSaveInstanceState(@NonNull Bundle outState) {
        super.onSaveInstanceState(outState);
        outState.putLongArray(SELECTED_INSTANCES_KEY, ArrayUtils.toPrimitive(
                selectedInstances.toArray(new Long[selectedInstances.size()])));
    }

@Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container,
                             Bundle savedInstanceState) {
        setHasOptionsMenu(true);
        View view = inflater.inflate(R.layout.fragment_forms, container, false);
        ButterKnife.bind(this, view);

        selectedInstances = new LinkedHashSet<>();

        LinearLayoutManager llm = new LinearLayoutManager(getActivity());
        llm.setOrientation(LinearLayoutManager.VERTICAL);
        recyclerView.setLayoutManager(llm);

        if (savedInstanceState != null) {
            long[] previousSelectedInstances = savedInstanceState.getLongArray(SELECTED_INSTANCES_KEY);
            for (int i = 0; i < previousSelectedInstances.length; i++) {
                selectedInstances.add(previousSelectedInstances[i]);
            }
        }

        return view;
    }

We need to update selectedInstances value with the selected items, and when onLoadFinished() adapter will automatically check the items according to the values stored in selectedInstances.

[iadeelzafar]

@lakshyagupta21 Thanks a lot! This worked and it has also less boiler plate code. I've updated the PR.

[iadeelzafar]

@shobhitagarwal1612 @lakshyagupta21 Could you please review this? Required changes have been made.

[shobhitagarwal1612]

@iadeelzafar Looks like the Circle CI build is failing (link)
Also, there are a couple of merge conflicts

[iadeelzafar]

@shobhitagarwal1612 I've rebased it into master so now, there are no merge conflicts. I'm unsure why the CircleCI "build_debug" is failing.

Execution failed for task ':skunkworks_crow:pmd'.
> 2 PMD rule violations were found. See the report at: file:///home/circleci/work/skunkworks_crow/build/reports/pmd/pmd.html

I'm unable to open this report.

[shobhitagarwal1612]

Here you go https://266-133386452-gh.circle-artifacts.com/0/reports/pmd/pmd.html
It is under artifacts tab

Alternatively, you can also run the check locally.
./gradlew pmd checkstyle lint findbugs

[iadeelzafar]

@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.

PMD file

[lakshyagupta21]

@iadeelzafar Have tried running this command locally ./gradlew pmd checkstyle lint findbugs as suggested by @shobhitagarwal1612.
Its always good to run this command before pushing any change, even your single line changes can fail the build. Currently, there are issues with checkstyle.
https://270-133386452-gh.circle-artifacts.com/0/reports/checkstyle/checkstyle.html

[iadeelzafar]

Got it. Reformatting is needed.

[iadeelzafar]

@lakshyagupta21 @shobhitagarwal1612 All checks have passed now.

[shobhitagarwal1612]

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!

[iadeelzafar]

@shobhitagarwal1612 Oh! Thanks for pointing this out. I've made these changes.

[shobhitagarwal1612]

It is still showing the same results. Please check again

[iadeelzafar]

@shobhitagarwal1612 Did you review the changes?

[shobhitagarwal1612]

Sorry for the delayed response.

I still noticed a bug when rotating the screen.
Before rotating, I selected all forms in Blank Forms screen. The button's label changes from Select All to Clear All
After rotation, the form is still selected but the button label changed to Select All

[iadeelzafar]

@shobhitagarwal1612 I'm looking into it. I'll update soon.

[shobhitagarwal1612]

You can just leave a comment when it's ready for another round of review

[lakshyagupta21]

Any updates on this?

[iadeelzafar]

@lakshyagupta21 @shobhitagarwal1612 Review the recent commit. I made changes to save state of the toggleButton. But this solution didn't worked. Have a look at it.

[shobhitagarwal1612]

Don't save the state of toggle button and it's text. Regenerate that based on selected forms and total forms

[iadeelzafar]

Alright. I'll look into this in a few days and make the changes soon.

[iadeelzafar]

Apologies for being inactive on this issue.
So, I worked on this again and now I've added the logic for toggle button's label to update but it's not working.
When we select all forms and screen is rotated, when I try to log the toggle button's label using
Log.v("DANG ", toggleButton.getText().toString());
On the verbose, it is shown as Clear all but in the app, the toggle button still has the Select all label.
@shobhitagarwal1612 @lakshyagupta21 Have a look at the code in my most recent commit. I've tried logging several times but according to that it should be working properly.

[iadeelzafar]

Build is failing because of using Log instead of Timber. I'll remove the log as soon as the logic works.

[lakshyagupta21]

@iadeelzafar Are you still working on this? If you're facing any difficulty let us know we'll help you out.

[iadeelzafar]

@lakshyagupta21 read my last comment. I wrote what I need assistance with.

[lakshyagupta21]

On the verbose, it is shown as Clear all but in the app, the toggle button still has the Select all label.

Check this code

if (formAdapter.getItemCount() > 0) {
     toggleButton.setText(getString(R.string.select_all));
     toggleButton.setEnabled(true);
 } else {
     toggleButton.setEnabled(false);
 }

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.
Hope this helps.

[lakshyagupta21]

Also resolve the conflicts.

[iadeelzafar]

@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.

[lakshyagupta21]

This code is causing the app to crash

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.

[ajay-prabhakar]

This code is causing the app to crash

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 opensource licenses app is crashing I fixed that before but I don't know how it is reproducing it looks like someone reverted the commit

[lakshyagupta21]

Any updates on this PR?

[iadeelzafar]

@lakshyagupta21 I think we should break it in two parts:
1- Handling state of the checkbox.
2- Handling state of the Select All/Clear All button.

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?

[lakshyagupta21]

@lakshyagupta21 I think we should break it in two parts:
1- Handling state of the checkbox.
2- Handling state of the Select All/Clear All button.

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.