sockeqwe / mosby

A Model-View-Presenter / Model-View-Intent library for modern Android apps
http://hannesdorfmann.com/mosby/
Apache License 2.0
5.49k stars 841 forks source link

some problem about error view in MvpActivity #42

Closed zeng1990java closed 9 years ago

zeng1990java commented 9 years ago

I have problem and why the error view is only can be TextView.

sockeqwe commented 9 years ago

TextView covers 95 % of the common use cases and allows Mosby out of the box to set the correct error string to it etc. Please note that you can add Drawables to TextView as described here: http://stackoverflow.com/questions/15352496/how-to-add-image-in-a-textview-text

If you really want to put a custom view as error view then you are on your own. MvpLceFragment and MvpLceActivity implementation requires TextView as error view. However, you can copy and paste the code to plugin in your custom view as error view.

wKovacs64 commented 9 years ago

@sockeqwe Would there be a negative impact on Mosby to use a View rather than a TextView? It would be nice to allow for richer error views like the connectivity example from the error patterns section in the Material design guide.

Automatically setting the error text could remain functional if the appropriate TextView ID was found. This would probably be a new ID, like R.id.errorMessage or something (while the outer View could use R.id.errorView). Might be able to prevent existing apps from breaking by including an instanceof check for when errorView is a TextView?

Alternatively, might there be a way to provide some sort of override functionality? Just trying to figure out a way to avoid depending on a locally modified version of Mosby.

sockeqwe commented 9 years ago

The problem is that a view can only have one id so if there is just one singe TextView you can't have a R.id.errorView and R.id.errorMessage at the same time. Yeah we could check that with if ... else ... instanceof but that leads to very fragile code. Hence it's not an option for me.

From my point of view the simplest way is to write your custom MvpLceFragment or Activity (with or without viewstate). There is really no magic in this "provided" classes and writing your own shouldn't be a big deal: https://github.com/sockeqwe/mosby/blob/6b432c128d5997fd5fb041e7dac2dd4532daa619/viewstate/src/main/java/com/hannesdorfmann/mosby/mvp/viewstate/lce/MvpLceViewStateFragment.java

https://github.com/sockeqwe/mosby/blob/master/mvp/src/main/java/com/hannesdorfmann/mosby/mvp/lce/MvpLceFragment.java

Alternatively, might there be a way to provide some sort of override functionality?

Yes, you could do the following: Wrap R.id.errorView (still a TextView) in a Layout to accomplish the desired design of the error view. There are 3 methods you can override:

Alternatively one could also override showError(), showContent() and showLoading() and place there the animations directly or just set the visibility of the views.

P.S: I think that connectivity example could be implemented with just a single TextView: TextView can have drawables on top of the text, so you could simply put the clouds drawable on top of the text and use ForegroundColorSpan for the "Try Again" button http://stackoverflow.com/questions/3282940/set-color-of-textview-span-in-android. By doing so you can keep your layout hierarchy flat.

wKovacs64 commented 9 years ago

Understood. Thanks.

jemshit commented 8 years ago

My problem is: i hide content view when there is error occured and show error text. Then i can not swipe down the swipeRefreshLayout and refresh the screen to see if i can load content again.

This is content view: unnamed

This is error view: unnamed 1

When i am in state of error, i can't load data again using swiperefresh. It is related to my code implementation but i cannot figure out better way:

<?xml version="1.0" encoding="utf-8"?>
<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent">

    <!-- Loading View -->
    <ProgressBar
        android:id="@+id/loadingView"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:layout_gravity="center"
        android:indeterminate="true" />

    <!-- Content View -->
    <android.support.v4.widget.SwipeRefreshLayout
        android:id="@+id/contentView"
        android:layout_width="match_parent"
        android:layout_height="match_parent">

        <android.support.v7.widget.RecyclerView
            android:id="@+id/recyclerView"
            android:layout_width="match_parent"
            android:layout_height="match_parent" />

    </android.support.v4.widget.SwipeRefreshLayout>

    <!-- Error view -->
    <TextView
        android:id="@+id/errorView"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content" />

</FrameLayout>

So if i hide contentView, swipeRefresh is gone and there is no way to refresh the screen to retry loading data. My goal is to be able to use swiperefresh even content is not loaded, is loaded, error view is shown or not. In all states i want to refresh the screen.

jemshit commented 8 years ago

In the sample email app, you have asked user to click on errorView to retry load content. unnamed 2

Since swipeRefresh is default user behavior to load data, i don't want user to click error text in order to retry. Let them able to be use swiperefresh again. And question is: is there any way to achieve this?

sockeqwe commented 8 years ago

I see. Well the idea of LCE classes (Loading-Error-Content) is to change (animate) the visibility each view R.id.errorView, R.id.loadingView and R.id.contentView so that in each state only 1 of the 3 views is visible at the same time on screen. However, contentView can be any Android ui view. So instead of defining SwipeRefreshLayout as contentView make the RecyclerView your contentView and like this:

<?xml version="1.0" encoding="utf-8"?>
<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent">

    <!-- Loading View -->
    <ProgressBar
        android:id="@+id/loadingView"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:layout_gravity="center"
        android:indeterminate="true" />

    <android.support.v4.widget.SwipeRefreshLayout
        android:id="@+id/swiperefreshLayout"
        android:layout_width="match_parent"
        android:layout_height="match_parent">

        <!-- Content View -->
        <android.support.v7.widget.RecyclerView
            android:id="@+id/contentView"
            android:layout_width="match_parent"
            android:layout_height="match_parent" />

    </android.support.v4.widget.SwipeRefreshLayout>

    <!-- Error view -->
    <TextView
        android:id="@+id/errorView"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content" />

</FrameLayout>

In that case swiperefreshlayout will always be visible and on top of everything. then it's up to you to decide when to display or hide the swiperefreshlayout or enable swiperefreshlayout gesture.

but that's only one possible way. You can also stick with swiperefreshlayout as contentView and override showContent() showError() and showLoading() to define which view will be shown or hidden in each method call.

as already said, LCE classes are designed to work this way. But if LCE classes are not solving your problem or you have to write workarounds, then simply don't use LCE as base class to extend from. Write your own class that don't extend from LCE but rather from MvpActivity or MvpFragment to achieve whatever you want to implement. Don't fight against LCE classes! if they simply don't fit your needs write your own one.

jemshit commented 8 years ago

If i make recyclerView as contentView, i think there will be conflict, Since library uses code contentView.setRefreshing(true) in some places. So i did some workaround now and changed implementation of showError.

Instead of this:

 @BindDrawable(R.drawable.error)
    Drawable errorDrawable;
    @Bind(R.id.layoutAll)
    FrameLayout layoutView;
    ...

@Override public void showError(Throwable e, boolean pullToRefresh) {
    super.showError(e, pullToRefresh);
    contentView.setRefreshing(false);
  }

I have done this, i copied some code from showError() inside LceAnimator.java. Also added drawable on top of text:

@Override public void showError(Throwable e, boolean pullToRefresh) {
        showErrorView(e);
    }

    private void showErrorView(Throwable e){
         // Add drawable
        errorView.setCompoundDrawablesWithIntrinsicBounds(null, errorDrawable, null, null);
        // This is for allowing swipeRefreshLayout to get touch event when i try to swipe down 
        // on any coordinate of text or drawable.
        errorView.setOnTouchListener(new View.OnTouchListener() {
            @Override
            public boolean onTouch(View v, MotionEvent event) {
                int action = event.getAction();
                switch (action) {
                    case MotionEvent.ACTION_DOWN:
                        layoutView.dispatchTouchEvent(event);
                        break;

                    case MotionEvent.ACTION_UP:
                        v.getParent().requestDisallowInterceptTouchEvent(false);
                        break;
                }
                v.onTouchEvent(event);
                return true;
            }
        });
       // Not visible yet, so animate the view in
        errorView.setText(e.getMessage());

        adapter.setCountries(null);
        contentView.setVisibility(View.VISIBLE);
        contentView.setRefreshing(false);

        // Rest is Same code
        AnimatorSet set = new AnimatorSet();
        ObjectAnimator in = ObjectAnimator.ofFloat(errorView, "alpha", 1f);
        ObjectAnimator loadingOut = ObjectAnimator.ofFloat(loadingView, "alpha", 0f);

        set.playTogether(in, loadingOut);
        set.setDuration(200);

        set.addListener(new AnimatorListenerAdapter() {

            @Override public void onAnimationStart(Animator animation) {
                super.onAnimationStart(animation);
                errorView.setVisibility(View.VISIBLE);
            }

            @Override public void onAnimationEnd(Animator animation) {
                super.onAnimationEnd(animation);
                loadingView.setVisibility(View.GONE);
                loadingView.setAlpha(1f); // For future showLoading calls
            }
        });

        set.start();
    }

The Trick is i don't hide contentView, so the swipeRefreshLayout. I just make recyclerView to have 0 elements if there is error. (swipeRefreshLayout wll have height "wrap_content")

fragment.xml

<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:id="@+id/layoutAll"
    android:layout_height="match_parent">

    <!-- Loading View -->
    <ProgressBar
        android:id="@+id/loadingView"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:layout_gravity="center"
        android:indeterminate="true" />

    <!-- Content View -->
    <android.support.v4.widget.SwipeRefreshLayout
        android:id="@+id/contentView"
        android:layout_width="match_parent"
        android:layout_height="wrap_content">

        <android.support.v7.widget.RecyclerView
            android:id="@+id/recyclerView"
            android:layout_width="match_parent"
            android:layout_height="match_parent" />

    </android.support.v4.widget.SwipeRefreshLayout>

    <!-- Error view -->
    <TextView
        android:id="@+id/errorView"
        android:layout_gravity="center"
        android:textSize="20sp"
        android:textColor="@color/textDarkSecondary"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content" />

</FrameLayout>

adapter.java

public class CountriesAdapter extends RecyclerView.Adapter<CountriesAdapter.CountryViewHolder> {
    Context context;
    List<Country> list;
    ...
     public void setCountries(List<Country> newList){
        if(newList==null){
            list.clear();
        }else {
            list = newList;
        }
        notifyDataSetChanged();
    }

Thanks.