Challenge Solution - having trouble


#1

I’m attempting the challenge and am curious about the solution. I’m assuming the couple bits about resuming from fragments had to do with the solution, so I’m attempting to set an intent for returning an activity result. However, my Intent data in the onActivityResult method within CrimeListFragment is always null! This is frustrating. I’ve logged all of the lifecycle methods to try to get to the bottom of it, with limited results. I believe I’m calling everything in the right order.

My code is available on github at github.com/mwarger/CriminalIntent

If someone could possibly help point me in the right direction I would very much appreciate it. Thank you!

For reference, the onActivityResult method looks like this:

CrimeListFragment.java

@Override public void onActivityResult(int requestCode, int resultCode, Intent data) { super.onActivityResult(requestCode, resultCode, data); Log.d(TAG, "onActivityResult "); if (requestCode == REQUEST_CRIME){ listIndex = mAdapter.mCrimes.indexOf(CrimeLab.get(getActivity()) .getCrime((UUID) data.getSerializableExtra(ARG_CRIME_ID))); Log.d(TAG, "onActivityResult item changed " + listIndex); } }

The returnResult method within the CrimeFragment.java

public void returnResult() { Intent data = new Intent(); data.putExtra(ARG_CRIME_ID, mCrime.getId()); getActivity().setResult(Activity.RESULT_OK, data); }


#2

I’m by no means an expert (which is why I’m reading the book), but I think it’s as simple as using the position (list index) of the item clicked (which is an int) as the requestCode when calling startActivityForResult(). See the “Getting a result back from a child activity” section in chapter 5 (page 101). The first part of that section talks about passing a requestCode parameter to startActivityForResult(). Per the book, the requestCode is “a user-defined integer that is sent to the child activity and then received back by the parent”. In Chapter 5 they have you define a constant to use as the requestCode, but I see no reason why you couldn’t just use the position/index of the clicked list item as the requestCode. So, I’m thinking you’d just need to do the following:


public class CrimeListFragment extends Fragment {

   ....

    private class CrimeHolder extends RecyclerView.ViewHolder implements View.OnClickListener {
       
        ....

        @Override
        public void onClick(View v) {
            Intent intent = CrimeActivity.newIntent(getActivity(), mCrime.getId());
            startActivityForResult(intent, getAdapterPosition());
        }
    }

    @Override
    public void onActivityResult(int requestCode, int resultCode, Intent data) {
        super.onActivityResult(requestCode, resultCode, data);
        mAdapter.notifyItemChanged(requestCode);
    }

    ....
}

And I believe you would then want to remove the onResume() method, which calls updateUI(), as I don’t think that’s needed any longer. But I’m not 100% sure about that.

I haven’t tried any of this yet, but I plan to see if I can get something along these lines working. I’d be interested to hear from one of the moderators or an expert if this is an acceptable/optimal way to do it.


#3

So i tried my solution above. In addition to removing the onResume() method, I also reverted the updateUI() method back to it’s original state:

    private void updateUI() {
        CrimeLab crimeLab = CrimeLab.get(getActivity());
        List<Crime> crimes = crimeLab.getCrimes();

        mAdapter = new CrimeAdapter(crimes);
        mCrimeRecyclerView.setAdapter(mAdapter);

    }

I can confirm that doing it this way DOES indeed work. But I’d still be curious to hear from an expert if this (using the adapter position as the requestCode) is a recommended/optimal approach, and/or if there are any other gotcha’s I haven’t addressed.


#4

What you’re doing with the request code is a safe thing to do. That said, it’s not necessarily the best way to use the request code because it limits what you can do in the future. As you’ve seen earlier in the book, the request code is typically used to identify which activity the user is returning from, which allows you to handle results coming back from different activities. You’d have to change your request code implementation if you needed to start some other activity for a result.

Your challenge solution is a good one. I like that you’ve found a way to solve the problem without having to involve the other fragment.

There is actually another solution that doesn’t require you to use startActivityForResult at all. You could store the last clicked adapter position as an instance variable on your CrimeListFragment. Then, when updateUI is called as you return to CrimeListFragment, you just use that instance variable and clear its value. I haven’t tried this out myself, but it should be safe in every situation. The code would look something like this:

public class CrimeListFragment extends Fragment {
  private int mLastAdapterClickPosition = -1;

  ...

  private void updateUI() {
      CrimeLab crimeLab = CrimeLab.get(getActivity());
      List<Crime> crimes = crimeLab.getCrimes();
      if (mAdapter == null) {
          mAdapter = new CrimeAdapter(crimes);
          mCrimeRecyclerView.setAdapter(mAdapter);
      } else {
          if (mLastAdapterClickPosition < 0) {
            mAdapter.notifyDataSetChanged();
          } else {
            mAdapter.notifyItemChanged(mLastAdapterClickPosition);
            mLastAdapterClickPosition = -1;
          }
      }
  }


