Setting variable through protected function - lost reference

Question

Update 2017 (in progress...)

In summary the situation now looks like this. I click instance of B (android fragment) and expect onContextItemSelected of that instance to be to be called. The onContextItemSelected is called indeed but it turns out that this method of instance of class C.

I've been asked to show the project. After looking at this code after about 2 years I decided I should do a little more clarification on it first as it not event commented above maybe 5%. I don't think I would be able to do much more today buy I've draw some overview on whole project so you know where to look at. It's not fully completed and may contain some mistakes but more less this is how it look like:

I will try to post clear steps on how to reproduce it tomorrow. But if you want to look at it anyway I'm including the files already here

A is BaseCustomFragment

B and C are classes which extends it.

Old

I've got something like this:

public class A extends Fragment implements OnItemClickListener, OnItemLongClickListener{
    protected D loc;

    protected void setContext(D l){
        Log.d("A", "setContext :" + String.valueOf(l));
        loc = l;
        Log.d("A", "setContext2 :" + String.valueOf(loc));
    }

    public boolean onContextItemSelected(MenuItem item) {
        Log.d("A", "itemSelected :" + String.valueOf(loc));
    }
}

and

public class B extends A{

    public boolean onItemLongClick(AdapterView<?> pr, View view,int p, long id) {
        D d = (D) pr.getItemAtPosition(p);
        Log.d("B", "longClick :" + String.valueOf(d));
        setContext(d);
        return false;
    }
}

and log look like this:

B longClick :data

A setContext :data

A setContext2 :data

A itemSelected :null

I'm not touching loc or setContex anywhere else. I have completely no idea what is going on. How it is possible?

I'm setting A class as ListView listener. I'm using this fragment in ViewPager. onItemSelected is called right after the setContext. Don't know what to say more about this.

Edit:

Declate as volatile didn't fix it but... Even more wierd stuff - in class A I've got:

public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
    lv = (ListView) inflater.inflate(layoutResourceId, container, false);
    lv.setOnItemClickListener(this);
    lv.setOnItemLongClickListener(this);
    lv.setAdapter(adapter);
    //this.registerForContextMenu(lv);
    return lv;
}

I also added to class B for now:

@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
    super.onCreateView(inflater, container, savedInstanceState);
    this.registerForContextMenu(lv);
    return lv;
}
@Override
public boolean onContextItemSelected(MenuItem item) {
    //Never called!
    Log.d("Loc", "selected :" + name);
    return super.onContextItemSelected(item);
}

Where the magic happens is that I have also class Cwhich is basically the same as B only that i didn't override those extra methods above:

public class C extends A {      
    @Override
    public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
        ...
    }   
    @Override
    public boolean onItemLongClick(AdapterView<?> parent, View view,int position, long id) {
        ...
    }       
}

And the log shows:

B longClick :data - from instance of B

A setContext :data - from instance of B

A setContext2 :data - from instance of B

A itemSelected :null - from instance of C!!!

How about that? I'm calling registerForContext only in class B Any ideas?

Answer 1

Update

What's going wrong?

You use ViewPager in combination with FragmentPagerAdapter. What is an important implementation deatil of this combination is that you have several Fragments attached to the Activity. This is required for ViewPager to work as it need at least next and previous Fragments with their Views to be ready for swapping.

What's wrong is that you don't take this behavior of ViewPager into account when you dispatch your MenuItems using only title. This is in general a horrible idea and this is just on of examples why it is wrong. When context menu is shown and user selects some item, first MainActivity has a chance to handle it, then the event goes to the FragmentManager which dispatches the event to all attached Fragments (because there is no other reasonable choice if you think about it). Thus each of your Fragments receives onMenuItemSelected call and tries to handle it as MenuItem titles are the same. And of course you have an exception because for other Fragments "context" of currently selected item is not set.

How to fix it?

Obviously don't use just title to dispatch event for your MenuItems. You can actually generate unique groupId or itemId for each Fragment. But I prefer more OOP-like way. First get rid of all your original context-menu management code (menus, mpos, setMenu, addMenuItem, removeItemMenu, onMenuItemSelected, etc.) and replace them with something like this

public class BaseCustomFragment extends Fragment implements OnItemClickListener, OnItemLongClickListener
{

    private List<MenuItemAction> menus = new ArrayList<MenuItemAction>();

    // base class for all actions for context-menu in BaseCustomFragment 
    abstract class MenuItemAction implements MenuItem.OnMenuItemClickListener
    {
        private final String title;

        public MenuItemAction(String title)
        {
            this.title = title;
        }

        public String getTitle()
        {
            return title;
        }

        public final boolean isVisible()
        {
            return isVisibleImpl(conCon, locCon);
        }

