[android] Fixed the crash while clicking on Deleted Bookmarks

[abhibana]

On Bookmarks Lists page, if a User has created a new List along with My Places List and then deletes it, the deleted item stays on the List. Now, if the user clicks on the deleted List Item then the app was crashing as the deleted item is unavailable. Root cause of this issue is below -

• When a category is deleted then deleteCategory() of BookmarkManager gets called.
• The deleteCategory() internally calls nativeDeleteCategory().
• After a category is successfully deleted, then OnBookmarksChanged() should be invoked in order to update the List. But this wasn't happening. So deleted category wasn't getting removed from the cached list. Thus the user deleted category was visible even after deletion.
• When the user clicked on the deleted category, it'll eventually crash as the category itself isn't present

Fix: Added a OnBookmarksChanged() call invocation on successful deletion of category
Fixes: #3828

Issue	Fix

[Jean-BaptisteC]

✅ Pixel 6 - Android 13

[abhibana]

Hi @biodranik, can you please review and merge this PR if it looks ok. It's been pending over 2 weeks.

[biodranik]

Looks like there is a bug in the C++ bookmark_manager. The notification is not sent from the core because the if before it doesn't detect deleted category event (see the code below).

void BookmarkManager::NotifyChanges()
{
  CHECK_THREAD_CHECKER(m_threadChecker, ());

  m_changesTracker.AcceptDirtyItems();
  // TODO: This if does not detect when bookmark list/group/category is deleted.
  if (!m_firstDrapeNotification &&
    !m_changesTracker.HasChanges() &&
    !m_bookmarksChangesTracker.HasChanges() &&
    !m_drapeChangesTracker.HasChanges())
  {
    return;
  }

  if (m_changesTracker.HasBookmarksChanges())
    NotifyBookmarksChanged();

  if (m_changesTracker.HasCategoriesChanges())
    NotifyCategoriesChanged();
  // ...

Note also that there is a separate NotifyCategoriesChanged notification that is not used by Android and iOS code.

Looks like the code was written by different people, who did not implement it properly.

On iOS, for example, the UI code manually removes the deleted list item and do not use any subscription at all.

A proper fix is likely to fix notification and leverage correct notification for categories/lists (not for bookmarks). And also do it properly on iOS.

A better quick-and-dirty workaround than the existing in this PR could be to delete the list item from the Android UI code without using such notifications. WDYT?

[abhibana]

@biodranik, TBH I don't think we can do the removal of the list item from the Android UI Code as the underlying dataset for Android List is backed by BookmarkManager data. Unless the item is removed from the original list, it won't reflect in the Android UI. However, the removal of the item is asynchronous. Even if I presumably refresh the dataset by calling BookmarkManager.INSTANCE.getCategories(), it will not work as the underlying dataset might not have changed by that time. That's why the listener based approach is perfect.

Anyways, the Android code is heavily using notifyDataSetChanged() heavily which isn't the recommended pattern. We should have used notifyItemRemoved() but in order to do that a big change has to be made both on the Android Adapter side and as well as the Data layer. In my opinion, having the OnBookmarksChanged() being invoked from JNI code is better solution than hacking it from Android UI.

Let me know WDYT ??

[biodranik]

What about fixing the C++ code by sending the notification, as I suggested, in the right way?

[abhibana]

That is the best approach. However, the current solution is only going to impact Android side of things. As bookmark_manager.cpp is shared between both Android and iOS. I don't have full context of the C++ code and how your proposed changes is going to get consumed by the JNI layer and passed onto the Android / iOS App. If you've any pointers, I can definitely check.

[biodranik]

iOS doesn't use this code. But it can start using it later. Or it can be used on desktops. Please fix the non-working c++ notification.

[rtsisyk]

Hi @abhibana , did you have a chance to take a look at this issue?

[rtsisyk]

up. Please rebase at least. I can't do that via UI :(

[biodranik]

Thanks for your investigation, @abhibana !
I've fixed the missing C++ notification, it wasn't an obvious change. A debugger was helpful.

@vng @rtsisyk PTAL

[vng]

@biodranik I'm unfamiliar with this logic, but isn't the same case here? Can you please check if you are already in a context?

void BookmarkManager::MarksChangesTracker::OnDeleteLine(kml::TrackId lineId)
{
  auto const it = m_createdLines.find(lineId);
  if (it != m_createdLines.end())
    m_createdLines.erase(it);
  else
    m_removedLines.insert(lineId);
}

[biodranik]

@vng there is no way to delete the track right now. And there is no way to add/create a track from the bookmarks dialog now. So this case will work. I propose to review and test this logic later, when we implement track recording and deleting. It looks unnecessarily complex.

[rtsisyk]

Thanks for the contribution!!