  private class CrimeHolder extends RecyclerView.ViewHolder implements View.OnClickListener {
   
      ...
      
      @Override
      public void onClick(View v) {
          mLastAdapterClickPosition = getAdapterPosition();
          Intent intent = CrimeActivity.newIntent(getActivity(), mCrime.getId());
          startActivity(intent);
      }
  }
  
}

Hello, I'd like to know answer to challenge of Chapter 10
My Challenge Solution (Feels Wrong)
#5

Thank you both for your replies and suggestions to my question. I understand now - I may have been overthinking this a bit. Cheers!


#6

I was originally going to do it that way, and your point about requestCode not being future proof is a good one. But, if you use an instance variable on CrimeListFragment, you would then need to implement onSaveInstanceState() to make sure that the new instance variable could be restored in case the activity was destroyed, right? I tested it by turning on the dev tool option “Always destroy activities”. And sure enough, when I click on a crime and CrimeActivity is opened, CrimeListActivity is destroyed (and the value of CrimeListFragment’s instance variable along with it). Then when I return from CrimeActivity (by pressing the back button), CrimeListActivity is recreated. It still technically works, due to the conditional that redraws the whole list when the edit index is -1. But it seems that if you use an instance variable, then a complete solution to the challenge would involve implementing onSaveInstanceState. Is that right?

Update: Now that I think about it, I guess you don’t have to save the current index to the activity record. If the activity is destroyed and has to be recreated, then it has to reload/redraw the whole list anyway, so there’s no real need to save the index of the item that was edited. Correct?


#7

Yep. That’s exactly it!


#8

Dear Chris,

I had the same solution in mind as you, but I have some doubts.
Now notifyDataSetChanged() is always called after an onCreate() (so also after configuration change) and/or after OnResume() (returning to app after pressing the home button, or after returning from sleep).
This way, the “resource-heavy” notityDataSetChanged() is still called quite a lot no?


#9

You’re right that it may still be called more than it needs to. There isn’t a need to have the updateUI() call in onCreateView and onResume. You can remove the call that’s in onCreateView (onResume is guaranteed to be called shortly after onCreateView is called). So that cuts down a lot.

As for rotation, the view+adapter will be destroyed so that will be set up from scratch again (there’s no way around that). I hadn’t thought about the case where you turn your screen off and then back on. You could add some kind of extra state/guard for that too.

In the end, I’d encourage you to consider the tradeoff between code maintainability and performance (as you can see, this is starting to get more and more complicated as we think about edge cases). In this app, you won’t see much of a difference between the resource-heavy implementation and the optimized implementation. In some apps though (the Facebook news feed, for example), it might make a big difference.

Thanks for pointing out those edge cases!


#10

This solution seems to work, but

...
private int lastSelectedCrimePositionID = -1;
...
public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
...

        mCrimeAdapter = new CrimeAdapter(CrimeLab.getInstance(getActivity()).getCrimes());
        mRecyclerView.setAdapter(mCrimeAdapter);
        lastSelectedCrimePositionID = -1;
        return v;
}

    @Override
    public void onActivityResult(int requestCode, int resultCode, Intent data) {
        if (requestCode == REQUEST_CRIME) {
            if (resultCode == Activity.RESULT_OK) {
                Log.d(TAG, "onActivityResult with result ok, lastSelectedCrimePositionID " + lastSelectedCrimePositionID);
                if (lastSelectedCrimePositionID >= 0) {
                    mCrimeAdapter.notifyItemChanged(lastSelectedCrimePositionID);
                }
            } else {
                Log.d(TAG, "onActivityResult : ResultCode Not ResultOK");
            }
        }
    }

I havent overriden onResume() with assumption that onActivityResult is called before onResume() and its not possible to the change the contents of List other than from CrimeActivity i.e resulting a call to onActivityResult.

CrimeFragment.java

        mSolvedCheckBox.setOnCheckedChangeListener(new CompoundButton.OnCheckedChangeListener() {
            @Override
            public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
                mCrime.setSolved(isChecked);
                updateResultIntentStatingCrimeWasUpdated();
            }
        });
...
        mTitleField.addTextChangedListener(new TextWatcher() {
            @Override
            public void beforeTextChanged(CharSequence s, int start, int count, int after) {
            }

            @Override
            public void onTextChanged(CharSequence s, int start, int before, int count) {
                Log.d(TAG, "onTextChanged ");
                mCrime.setTitle(s.toString());
                updateResultIntentStatingCrimeWasUpdated();
            }

            @Override
            public void afterTextChanged(Editable s) {
            }
        });
