Skip to content

Conversation

ecgreb
Copy link
Collaborator

@ecgreb ecgreb commented Mar 15, 2017

Overview

Properly handle place picker dialog on configuration change so state is persisted and dialog is not leaked.

Proposed Changes

  • Converts place picker dialog from standalone AlertDialog to a DialogFragment to maintain view state.
  • Add PlaceStore singleton to persist current selection data through configuration change when PlacePickerPresenter is destroyed and recreated along with PlacePickerActivity.
  • Updates sample app to (naively) handle configuration change by preventing launching another instance of the place picker if one is already active.

Fixes #324

public class PlaceDialogFragment extends DialogFragment implements DialogInterface.OnClickListener {
public static final String TAG = PlaceDialogFragment.class.getSimpleName();

private static final String ARG_NAME_TITLE = "title";
Copy link
Member

Choose a reason for hiding this comment

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

To be more consistent with the property set on the dialog and with the setMessage method, we should change this to ARG_NAME_MESSAGE

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. Updated!

* @param title text to be displayed in the dialog.
* @return a new instance of the dialog fragment.
*/
public static PlaceDialogFragment newInstance(String title) {
Copy link
Member

Choose a reason for hiding this comment

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

Related to above, we should change the param name from title to message

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also fixed

@sarahsnow1 sarahsnow1 merged commit 70b399c into master Mar 17, 2017
@sarahsnow1 sarahsnow1 deleted the 324-leaky-dialog branch March 17, 2017 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants