1

Please keep in mind I'm a beginner in Android, so it is possible that somewhere I overlooked something. Also I have more then one question here, because my approach to the whole project might be wrong.

I have an application that has side menu implemented via DrawerLayout. The "main screen" is a Fragment and with Drawer I create the side menu with a list in it. After that this is how I change the fragments upon clicking on the side menu:

private void displayView(int position) {
    // update the main content by replacing fragments

    Fragment fragment = null;
    switch (position) {
        case 0:
            fragment = new HomeFragment();
            break;
        case 1:
            fragment = new FavoritesFragment();
            break;
        case 2:
            fragment = new LastVisitedFragment();
            break;
        case 3:
            fragment = new SettingsFragment();
            break;
        default:
            break;
    }

    if (fragment != null) {
        FragmentManager fragmentManager = getFragmentManager();
        fragmentManager.beginTransaction().replace(R.id.frame_container, fragment).commit();

        // update selected item and title, then close the drawer
        mDrawerList.setItemChecked(position, true);
        mDrawerList.setSelection(position);
        setTitle(navMenuTitles[position]);
        mDrawerLayout.closeDrawer(mDrawerList);
    } else {
        Log.e("MainActivity", "Error in creating fragment");
    }
}

The code above is implemented in my main activity. Next this is how my HomeFragment looks: (The home fragment is my home page so to say)

public class HomeFragment extends Fragment {

    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {

        Log.i("HomeFragment", "OK");
        View rootView = inflater.inflate(R.layout.fragment_home, container, false);
        rootView.setOnClickListener(new View.OnClickListener() {

            @
            Override
            public void onClick(View v) {
                Log.i("HomeFragment", "OK0");
                switch (v.getId()) {
                    case R.id.rec_prod1:
                        Log.i("HomeFragment", "OK1");
                        Intent intent1 = new Intent(getActivity(), SingleItemActivity.class);
                        startActivity(intent1);
                        break;
                    case R.id.rec_prod2:
                        Log.i("HomeFragment", "OK2");
                        Intent intent2 = new Intent(getActivity(), SingleItemActivity.class);
                        startActivity(intent2);
                        break;
                    case R.id.rec_prod3:
                        Log.i("HomeFragment", "OK3");
                        Intent intent3 = new Intent(getActivity(), SingleItemActivity.class);
                        startActivity(intent3);
                        break;

                }
            }
        });


        return rootView;
    }

}

I have tried it with onTouchListener to but didn't work either, the single difference was that with onTouch it entered in to the onTouch method, bet nothing happened after. Also somewhere on this site I have red to set the scrollview clickable=true and all childs to false in order for it to work, but that didn't do either. Also fragment_home.xml is a custom layout with the Parent being a ScrollView (It's a list with three coloumns and 5 rows but each element is different, they all have an Imageview and different number of textviews, I didn't use a ListView or GridList since each row has a title, and the elements in each row have different layouts) I attach only an overview of it since the real xml code is over 800 lines long.

<ScrollView>
<LinearLayout>
   <!--This part repeats 5 times creating each row -->  
   <TextView>   
   <LinearLayout>
     <!-- This part repeats 3 times for each element of a column -->
     <RelativeLayout>
        <ImageView>
            <!-- This part changes acodringly to the number of textviews, I can have 1 to 3 TextViews here-->
            <LinearLayout>
              <TextView>
              <TexytView>
            </LinearLayout>
    </RelativeLayout>
  </LinearLayout>           
</LinearLayout>
</ScrollView> 

So I have two main questions. 1. Why doesn't the clicking/touching work. 2. Is the project structure(architecture) ok like this? The whole idea is that when a click happens some elements take me to a product view screen, while others to a list of product elements (the data that fills all this will come from a server, that's why I started a new Activity in the onClick method).

Jerodev
  • 32,252
  • 11
  • 87
  • 108

2 Answers2

0

You either need to set an onclick action via XML (this works for activities), or you need to set it in code (almost like you have it) but assigned to the views themselves

Personally I prefer to define the click actions in the code. I think it makes much more sense when reading a project and you avoid any issues like this. It's also the recommended way to do it and is how most sample code and examples will do it (see Android Documentation)

To define the click via code do it like this:

public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {

    View rootView = inflater.inflate(R.layout.fragment_home, container, false);
    View OK3 = rootView.findViewById(R.id.rec_prod3); //pick a better name than i have though
    if (OK3 != null) {
        OK3.setOnClickListener(new View.OnclickListener() {
            Log.i("HomeFragment", "OK3");
            Intent intent3 = new Intent(getActivity(), SingleItemActivity.class);
            startActivity(intent3);
        });
    }

Most examples use this method as the alternative of using a switch statement and directing every click through the same method is bad practice - essentially you are hacking a shortcut Google has provided for XML support into doing something it wasn't intended for. For more information as to why it is better to use a dedicated listener, look up Inheritance vs Composition - as a rule "Has A" always beats "Is A" for code readability and maintainability.

Nick Cardoso
  • 20,807
  • 14
  • 73
  • 124
  • Thank you, anonymous moderator, for cleaning up the comments :) Anyway, there's an important thing to note here again: "set an onclick action via XML" unfortunately doesn't work for fragments (see [How to handle button clicks using the xml onClick within Fragments](http://stackoverflow.com/questions/6091194/how-to-handle-button-clicks-using-the-xml-onclick-within-fragments/14571018)). – 0101100101 Aug 08 '14 at 13:45
  • I didn't vote on your answer because you offend me. I genuinely think your approach is bad, especially when teaching a new developer - and that's what the votes are intended for. Increasing cyclic complexity with switch statements is bad for maintainability and using inheritance instead of composition is also bad (so says general consensus), though of course some people have divided opinions and you don't need to agree - that's why questions accept multiple answers. – Nick Cardoso Aug 11 '14 at 19:19
-1

You're setting the OnClickListener for the whole fragment, not the actual views you are querying in the switch-statement. Of course the specific code parts are never reached! This approach should work (and is officially supported by Google and well received by users on Stack Overflow):

public class HomeFragment extends Fragment implements View.OnClickListener {

public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
    Log.i("HomeFragment", "OK");
    View rootView = inflater.inflate(R.layout.fragment_home, container, false);
    rootView.findViewById(R.id.rec_prod1).setOnClickListener(this);
    rootView.findViewById(R.id.rec_prod2).setOnClickListener(this);
    rootView.findViewById(R.id.rec_prod3).setOnClickListener(this);
    return rootView;
}

@Override
public void onClick(View v) {
    Log.i("HomeFragment", "OK0");
    switch (v.getId()) {
    case R.id.rec_prod1:
        Log.i("HomeFragment", "OK1");
        Intent intent1 = new Intent(getActivity(), SingleItemActivity.class);
        startActivity(intent1);
        break;
    case R.id.rec_prod2:
        Log.i("HomeFragment", "OK2");
        Intent intent2 = new Intent(getActivity(), SingleItemActivity.class);
        startActivity(intent2);
        break;
    case R.id.rec_prod3:
        Log.i("HomeFragment", "OK3");
        Intent intent3 = new Intent(getActivity(), SingleItemActivity.class);
        startActivity(intent3);
        break;
    }
}
}

For further reading:

Setting the onClick via XML would also work, but sadly, only in activities, not in fragments: The method is invoked on the fragment's parent activity (see How to handle button clicks using the XML onClick within Fragments).

Still, the above approach has a disadvantage: onClick is public and thus could be called from outside. For example, the parent activity could call

((View.OnClickListener) homeFragment).onClick(
    homeFragment.getView().findViewById(R.id.rec_prod1));

This would fire an onClick event for rec_prod1. IMO, the readability and convenience this approach brings outweighs this. Of course, there are other approaches with their own advantages and disadvantages. Most importantly, to prevent this public access, you can create an anonymous class for each listener (see Nick's answer and its disadvantages) or use the code below instead:

public class HomeFragment extends Fragment {

public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
    Log.i("HomeFragment", "OK");
    View rootView = inflater.inflate(R.layout.fragment_home, container, false);
    View.OnClickListener onClick = new RecProdClickListener();
    rootView.findViewById(R.id.rec_prod1).setOnClickListener(onClick);
    rootView.findViewById(R.id.rec_prod2).setOnClickListener(onClick);
    rootView.findViewById(R.id.rec_prod3).setOnClickListener(onClick);
    return rootView;
}

private class RecProdClickListener implements View.OnClickListener {
    @Override
    public void onClick(View v) {
        Log.i("HomeFragment", "OK0");
        switch (v.getId()) {
        case R.id.rec_prod1:
            Log.i("HomeFragment", "OK1");
            Intent intent1 = new Intent(getActivity(), SingleItemActivity.class);
            startActivity(intent1);
            break;
        case R.id.rec_prod2:
            Log.i("HomeFragment", "OK2");
            Intent intent2 = new Intent(getActivity(), SingleItemActivity.class);
            startActivity(intent2);
            break;
        case R.id.rec_prod3:
            Log.i("HomeFragment", "OK3");
            Intent intent3 = new Intent(getActivity(), SingleItemActivity.class);
            startActivity(intent3);
            break;
        }
    }
}
}

Beside all that several anonymous click listeners will lead to more memory usage and a little bit longer intialization time, whereas reusing a click listener with a switch-statement will make performance a negligibly worse due to cyclomatic complexity.

But you can even mix the different approaches. Ultimately, it's up to your taste. Discussing what's best under what circumstances would belong to programmers.stackexchange and surely is an interesting topic for clean code in Android development.

Community
  • 1
  • 1
0101100101
  • 5,786
  • 6
  • 31
  • 55
  • I read title and says "OnTouchListener/OnClickListener" OnClickListener - > into - > OnTouchListener. I don't see any OnTouchListener... please read again title and re post your answer, thx. –  Sep 09 '15 at 13:04