Scrolling in Horizontal ScrollView losing selection

Question

When I chose an element from this HorizontalScrollView, I was setting focus to that element by calling:

else if (v.getParent() == candidatesScrollView.getChildAt(0))
{
    Button candidateButton = (Button) v;
    v.requestFocusFromTouch();
    v.setSelected(true);
            (...)
}

Here is part of my XML:

<HorizontalScrollView
        android:id="@+id/CandidatesHorizontalScrollView"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:layout_above="@+id/linearLayout2"
        android:clickable="false"
        android:focusable="false"
        android:focusableInTouchMode="false"
        android:visibility="gone" >

    <LinearLayout
        android:layout_width="match_parent"
        android:layout_height="match_parent"
        android:clickable="false"
        android:focusable="false"
        android:focusableInTouchMode="false"
        android:orientation="horizontal" >

        <Button
            android:id="@+id/horizontalscrollview1_button1"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:text="Button"
            android:textSize="25sp" />

        (...)
        // 11 more buttons
    </LinearLayout>

</HorizontalScrollView>

After that, when I scrolled the list without choosing other element, I was losing focus of previously selected element. I made some research about this topic, but there was no solution that could work for me. Finally, after almost two weeks I have created some solution by myself.

I would like you to review it and tell me what can I improve on.

I created a custom HorizontalScrollView class inside and have overridden the onTouchEvent() method. I don't think this is optimal way of doing that, because in that case I have to do calculations every time I move even one pixel. For example, if I add toast.show() to the below method, it will try to show as many toast as many I moved pixels (If I move by 10 pixels, it will try to show 10 Toast). Anyway, it works for me and the selection and focus are being kept. Please help me modify this code to make finally a good answer for that known issue:

@Override
public boolean onTouchEvent(MotionEvent ev)
{
    int i = 0;
    Button button = null;
    for (; i < 11; i++)
    {
        button = (Button)((LinearLayout)getChildAt(0)).getChildAt(i);
        if(button.isSelected())
            break;
    }
    super.onTouchEvent(ev);
    button.setSelected(true);
    button.requestFocusFromTouch();

    return true;
}

To be sure that the above code will work, you need to have only one selected item in your HorizontalScrollView at a time, i.e when you press different Button, you need to make the previous one setSelected(false).

Answer 1

I'm not familiar with Android, so just some generic notes:

1. int i = 0;
   Button button = null;
   for (; i < 11; i++)
   

   I think the most developers more familiar with the following (equal and shorter) form:

   for (int i = 0; i < 11; i++)
   

2. 11 and 0 are magic numbers. Use a named constant instead of them which explains the meaning of these numbers (and probably removes some duplication and makes maintenance easier).

3. I think MotionEvent variable could deserve a little bit longer name for better readability. (Clean Code by Robert C. Martin, Avoid Mental Mapping, p25)

4. You could increase the abstraction level and readability a little bit with a new method whose name contains the purpose of the loop:

   @Override
   public boolean onTouchEvent(MotionEvent event) {
       Button button = getSelectedButton();
       super.onTouchEvent(event);
       button.setSelected(true);
       button.requestFocusFromTouch();
   
       return true;
   }
   
   private Button getSelectedButton() {
       LinearLayout linearLayout = (LinearLayout) getChildAt(0);
       for (int i = 0; i < 11; i++) {
           Button button = (Button) linearLayout.getChildAt(i);
           if (button.isSelected()) {
               return button;
           }
       }
       throw new IllegalStateException("no button selected");
   }
   

   Note that the Button variable get a smaller scope and the linearLayout local variable.

   (Clean Code by Robert C. Martin, One Level of Abstraction per Function, p36; Clean Code by Robert C. Martin, G19: Use Explanatory Variables, p296; Effective Java, Second Edition, Item 45: Minimize the scope of local variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

5. If you return null the button.setSelected() will throw a NullPointerException (or if you don't find any selected button in the original code). I guess having zero selected button is not a valid state of the application. If it somehow happens I'd throw an exception immediately (instead of setting null as the Button reference).

   private Button getSelectedButton() {
       for (int i = 0; i < 11; i++) {
           Button button = (Button) ((LinearLayout) getChildAt(0)).getChildAt(i);
           if (button.isSelected())
               return button;
       }
       throw new IllegalStateException("no button selected");
   }
   

   (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

6. super.onTouchEvent(event);
   button.setSelected(true);
   

   Are you sure that you need setSelected(true) here? It seems that it's already selected before the super call (the for loop returns a selected button) but super.onTouchEvent() might unselect it. If super doesn't change this state you could remove the button.setSelected call.