Deleting crime after scrolling with ViewPager is not immediately reflected on the CrimeListFragment

YogurtProblems · March 20, 2018, 9:58pm

I’ve added just a simple “Remove Crime” Button to my CrimeFragment. Code for the click action:

mRemoveButton = (Button) v.findViewById(R.id.crime_remove);
    mRemoveButton.setOnClickListener(new View.OnClickListener(){
        @Override
        public void onClick(View v) {
            CrimeLab.get(getActivity()).removeCrime(mCrime.getId(), mCrime);



            getActivity().finish();

        }
    });

Relevant method in CrimeLabe:

public void removeCrime(UUID id, Crime c){
    mCrimes.remove(id, c);
}

This works fine in nearly all cases. I press the button, crime is removed, popped back to the list, and that crime is no longer on the list.

But…

If I select a crime, scroll sideways to another crime, and remove that crime, the updated list shows the one I initially selected as being the one removed. Then if I click into another crime, and back right out, the updated list shows perfect.

I think it has to do with the CrimePagerActivity code, but I am not sure.

package com.bignerdranch.android.criminalintent;


import android.content.Context;
import android.content.Intent;
import android.os.Bundle;
import android.support.v4.app.Fragment;
import android.support.v4.app.FragmentManager;
import android.support.v4.app.FragmentPagerAdapter;
import android.support.v4.app.FragmentStatePagerAdapter;
import android.support.v4.view.ViewPager;
import android.support.v7.app.AppCompatActivity;
import android.view.View;
import android.view.ViewParent;
import android.widget.Button;


import java.util.List;
import java.util.UUID;



public class CrimePagerActivity extends AppCompatActivity {

private static final String EXTRA_CRIME_ID = "com.bignerdranch.android.criminalintent.crime_id";

private ViewPager mViewPager;
private List<Crime> mCrimes;

private Button mFirstButton;
private Button mLastButton;

public static Intent newIntent(Context packageContext, UUID crimeID){
    Intent intent = new Intent(packageContext, CrimePagerActivity.class);
    intent.putExtra(EXTRA_CRIME_ID, crimeID);
    return intent;
}

@Override
protected void onCreate(Bundle savedInstanceState){
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_crime_pager);

    UUID crimeId = (UUID) getIntent().getSerializableExtra(EXTRA_CRIME_ID);

    mViewPager = (ViewPager) findViewById(R.id.crime_view_pager);



    mCrimes = CrimeLab.get(this).getCrimes();
    FragmentManager fragmentManager = getSupportFragmentManager();
    mViewPager.setAdapter(new FragmentStatePagerAdapter(fragmentManager) {

        @Override
        public Fragment getItem(int position) {
            Crime crime = mCrimes.get(position);
            return CrimeFragment.newInstance(crime.getId());
        }

        @Override
        public int getCount() {
            return mCrimes.size();
        }
    });

    for (int i = 0; i < mCrimes.size(); i++){
        if (mCrimes.get(i).getId().equals(crimeId)){
            mViewPager.setCurrentItem(i);
            break;
        }
    }

    mViewPager.addOnPageChangeListener(new myChangeListener());

    mFirstButton = (Button) findViewById(R.id.first_button);
    mFirstButton.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            mViewPager.setCurrentItem(0);
        }
    });

    mLastButton = (Button) findViewById(R.id.last_button);
    mLastButton.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            mViewPager.setCurrentItem(mViewPager.getAdapter().getCount() -1);
        }
    });


}

private class myChangeListener extends ViewPager.SimpleOnPageChangeListener {

    @Override
    public void onPageScrolled(int position, float positionOffSet, int positionOffsetPixels){
        if (mViewPager.getCurrentItem() == 0){
            mFirstButton.setEnabled(false);
        }
        else {
            mFirstButton.setEnabled(true);
        }

        if (mViewPager.getCurrentItem() == mViewPager.getAdapter().getCount() -1){
            mLastButton.setEnabled(false);
        }
        else {
            mLastButton.setEnabled(true);
        }
    }

}

}

Thank you

YogurtProblems · March 20, 2018, 10:06pm

Figured it out myself. It was the updateUI() code in CrimeListFragment

private void updateUI(){
    CrimeLab crimeLab = CrimeLab.get(getActivity());

    List<Crime> crimes = crimeLab.getCrimes();



    if (mAdapter == null){
        mAdapter = new CrimeAdapter(crimes);

        mCrimeRecyclerView.setAdapter(mAdapter);
    }
    else{
        mAdapter.notifyDataSetChanged();
        mAdapter.replaceList(crimes);
        //mAdapter.notifyItemChanged(mLastClickedPosition);
    }

    updateSubtitle();

}