...
    private void updateResultIntentStatingCrimeWasUpdated() {
        getActivity().setResult(Activity.RESULT_OK);
    }
..

#11

[quote=“cstewart”]
The code would look something like this:

[code]
public class CrimeListFragment extends Fragment {
private int mLastAdapterClickPosition = -1;

private void updateUI() {
CrimeLab crimeLab = CrimeLab.get(getActivity());
List crimes = crimeLab.getCrimes();
if (mAdapter == null) {
mAdapter = new CrimeAdapter(crimes);
mCrimeRecyclerView.setAdapter(mAdapter);
} else {
if (mLastAdapterClickPosition < 0) {
mAdapter.notifyDataSetChanged();
} else {
mAdapter.notifyItemChanged(mLastAdapterClickPosition);
mLastAdapterClickPosition = -1;
}
}
}

private class CrimeHolder extends RecyclerView.ViewHolder implements View.OnClickListener {

  ...
  
  @Override
  public void onClick(View v) {
      mLastAdapterClickPosition = getAdapterPosition();
      Intent intent = CrimeActivity.newIntent(getActivity(), mCrime.getId());
      startActivity(intent);
  }

}

}
[/code][/quote]

Hi, in the upDateUI method, why is this clause needed:


if (mLastAdapterClickPosition < 0) {
mAdapter.notifyDataSetChanged();
}

When would there be case when the position is -1 but the mAdapter wasn’t null so it wouldn’t enter that clause.
Maybe I’m not understanding it correctly.


#12

[code]private class CrimeHolder extends RecyclerView.ViewHolder implements View.OnClickListener {

  ...
  
  @Override
  public void onClick(View v) {
      mLastAdapterClickPosition = getAdapterPosition();
      Intent intent = CrimeActivity.newIntent(getActivity(), mCrime.getId());
      startActivity(intent);
  }

}[/code]

Since the clickPosition is used here, I am wondering whether we could directly pass the clickPosition to the CrimeActivity instead of the UUID. Then we could use the position to get the right Crime.


#13

@DoNotMicrowave:

This statement:

if (mLastAdapterClickPosition < 0) { mAdapter.notifyDataSetChanged(); }

is there because the updateUI() method is called in onResume. onResume will be called when you return from the second activity, but it will also be called across a rotation or if the user switches to another app and comes back. In those other cases, I had the value initialized to -1. You’re right that the RecyclerView will never give us back a < 0 click position.

@oscaryanh:
You could pass around the position in the list instead of the UUID and it would work. It just depends on how you want to identify a crime (by position or by UUID or by something else). UUID is a little safer than position because position could change as you add to the app (for example, what happens if you allow the user’s to re-order crimes in the list?).


#14

Thanks very much for that!


#15

Going back to the initial question, I’ve got it working by putting the returnResult inside the CrimeFragment onCreate.


#16

Thank you very much!


#17
if (mAdapter == null) {
 mAdapter = new CrimeAdapter(crimes);
 mCrimeRecyclerView.setAdapter(mAdapter);
} else {
    if (mLastAdapterClickPosition < 0) {
       mAdapter.notifyDataSetChanged();
    }

But since across a rotation android set mLastAdapterClickPosition to -1 wouldn’t it set mAdapter to null too? In this case even if mLastAdapterClickPosition < 0 condition will fall in if (mAdapter == null).

Maybe there are cases in that android low memory killer can destroy some instances variables and others not. Right? In that cases mAdapter.notifyDataSetChanged(); will be there just to be safe :wink:


#18

When an activity is killed due to memory pressure, every variable is destroyed. The whole instance of the activity is removed from memory.

That’s the trick here. Through a low-memory death and recreation, mLastAdapterClickPosition is going to get reset back to -1 AND the Adapter will get destroyed and the entire view of the activity will be destroyed. Everything will be rebuilt from scratch in the new instance, so there are no performance optimizations that you can do at that point.


#19

If the whole activity is destroyed due roteate or memory killer is there a case that i need this part of code?

if (mLastAdapterClickPosition < 0) {
    mAdapter.notifyDataSetChanged();

In our case I just need to change one item each time, so just mAdapter.notifyItemChanged(mLastAdapterClickPosition); will do the trick since the edges cases will be catched with the if (mAdapter == null) condition :wink:


#20

Hi! So, is this block provisioning for data set change(s) during the time the activity going to the background briefly and then becoming fully visible again?