        @Override
        public final boolean onMenuItemClick(MenuItem item)
        {
            AdapterContextMenuInfo info = (AdapterContextMenuInfo) item.getMenuInfo();
            Log.d("MenuItem", "menu item '" + title + "' for #" + info.position + " of  " + BaseCustomFragment.this.getClass().getSimpleName());
            handleActionImpl(conCon, locCon);
            return true;
        }

        protected final void startActivity(Uri uri)
        {
            startActivity(Intent.ACTION_VIEW, uri);
        }

        protected final void startActivity(String action, Uri uri)
        {
            startActivity(new Intent(action, uri));
        }

        protected final void startActivity(Class<? extends Activity> activityClass)
        {
            startActivity(new Intent(getActivity(), activityClass));
        }

        protected final void startActivity(Intent intent)
        {
            getActivity().startActivity(intent);
        }

        protected abstract boolean isVisibleImpl(Customer conCon, Localization locCon);

        protected abstract void handleActionImpl(Customer conCon, Localization locCon);

    }

    protected void setContext(Customer c, Localization l)
    {
        Log.d("BaseFrag", "setContext class:" + String.valueOf(this.getClass()));
        Log.d("BaseFrag", "setContext :" + String.valueOf(l));
        conCon = c;
        locCon = l;
        Log.d("BaseFrag", "setContext2 :" + String.valueOf(locCon));
        updateContextMenu();
    }

    protected void setMenus(MenuItemAction... menuItems)
    {
        this.menus = Arrays.asList(menuItems);
        updateContextMenu();
    }

    @Override
    public void onCreateContextMenu(ContextMenu menu, View v, ContextMenuInfo info)
    {
        super.onCreateContextMenu(menu, v, info);

        for (MenuItemAction menuItemAction : menus)
        {
            if (menuItemAction.isVisible())
            {
                MenuItem menuItem = menu.add(menuItemAction.getTitle());
                menuItem.setOnMenuItemClickListener(menuItemAction);
            }
        }
    }


    private void updateContextMenu()
    {
        if (lv == null)
            return;
        boolean hasVisibleItems = false;
        for (MenuItemAction menuItemAction : menus)
        {
            if (menuItemAction.isVisible())
            {
                hasVisibleItems = true;
                break;
            }
        }
        if (hasVisibleItems)
        {
            Log.d(getClass().getSimpleName(), "Attaching context menu for " + getClass().getSimpleName());
            this.registerForContextMenu(lv);
        }
        else
        {
            Log.d(getClass().getSimpleName(), "Detaching context menu for " + getClass().getSimpleName());
            this.unregisterForContextMenu(lv);
        }
    }

Here MenuItemAction is a base class for all "internal" menu items that would be held by each Fragment independently. I call them "internal" because "real" MenuItems are implementation details and you can't inherit from them so we'll create "real" ones from our "internal" ones. This transformation/creation is done inside onCreateContextMenu. Note that MenuItemAction dispatches events already providing your "context" data to where it is need. It also contains a few startActivity helper methods to simplify code.

Now we can add to BaseCustomFragment a bunch of helper methods to create your typical menu items:

    protected MenuItemAction createNavigateToLocationMenuItem()
    {
        return new MenuItemAction("Navigate to location")
        {
            @Override
            protected boolean isVisibleImpl(Customer conCon, Localization locCon)
            {
                return (locCon != null);
            }

            @Override
            protected void handleActionImpl(Customer conCon, Localization locCon)
            {
                startActivity(Uri.parse("google.navigation:q=" + Uri.encode(locCon.noZipString())));
            }
        };
    }

    protected MenuItemAction createCallMenuItem()
    {
        return new MenuItemAction("Call")
        {
            @Override
            protected boolean isVisibleImpl(Customer conCon, Localization locCon)
            {
                return (conCon != null) && (conCon.getPhoneNumbers().size() > 0);
            }

            @Override
            protected void handleActionImpl(Customer conCon, Localization locCon)
            {
                startActivity(Intent.ACTION_DIAL, Uri.parse("tel:" + Uri.encode(BaseCustomFragment.this.conCon.getPhoneNumbers().get(0).toString())));
            }
        };
    }

    protected MenuItemAction createSmsMenuItem()
    {
        return new MenuItemAction("SMS")
        {
            @Override
            protected boolean isVisibleImpl(Customer conCon, Localization locCon)
            {
                return (conCon != null) && (conCon.getPhoneNumbers().size() > 0);
            }

            @Override
            protected void handleActionImpl(Customer conCon, Localization locCon)
            {
                startActivity(Uri.parse("sms:" + Uri.encode(BaseCustomFragment.this.conCon.getPhoneNumbers().get(0).toString())));
            }
        };
    }

    protected MenuItemAction createEmailMenuItem()
    {
        return new MenuItemAction("Email")
        {
            @Override
            protected boolean isVisibleImpl(Customer conCon, Localization locCon)
            {
                return (conCon != null) && (conCon.getEmail().length() > 0);
            }

            @Override
            protected void handleActionImpl(Customer conCon, Localization locCon)
            {
                startActivity(Uri.parse("mailto:" + Uri.encode(conCon.getEmail())));
            }
        };
    }

