Skip to content
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

Jump to bottom

Fixes #3639 (Fix Save State implementation of CheckBoxTriState ) #3686

Merged
merged 9 commits into from
Apr 26, 2020
Merged

Fixes #3639 (Fix Save State implementation of CheckBoxTriState ) #3686

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
31c479b
Fixes #3639
ashishkumar468 Apr 21, 2020
28f4aaa
removed unused imports in CheckBoxTriStates
ashishkumar468 Apr 21, 2020
cd3dc1c
set CheckBoxTriState onViewStateRestored
ashishkumar468 Apr 21, 2020
431b2d2
Set Callback for CheckBoxTriState in onViewCreated as well, it maybe …
ashishkumar468 Apr 22, 2020
9f0f132
bug-fix restore check box state
ashishkumar468 Apr 22, 2020
ebe2511
donot save checkbox state
ashishkumar468 Apr 23, 2020
f0f4e18
donot save checkbox's state in onSaveInstanceState
ashishkumar468 Apr 23, 2020
5a9932f
reset changes in ProjectCodeStyleConfiguration
ashishkumar468 Apr 24, 2020
95507bc
reset changes in NearbyParentFragment
ashishkumar468 Apr 24, 2020
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
72 changes: 1 addition & 71 deletions app/src/main/java/fr/free/nrw/commons/nearby/CheckBoxTriStates.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package fr.free.nrw.commons.nearby;

import android.content.Context;
import android.os.Parcel;
import android.os.Parcelable;
import android.util.AttributeSet;
import android.widget.CompoundButton;

Expand All @@ -12,7 +10,6 @@
import java.util.List;

import fr.free.nrw.commons.R;
import fr.free.nrw.commons.nearby.presenter.NearbyParentFragmentPresenter;

/**
* Base on https://stackoverflow.com/a/40939367/3950497 answer.
Expand Down Expand Up @@ -64,12 +61,6 @@ public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
*/
private OnCheckedChangeListener clientListener;

/**
* This flag is needed to avoid accidentally changing the current {@link #state} when
* {@link #onRestoreInstanceState(Parcelable)} calls {@link #setChecked(boolean)}
* evoking our {@link #privateListener} and therefore changing the real state.
*/
private boolean restoring;

