[android] Fix bookmark edits not updating in place page on screen rotation #10260

pastk · 2025-02-15T12:27:04Z

savsch commented

2025-02-15 12:27:04 +00:00

(Migrated from github.com)

The callback listener supposed to be invoked after bookmark edits (mListener in EditBookmarkFragment) was made null on configuration changes. This change fixes the issue by leveraging the fact that the parent fragment (for cases when the issue occurs, i.e. PlacePageBookmarkFragment) itself implements the listener.

On master branch

https://github.com/user-attachments/assets/74f786e1-fbed-4bab-9efe-fae4d45985fc

On this branch

https://github.com/user-attachments/assets/77a27cd3-341c-4cbb-840d-7cec117c84a5

Fixes #2418 The callback listener supposed to be invoked after bookmark edits (`mListener` in `EditBookmarkFragment`) was made null on configuration changes. This change fixes the issue by leveraging the fact that the parent fragment (for cases when the issue occurs, i.e. `PlacePageBookmarkFragment`) itself implements the listener. ### On master branch https://github.com/user-attachments/assets/74f786e1-fbed-4bab-9efe-fae4d45985fc ### On this branch https://github.com/user-attachments/assets/77a27cd3-341c-4cbb-840d-7cec117c84a5

🎉 1

pastk approved these changes 2025-02-22 11:52:37 +00:00

pastk left a comment

Thanks for the fix!!

pastk reviewed 2025-02-22 12:08:34 +00:00

android/app/src/main/java/app/organicmaps/widget/placepage/EditBookmarkFragment.java

					
				@ -184,0 +185,4 @@

				  public void onAttach(@NonNull Context context)

				  {

				    super.onAttach(context);

				    if (mListener == null && getParentFragment() instanceof EditBookmarkListener)

pastk commented

2025-02-22 12:08:34 +00:00

Could you please add a comment explaining why this is necessary.

Also I wonder why there is no issue with "lost" listener when a parent is BookmarksListFragment

Could you please add a comment explaining why this is necessary. Also I wonder why there is no issue with "lost" listener when a parent is BookmarksListFragment

rtsisyk reviewed 2025-02-28 20:03:29 +00:00

rtsisyk left a comment

Please add a comment at least.

savsch (Migrated from github.com) reviewed 2025-03-01 22:31:21 +00:00

android/app/src/main/java/app/organicmaps/widget/placepage/EditBookmarkFragment.java

					
				@ -184,0 +185,4 @@

				  public void onAttach(@NonNull Context context)

				  {

				    super.onAttach(context);

				    if (mListener == null && getParentFragment() instanceof EditBookmarkListener)

savsch (Migrated from github.com) commented

2025-03-01 22:31:21 +00:00

Sorry to keep you waiting; been very busy the last few days.

why there is no issue with "lost" listener when a parent is BookmarksListFragment

BookmarkListActivity has a configChanges="orientation|..." unlike MwmActivity where the issue occurs.

Also, now that I think about it a bit more, I shouldn't have resorted to the onAttach workaround.

There's an arguably cleaner way to fix this:

PlacePageBookmarkFragment.java
@@ -19,6 +19,7 @@ import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.fragment.app.Fragment;
 import androidx.fragment.app.FragmentActivity;
+import androidx.fragment.app.FragmentFactory;
 import androidx.lifecycle.Observer;
 import androidx.lifecycle.ViewModelProvider;
 import app.organicmaps.R;
@@ -120,6 +121,25 @@ public class PlacePageBookmarkFragment extends Fragment implements View.OnClickL
     }
   }
 
+  @Override
+  public void onCreate(@Nullable Bundle savedInstanceState)
+  {
+    // Custom FragmentFactory to prevent EditBookmarkFragment's mListener
+    // from becoming null on configuration changes
+    getChildFragmentManager().setFragmentFactory(new FragmentFactory()
+    {
+      @NonNull
+      @Override
+      public Fragment instantiate(@NonNull ClassLoader classLoader, @NonNull String className)
+      {
+        Class<? extends Fragment> fragmentClass = loadFragmentClass(classLoader, className);
+        Fragment fragment = super.instantiate(classLoader, className);
+        if (fragmentClass == EditBookmarkFragment.class)
+          ((EditBookmarkFragment) fragment).setEditBookmarkListener(PlacePageBookmarkFragment.this);
+        return fragment;
+      }
+    });
+    super.onCreate(savedInstanceState);
+  }
+
   @Override
   public void onClick(View v)
   {
@@ -127,7 +147,7 @@ public class PlacePageBookmarkFragment extends Fragment implements View.OnClickL
     EditBookmarkFragment.editBookmark(currentBookmark.getCategoryId(),
                                       currentBookmark.getBookmarkId(),
                                       activity,
-                                      activity.getSupportFragmentManager(),
+                                      getChildFragmentManager(),
                                       PlacePageBookmarkFragment.this);
   }

Should I push this instead?

Sorry to keep you waiting; been very busy the last few days. --- > why there is no issue with "lost" listener when a parent is BookmarksListFragment `BookmarkListActivity` has a `configChanges="orientation|..."` unlike `MwmActivity` where the issue occurs. --- Also, now that I think about it a bit more, I shouldn't have resorted to the `onAttach` workaround. There's an arguably cleaner way to fix this: ```diff PlacePageBookmarkFragment.java @@ -19,6 +19,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.fragment.app.Fragment; import androidx.fragment.app.FragmentActivity; +import androidx.fragment.app.FragmentFactory; import androidx.lifecycle.Observer; import androidx.lifecycle.ViewModelProvider; import app.organicmaps.R; @@ -120,6 +121,25 @@ public class PlacePageBookmarkFragment extends Fragment implements View.OnClickL } } + @Override + public void onCreate(@Nullable Bundle savedInstanceState) + { + // Custom FragmentFactory to prevent EditBookmarkFragment's mListener + // from becoming null on configuration changes + getChildFragmentManager().setFragmentFactory(new FragmentFactory() + { + @NonNull + @Override + public Fragment instantiate(@NonNull ClassLoader classLoader, @NonNull String className) + { + Class<? extends Fragment> fragmentClass = loadFragmentClass(classLoader, className); + Fragment fragment = super.instantiate(classLoader, className); + if (fragmentClass == EditBookmarkFragment.class) + ((EditBookmarkFragment) fragment).setEditBookmarkListener(PlacePageBookmarkFragment.this); + return fragment; + } + }); + super.onCreate(savedInstanceState); + } + @Override public void onClick(View v) { @@ -127,7 +147,7 @@ public class PlacePageBookmarkFragment extends Fragment implements View.OnClickL EditBookmarkFragment.editBookmark(currentBookmark.getCategoryId(), currentBookmark.getBookmarkId(), activity, - activity.getSupportFragmentManager(), + getChildFragmentManager(), PlacePageBookmarkFragment.this); } ``` Should I push this instead?

pastk reviewed 2025-03-02 05:09:31 +00:00

android/app/src/main/java/app/organicmaps/widget/placepage/EditBookmarkFragment.java

					
				@ -184,0 +185,4 @@

				  public void onAttach(@NonNull Context context)

				  {

				    super.onAttach(context);

				    if (mListener == null && getParentFragment() instanceof EditBookmarkListener)