Skip to content

Override on destory to retain fragments #2979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ashishkumar468 wants to merge 3 commits into from
Closed

Override on destory to retain fragments #2979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eeefb6f
Override on destory to retain fragments
ashishkumar468 Jun 2, 2019
ecffc38
* Donot call retainInstanceState in onCreate of the fragments
ashishkumar468 Jun 2, 2019
5a1a989
Attached image scale listener to upload media image
ashishkumar468 Jun 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@
import android.app.ProgressDialog;
import android.content.Intent;
import android.os.Bundle;
import android.view.View;
import android.view.ViewGroup;
import android.widget.ImageButton;
import android.widget.LinearLayout;
import android.widget.RelativeLayout;
import android.widget.TextView;
import androidx.appcompat.app.AlertDialog;
import androidx.cardview.widget.CardView;
import androidx.fragment.app.Fragment;
Expand All @@ -18,18 +24,6 @@
import androidx.recyclerview.widget.RecyclerView;
import androidx.viewpager.widget.PagerAdapter;
import androidx.viewpager.widget.ViewPager;

import android.view.View;
import android.widget.ImageButton;
import android.widget.LinearLayout;
import android.widget.RelativeLayout;
import android.widget.TextView;
import java.util.ArrayList;
import java.util.List;

import javax.inject.Inject;
import javax.inject.Named;

import butterknife.BindView;
import butterknife.ButterKnife;
import butterknife.OnClick;
Expand All @@ -51,7 +45,11 @@
import fr.free.nrw.commons.utils.PermissionUtils;
import fr.free.nrw.commons.utils.ViewUtil;
import io.reactivex.disposables.CompositeDisposable;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import javax.inject.Inject;
import javax.inject.Named;
import timber.log.Timber;

public class UploadActivity extends BaseActivity implements UploadContract.View ,UploadBaseFragment.Callback{
Expand Down Expand Up @@ -438,6 +436,11 @@ public void setFragments(List<Fragment> fragments) {
public int getItemPosition(Object object){
return PagerAdapter.POSITION_NONE;
}

@Override
public void destroyItem(ViewGroup container, int position, Object object) {
Copy link

@pullrequest pullrequest bot Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit does not give context about why disabling the item destruction is needed, it would be good to at least document that.

The items are going to hold images right? Are you worried that keeping the images (or whatever is in the fragments) around can cause too much memory pressure? Why was a FragmentStatePagerAdapter used in the first place?

Looking at its documentation, there is a mention to FragmentPagerAdapter that does not do any memory clearing. Maybe refactor to use that as base class instead if you really want to disable that? The memory management is a defining feature of FragmentStatePagerAdapter, if would probably be easier to change the base class, and reduce the risks of introducing hard to track bugs.

//I do not want the items to be destroyed, I want extant
}
}


Expand Down
1 change: 0 additions & 1 deletion app/src/main/java/fr/free/nrw/commons/upload/UploadBaseFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ public class UploadBaseFragment extends CommonsDaggerSupportFragment {
@Override
public void onCreate(@Nullable Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setRetainInstance(true);
}

public void setCallback(Callback callback) {
Expand Down
25 changes: 24 additions & 1 deletion app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import android.os.Bundle;
import android.text.TextUtils;
import android.util.Log;
Copy link

@pullrequest pullrequest bot Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused import?

import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
Expand All @@ -16,6 +17,7 @@
import androidx.recyclerview.widget.LinearLayoutManager;
import androidx.recyclerview.widget.RecyclerView;

import com.github.chrisbanes.photoview.OnScaleChangedListener;
import com.github.chrisbanes.photoview.PhotoView;
import com.jakewharton.rxbinding2.widget.RxTextView;

Expand Down Expand Up @@ -155,6 +157,19 @@ private void init() {
} else {
btnCopyPreviousTitleDesc.setVisibility(View.VISIBLE);
}

attachImageViewScaleChangeListener();
Copy link

@pullrequest pullrequest bot Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems irrelevant to the rest of the patch. Mention it in the commit message? Or separate to its own patch?

It's a best practice to make each commit have an easily understandable single purpose:
https://github.com/trein/dev-best-practices/wiki/Git-Commit-Best-Practices#commit-related-changes

}

/**
* Attaches the scale change listener to the image view
*/
private void attachImageViewScaleChangeListener() {
Copy link

@pullrequest pullrequest bot Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the method to something like attachCollapseMediaDetailsOnImageInteractionListener() ?

Ideally a reader of the init() method should know right away that the purpose of the listener is to collapse MediaDetails. Currently the method name does not say anything about its purpose.

photoViewBackgroundImage.setOnScaleChangeListener(
(scaleFactor, focusX, focusY) -> {
//Whenever the uses plays with the image, lets collapse the media detail container
Copy link

@pullrequest pullrequest bot Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/uses/user

Also, this is actually a comment that would be nice to include in the Javadoc, to give a reason why this method is there.

expandCollapseLlMediaDetail(false);
});
}

/**
Expand Down Expand Up @@ -310,7 +325,15 @@ public void onDestroyView() {

@OnClick(R.id.rl_container_title)
public void onRlContainerTitleClicked() {
llContainerMediaDetail.setVisibility(isExpanded ? View.GONE : View.VISIBLE);
expandCollapseLlMediaDetail(!isExpanded);
}

/**
* show hide media detail based on
Copy link

@pullrequest pullrequest bot Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation nits:

  • Javadoc should use Sentence case, terminate with a full stop. (styleguide)

  • @param starts a specific block describing the argument, if you want to refer to an argument in the javadoc body, use @code like:

    /** Show or hide media detail based on {@code shouldExpand}. */

Generally try to optimise the documentation writing for how it renders in the IDE (e.g. Ctrl-Q in Android Studio) or when the HTML javadoc is generated

* @param shouldExpand
*/
private void expandCollapseLlMediaDetail(boolean shouldExpand){
llContainerMediaDetail.setVisibility(shouldExpand ? View.VISIBLE : View.GONE);
isExpanded = !isExpanded;
ibExpandCollapse.setRotation(ibExpandCollapse.getRotation() + 180);
}
Expand Down