public CheckBoxTriStates(Context context) {
super(context);
Expand All @@ -91,7 +82,7 @@ public int getState() {
}

public void setState(int state) {
if(!this.restoring && this.state != state) {
if(this.state != state) {
this.state = state;

if(this.clientListener != null) {
Expand All @@ -118,27 +109,6 @@ public void setOnCheckedChangeListener(@Nullable OnCheckedChangeListener listene
super.setOnCheckedChangeListener(privateListener);
}

@Override
public Parcelable onSaveInstanceState() {
Parcelable superState = super.onSaveInstanceState();

SavedState ss = new SavedState(superState);

ss.state = state;

return ss;
}

@Override
public void onRestoreInstanceState(Parcelable state) {
this.restoring = true; // indicates that the ui is restoring its state
SavedState ss = (SavedState) state;
super.onRestoreInstanceState(ss.getSuperState());
setState(ss.state);
requestLayout();
this.restoring = false;
}

private void init() {
state = UNKNOWN;
updateBtn();
Expand All @@ -164,44 +134,4 @@ private void updateBtn() {
setButtonDrawable(btnDrawable);

}

static class SavedState extends BaseSavedState {
int state;

SavedState(Parcelable superState) {
super(superState);
}

private SavedState(Parcel in) {
super(in);
state = in.readInt();
}

@Override
public void writeToParcel(Parcel out, int flags) {
super.writeToParcel(out, flags);
out.writeValue(state);
}

@Override
public String toString() {
return "CheckboxTriState.SavedState{"
+ Integer.toHexString(System.identityHashCode(this))
+ " state=" + state + "}";
}

@SuppressWarnings("hiding")
public static final Parcelable.Creator<SavedState> CREATOR =
new Parcelable.Creator<SavedState>() {
@Override
public SavedState createFromParcel(Parcel in) {
return new SavedState(in);
}

@Override
public SavedState[] newArray(int size) {
return new SavedState[size];
}
};
}
}
Empty file removed app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyListFragment.java
View file
Open in desktop
Empty file.
Empty file removed app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyMapFragment.java
View file
Open in desktop
Empty file.
30 changes: 23 additions & 7 deletions app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,9 @@

public class NearbyParentFragment extends CommonsDaggerSupportFragment
implements NearbyParentFragmentContract.View,
WikidataEditListener.WikidataP18EditListener, LocationUpdateListener {
WikidataEditListener.WikidataP18EditListener, LocationUpdateListener ,CheckBoxTriStates.Callback{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect zero changes to this file, if we just remove the state restoration in CheckboxTriState that is the minimal change to solve this issue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@macgills We have removed the manual state restoration thing, but the android system also restores the view right so if we don't set the callback in such cases, it will throw an NPE, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is called on master after onViewCreated

checkBoxTriStates.setCallback((o, state, b, b1) -> presenter.filterByMarkerType(o,state,b,b1));

There was an internal problem where the checkbox will throw an NPE if you don't set a click listener to it and it processes a click event, that should not be the case. That problem arose because the state restoration was happening before the click listener was set and your version of state restoration didn't use the isRestoring flag to prevent the state restoration being interpreted as human input.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still halfway through my morning coffee, that wasn't a great explanation...

I'll perk up in a bit if you have any questions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair point, done


private static final String CHECKBOX_STATE = "checkbox_state";
@BindView(R.id.bottom_sheet) RelativeLayout rlBottomSheet;
@BindView(R.id.bottom_sheet_details) View bottomSheetDetails;
@BindView(R.id.transparentView) View transparentView;
Expand Down Expand Up @@ -220,9 +221,15 @@ public View onCreateView(@NonNull LayoutInflater inflater, ViewGroup container,
@Override
public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) {
super.onViewCreated(view, savedInstanceState);
checkBoxTriStates.setCallback(this);
//Restore checkbox's state
if (null != savedInstanceState) {
int checkBoxSavedState = savedInstanceState.getInt(CHECKBOX_STATE,
CheckBoxTriStates.UNKNOWN);
checkBoxTriStates.setState(checkBoxSavedState);
}
isDarkTheme = systemThemeUtils.isDeviceInNightMode();
cameraMoveListener= () -> presenter.onCameraMove(mapBox.getCameraPosition().target);
addCheckBoxCallback();
presenter.attachView(this);
initRvNearbyList();
initThemePreferences();
Expand Down Expand Up @@ -285,10 +292,6 @@ private void initRvNearbyList() {
rvNearbyList.setAdapter(adapterFactory.create(null));
}

private void addCheckBoxCallback() {
checkBoxTriStates.setCallback((o, state, b, b1) -> presenter.filterByMarkerType(o,state,b,b1));
}

private void performMapReadyActions() {
if (isVisible() && isVisibleToUser && isMapBoxReady) {
checkPermissionsAndPerformAction(() -> {
Expand Down Expand Up @@ -363,6 +366,7 @@ public void onStop() {

@Override
public void onSaveInstanceState(Bundle outState) {
outState.putInt(CHECKBOX_STATE,checkBoxTriStates.getState());
super.onSaveInstanceState(outState);
mapView.onSaveInstanceState(outState);
}
Expand Down Expand Up @@ -636,7 +640,7 @@ public LatLng getLastFocusLocation() {

@Override
public boolean isCurrentLocationMarkerVisible() {
if (latLngBounds == null) {
if (latLngBounds == null || currentLocationMarker==null) {
Timber.d("Map projection bounds are null");
return false;
} else {
Expand Down Expand Up @@ -962,6 +966,12 @@ public void backButtonClicked() {
presenter.backButtonClicked();
}

@Override
public void filterByMarkerType(@Nullable List<Label> selectedLabels, int state, boolean b,
boolean b1) {
presenter.filterByMarkerType(selectedLabels,state,b,b1);
}

/**
* onLogoutComplete is called after shared preferences and data stored in local database are cleared.
*/
Expand Down Expand Up @@ -1502,4 +1512,10 @@ private void startTheMap() {
mapView.onStart();
performMapReadyActions();
}

@Override
public void onViewStateRestored(@Nullable Bundle savedInstanceState) {
super.onViewStateRestored(savedInstanceState);
checkBoxTriStates.setCallback(this);
}
}