As you can see, I just call mAdapter.notifyDataSetChanged() every time instead of .notifyItemChanged.

This also fixes any weird crashings I had. Is this considered clean code?

Srinath · March 21, 2018, 4:32am

In fact, this was one of the challenges in an earlier chapter was efficiently reloading the RecyclerView. Reloading the entire data set when only one item was changed is highly inefficient. notifyItemChanged is the way to go .

You can probably have a boolean track to check if the user has actually deleted the Crime item or added a new one. So, when an user presses the back button to exit the CrimePagerActivity and back to the CrimeListActivity hosting the CrimeListFragment, the user gets to see the efficiently updated list.

Probably you can modify your else block to look the one below:

else {
	if (!itemAddedOrDeleted) {
		adapter.notifyDataSetChanged();
	}
	else {
		adapter.notifyItemChanged(position);
	}
}

YogurtProblems · March 21, 2018, 1:26pm

Yeah, that’s why I had that code in there. I wasn’t sure how to fix it and still keep the efficiency, but your solution seems to make sense. I will give it a shot. Thanks

YogurtProblems · March 21, 2018, 7:06pm

So I have figured out how to get this to work with adding a crime, but not removing one. I am stuck on trying to pass some data back from the CrimeFragment to the CrimeListFragment.

Srinath · March 21, 2018, 8:00pm

What data/information are you trying to pass back from the CrimeFragment to the CrimeListFragment? Deleting the crime from within the CrimeFragment class will update the underlying data that is referenced via (CrimeLab.get(getActivity()).getCrimes()) . So, as you soon as you delete like this:

@Override
    public boolean onOptionsItemSelected(MenuItem item) {
        switch (item.getItemId()) {
            case R.id.delete_crime:
                deleteCurrentCrime();
                getActivity().finish();
                return true;
            default:
                return super.onOptionsItemSelected(item);
        }
    }

The hosting activity (CrimePagerActivity) should terminate and take you back to CrimeListActivity which in turns hosts the CrimeListFragment. Finally, since, we have overrode the onResume() and inform the adapter the change via notifyItemChanged(position) the list is updated and you won’t see the deleted crime as part of the list.

YogurtProblems · March 21, 2018, 8:08pm

I’m trying to get some data back so I can run a check to see if the user deleted the crime or not.

When adding a new crime, I have this method:

private void addNewCrime() {

    mCrimeCountChanged = true;

    Crime crime = new Crime();
    CrimeLab.get(getActivity()).addCrime(crime);
    Intent intent = CrimePagerActivity.newIntent(getActivity(), crime.getId());
    startActivity(intent);
}

This just flags mCrimeCountChanged when adding a crime.

I can not figure out the best way to get some data back from the CrimeFragment in order to flag mCrimeCountChanged when a Crime is deleted.

I also have my updateUI() code set to disable the mCrimeCountChange boolean after it is used:

private void updateUI(){
    CrimeLab crimeLab = CrimeLab.get(getActivity());

    List<Crime> crimes = crimeLab.getCrimes();

    if (crimes.size()> 0){
        mEmptyView.setVisibility(View.INVISIBLE);
    }
    else {
        mEmptyView.setVisibility(View.VISIBLE);
    }

    if (mAdapter == null){
        mAdapter = new CrimeAdapter(crimes);
        mCrimeRecyclerView.setAdapter(mAdapter);
    }
    else{
        if (mCrimeCountChanged){
            mAdapter.notifyDataSetChanged();
            mAdapter.replaceList(crimes);
            mCrimeCountChanged = false;
        }
        else {
            //mAdapter.notifyDataSetChanged();
            mAdapter.replaceList(crimes);
            mAdapter.notifyItemChanged(mLastClickedPosition);
        }
    }
    
    updateSubtitle();
}

As I reread the Challenge, I do see that it wanted the delete to be an Action item. I made it just a Button on CrimeFragment, but the method of passing a flag back to CrimeListFragment should be the same, no?

Srinath · March 21, 2018, 10:42pm

I am so sorry , I meant for me to say “have a boolean track changes within a Crime instance” so that the CrimeListFragment would show the updated crime. This is associated with another challenge. No idea why I mentioned that. So please ignore the above quote.

Now, let’s solve this problem with add/delete Crime instance. You do not need any kind of tracker to do this. Simply, add/delete from the list and propagate the model layer level change to the view via the controller (Activity/Fragment). So, when you do this, within the addNewCrime() in your code:

a new crime object is added to the list of crimes stored with CrimeLab. That is why the date and id of the crime is pre-filled when a new crime is opened via the add crime menu action item in the CrimeListFragment. After setting the title of the crime and pressing back, we are loaded back into the CrimeListFragment. Since, we have an overridden onResume() that has this line - adapter.notifyItemChanged(position), the list is updated with the newly added crime.

Same thing with delete too. This is my deleteCurrentCrime() method that I use in the CrimeFragment class.

private void deleteCurrentCrime() {
    CrimeLab crimeLab = CrimeLab.get(getActivity());
    crimeLab.deleteCrime(crime); // I pass the current crime object
}

The deleteCrime(Crime crime) looks like this in CrimeLab.java:

public void deleteCrime(Crime c) {
    crimes.remove(c);
}

That’s it. This takes care of deleting the Crime object from the list of crimes and getActivity().finish() terminates the calling activity, which is CrimePagerActivity. Finally, we are loaded back to the CrimeList and again because of the RecyclerView update via the notifyItemChanged we will see the current version of the list (w/ crime removed).

Yes, the delete crime should be a menu action item within the CrimeFragment. I used the bin icon for this menu item.

YogurtProblems · March 22, 2018, 6:27pm

So all of that makes sense, but it doesn’t address the issue I have. I have effectively the same code you have, but I get an issue when swiping. Let me try to make an example:

Let’s say I have 4 crimes in my list:

Crime 1
Crime 2
Crime 3
Crime 4

If I open Crime 4, then swipe to Crime 3 and delete that one, the list will show up like this:

Crime 1
Crime 2
Crime 3

I have deleted Crime 3, but the list shows as Crime 4 being deleted. If I then click into a Crime, and then back right out of it, the list shows up properly, as:

Crime 1
Crime 2
Crime 4

I am pretty sure this has to do with

mAdapter.notifyItemChanged(mLastClickedPosition);

Being called the first time, and

mAdapter.notifyDataSetChanged();

Being called the second time. This is why I am trying to pass some sort of data back from the Removed crime. Something that would trigger either .notifyDataSetChanged(); or .notifyItemChanged(); if I could get the correct list position of the deleted item. This is the part I am stuck on. It’s a minor thing, but I’d like to figure out how to get it to work, for my own knowledge.

Srinath · March 23, 2018, 6:37pm

can you paste your CrimeFragment.java class here.

hybris · July 6, 2018, 6:48pm

I compare crimes.size() to mAdapter.getItemCount() to check if an item was cancelled or not.
If crimes.size() is smaller, I call mAdapter.notifyItemRemoved(crimePosition).

private void updateUI() {
        CrimeLab crimeLab = CrimeLab.get(getActivity());
        List crimes = crimeLab.getCrimes();
      
        if (mAdapter == null) {
            mAdapter = new CrimeAdapter(crimes);
            mCrimeRecyclerView.setAdapter(mAdapter);
        } else {
            mAdapter.replaceList(crimes);
            if (crimes.size() < mAdapter.getItemCount()) {
                mAdapter.notifyItemRemoved(crimePosition);
            } else {
                mAdapter.notifyItemChanged(crimePosition);
            }
           
            mAdapter.notifyDataSetChanged();
        }
       ...

linxz · March 10, 2019, 3:43am

Someone talked about it in next Chapter. Deleting Crime deletes the Crime from the DB but throws IndexOutOfBoundsException
For this case, it works for me to change mCrimes = new ArrayList<>(); to mCrimes = new CopyOnWriteArrayList<>(); in CrimeLab.java.

How can I got it? Just basing on the output of the Logcat: java.util.ConcurrentModificationException,
I searched remove Fragment Page from ViewPager in Android java.util.ConcurrentModificationException, and find https://stackoverflow.com/questions/6621991/how-to-handle-concurrentmodificationexception-in-android

Topic		Replies	Views
How to know your index from ViewPager to remove the crime? 13. The Toolbar	2	1737	December 22, 2017
Deleting crimes from CrimePagerActivity instead of CrimeFragment 14. SQLite Databases	0	1887	January 20, 2019
Remove the crime and stay in the fragment crime 13. The Toolbar	1	1806	November 22, 2017
Adding onCrimeDeleted() to CrimeListFragment.Callbacks 17. Two-Pane Master-Detail Interfaces	0	1813	September 9, 2017
Review request of dual-delete challenge solution 17. Two-Pane Master-Detail Interfaces	1	1671	March 24, 2017

Deleting crime after scrolling with ViewPager is not immediately reflected on the CrimeListFragment

Related topics