    protected MenuItemAction createNewOrderMenuItem()
    {
        return new MenuItemAction("New Order")
        {
            @Override
            protected boolean isVisibleImpl(Customer conCon, Localization locCon)
            {
                return true;
            }

            @Override
            protected void handleActionImpl(Customer conCon, Localization locCon)
            {
                startActivity(OrderActivity.class);
            }
        };
    }

So what's left is to use this infrastructure specific fragments such as

public class CustomersFrag extends BaseCustomFragment
{
    public CustomersFrag()
    {

        ... 

        setMenus(createCallMenuItem(),
                 createSmsMenuItem(),
                 createEmailMenuItem(),
                 createNewOrderMenuItem(),
                 createNavigateToLocationMenuItem());
    }

    ... 

    @Override
    public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id)
    {
        Customer c = (Customer) parent.getItemAtPosition(position);
        setContext(c, c.getLocalization());
        return super.onItemLongClick(parent, view, position, id);
    }

or

public class LocalizationsFrag extends BaseCustomFragment
{
    public LocalizationsFrag()
    {

        ... 

        setMenus(
                createNavigateToLocationMenuItem(),
                createNewOrderMenuItem());
    }

    ... 

    @Override
    public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id)
    {
        Localization o = (Localization) parent.getItemAtPosition(position);
        setContext(null, o);
        return super.onItemLongClick(parent, view, position, id);
    }

Notice how you have no if/else or switch by menu item title in your code anymore. Moreover, you don't have to do your strange addMenuItem/removeMenuItem in each onItemLongClick depending on data in specific fragments because:

1. Each MenuItemAction now has own isVisibleImpl and handleActionImpl methods
2. setContext calls updateContextMenu

All for you menu actions seems to be generic and fit into simple "context" you already have. Thus I made MenuItemAction just a non-static inner class that can get it's parent Fragment using BaseCustomFragment.this. If at some point this becomes a limitation and you would like to create an action that is specific to sub subclass of BaseCustomFragment and you want it to use data from that fragment you just have to create menu item in the corresponding sub-class. Imagine you want to add a "Copy Order" menu item. You do something like this:

public class OrdersFragment extends BaseCustomFragment
{

    ... 


    private Order currentOrder;

    public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id)
    {
        Order o = (Order) parent.getItemAtPosition(position);
        currentOrder = o;
        setContext(o.getCustomer(), o.getLocalization());

        return super.onItemLongClick(parent, view, position, id);
    }

    protected MenuItemAction createNewOrderMenuItem()
    {
        return new MenuItemAction("Copy Order")
        {
            @Override
            protected boolean isVisibleImpl(Customer conCon, Localization locCon)
            {
                return (OrdersFragment.this.currentOrder != null);
            }

            @Override
            protected void handleActionImpl(Customer conCon, Localization locCon)
            {
                Intent intent = new Intent(getActivity(), OrderActivity.class);
                intent.putExtra("copySrcId", OrdersFragment.this.currentOrder.getId());
                startActivity(intent);
            }
        };
    }

and now your "Copy Order" MenuItemAction has access to currentOrder defined inside OrdersFragment.

Side Notes

Looking at your code I've noticed a few more things that I believe worth mentioning:

• Using integer type for phone number is a bad idea. Phone numbers might contain additional symbols such as "+" or "*" or "#" that will not fit into and integer-based type. Phone numbers might start with "0" and you'll loose converting to integer-based type.
• I think you should learn java.util package better. For example there is a contains method in java.util.List and its subclasses.
• It is not clear why you use very specific types in your declarations when you can user superclass/interface. For example, all your Model classes seem to use java.sql.Timestamp for date fields for no apparent reasons (do you realy need nanoseconds?). Or you use ArrayList everywhere although sometimes just List is enough
• In your MainActivity in prepareTabs you don't call adapter.notifyDataSetChanged(); after you've added tabs. Actually this leads to a crash with newer support library.

Hope this helps

---

Old answer (and most probably irrelevant)

Add this to each Log call. I suspect that the answer is that in

B longClick :data

A setContext :data

A setContext2 :data

A itemSelected :null

somehow the last Log is done for different object that the first 3. Also is it true that the issue is not reproducible every time? If so maybe it is related to device rotation or something else triggering Activity re-creation in the middle of your event handling. So you can add log to your onPause and onResume and see if this is the case.

Answer 2

If your A object is being accessed from multiple threads (which seems likely given your event listeners), you can see problems like this (the general terminology for it is "stale data"). To quickly check whether this is a threading issue, declare your member variable as protected volatile D loc; and see if the problem goes away (see this resource about volatile). If this does indeed solve the problem, you will need to go about implementing more advanced threading protections to make sure you don't run into any more nefarious/subtle threading bugs.