[ios] Safe implementation of bookmark/category pair, fixes crash

iphone/Maps/Bookmarks/BalloonView.h

@@ -10,7 +10,7 @@
 
   Framework::AddressInfo m_addressInfo;
   // If we clicked already existing bookmark, it will be here
-  BookmarkAndCategory m_rawBookmark;
+  BookmarkAndCategory m_editedBookmark;
 }
 
 @property(nonatomic, retain) NSString * title;

iphone/Maps/Bookmarks/BalloonView.mm

@@ -142,14 +142,14 @@
 
   isDisplayed = YES;
 
-  m_rawBookmark = bm;
+  m_editedBookmark = bm;
 
   CGFloat const w = self.pinImage.bounds.size.width;
   CGFloat const h = self.pinImage.bounds.size.height;
   self.pinImage.frame = CGRectMake(pt.x - w/2, pt.y - h, w, h);
   // Do not show pin if we're editing existing bookmark.
   // @TODO move pin (and probably balloon drawing) to cross-platform code
-  self.pinImage.hidden = (m_rawBookmark.second != 0);
+  self.pinImage.hidden = IsValid(m_editedBookmark);
 
   [view addSubview:self.pinImage];
 
@@ -225,21 +225,14 @@
 
 - (void) deleteBMHelper
 {
-  BookmarkCategory * cat = m_rawBookmark.first;
-  Bookmark const * bm = m_rawBookmark.second;
-  if (cat && bm)
+  if (IsValid(m_editedBookmark))
   {
-    for (size_t i = 0; i < cat->GetBookmarksCount(); ++i)
-    {
-      if (cat->GetBookmark(i) == bm)
-      {
-        cat->DeleteBookmark(i);
-        break;
-      }
-    }
+    BookmarkCategory * cat = GetFramework().GetBmCategory(m_editedBookmark.first);
+    if (cat)
+      cat->DeleteBookmark(m_editedBookmark.second);
+    // Clear!
+    m_editedBookmark = MakeEmptyBookmarkAndCategory();
   }
-  // Clear!
-  m_rawBookmark = BookmarkAndCategory(0, 0);
 }
 
 - (void) addOrEditBookmark

iphone/Maps/Classes/MapViewController.mm

@@ -141,8 +141,10 @@
   CGPoint const pixelPos = [point CGPointValue];
   CGFloat const scaleFactor = self.view.contentScaleFactor;
 
-  BookmarkAndCategory res = GetFramework().GetBookmark(m2::PointD(pixelPos.x * scaleFactor, pixelPos.y * scaleFactor));
-  if (!res.first || !res.second)
+  Framework & f = GetFramework();
+  BookmarkAndCategory const res = f.GetBookmark(m2::PointD(pixelPos.x * scaleFactor, pixelPos.y * scaleFactor));
+
+  if (!IsValid(res))
   {
     if (m_bookmark.isDisplayed)
       [m_bookmark hide];
@@ -156,13 +158,21 @@
   else
   {
     // Already added bookmark was clicked
-    m2::PointD const globalPos = res.second->GetOrg();
-    m_bookmark.globalPosition = CGPointMake(globalPos.x, globalPos.y);
-    // Override bookmark name which was set automatically according to the point address in previous line
-    m_bookmark.title = [NSString stringWithUTF8String:res.second->GetName().c_str()];
-    m_bookmark.color = [NSString stringWithUTF8String:res.second->GetType().c_str()];
-    m_bookmark.setName = [NSString stringWithUTF8String:res.first->GetName().c_str()];
-    [m_bookmark showInView:self.view atPoint:[self globalPoint2ViewPoint:m_bookmark.globalPosition] withBookmark:res];
+    BookmarkCategory const * cat = f.GetBmCategory(res.first);
+    if (cat)
+    {
+      Bookmark const * bm = cat->GetBookmark((size_t)res.second);
+      if (bm)
+      {
+        m2::PointD const globalPos = bm->GetOrg();
+        m_bookmark.globalPosition = CGPointMake(globalPos.x, globalPos.y);
+        // Override bookmark name which was set automatically according to the point address in previous line
+        m_bookmark.title = [NSString stringWithUTF8String:bm->GetName().c_str()];
+        m_bookmark.color = [NSString stringWithUTF8String:bm->GetType().c_str()];
+        m_bookmark.setName = [NSString stringWithUTF8String:cat->GetName().c_str()];
+        [m_bookmark showInView:self.view atPoint:[self globalPoint2ViewPoint:m_bookmark.globalPosition] withBookmark:res];
+      }
+    }
   }
 }
 

map/bookmark.cpp

@@ -41,6 +41,10 @@ void BookmarkCategory::DeleteBookmark(size_t index)
     if (!m_file.empty())
       (void)SaveToKMLFileAtPath(m_file);
   }
+  else
+  {
+    LOG(LWARNING, ("Trying to delete non-existing bookmark in category", GetName(), "at index", index));
+  }
 }
 
 Bookmark const * BookmarkCategory::GetBookmark(size_t index) const

map/bookmark.hpp

@@ -69,10 +69,14 @@ public:
   static string GenerateUniqueFileName(const string & path, string const & name);
 };
 
-/// Non-const category is needed to "edit" bookmark (actually, re-add it)
-typedef pair<BookmarkCategory *, Bookmark const *> BookmarkAndCategory;
+/// <category name, bookmark index>
+typedef pair<string, int> BookmarkAndCategory;
 inline BookmarkAndCategory MakeEmptyBookmarkAndCategory()
 {
-  return BookmarkAndCategory(reinterpret_cast<BookmarkAndCategory::first_type>(0),
-                          reinterpret_cast<BookmarkAndCategory::second_type>(0));
+  return BookmarkAndCategory(string(), int(-1));
+}
+
+inline bool IsValid(BookmarkAndCategory const & bmc)
+{
+  return (!bmc.first.empty() && bmc.second >= 0);
 }

map/framework.cpp

@@ -372,8 +372,8 @@ BookmarkAndCategory Framework::GetBookmark(m2::PointD pt, double visualScale) co
   int const sm = 30 * visualScale;
   m2::RectD rect(PtoG(m2::PointD(pt.x - sm, pt.y - sm)), PtoG(m2::PointD(pt.x + sm, pt.y + sm)));
 
-  Bookmark const * retBookmark = 0;
-  BookmarkCategory * retBookmarkCategory = 0;
+  int retBookmark = -1;
+  string retBookmarkCategory;
   double minD = numeric_limits<double>::max();
 
   for (size_t i = 0; i < m_bookmarks.size(); ++i)
@@ -389,8 +389,8 @@ BookmarkAndCategory Framework::GetBookmark(m2::PointD pt, double visualScale) co
         double const d = rect.Center().SquareLength(pt);
         if (d < minD)
         {
-          retBookmark = bm;
-          retBookmarkCategory = m_bookmarks[i];
+          retBookmark = static_cast<int>(j);
+          retBookmarkCategory = m_bookmarks[i]->GetName();
           minD = d;
         }
       }

map/map_tests/bookmarks_test.cpp

@@ -177,17 +177,17 @@ UNIT_TEST(Bookmarks_Getting)
   fm.AddBookmark("cat3", Bookmark(m2::PointD(41, 40), "3", "placemark-red"));
 
   BookmarkAndCategory res = fm.GetBookmark(pixC, 1.0);
-  TEST(res.second != 0, ());
-  TEST(res.first != 0, ());
-  TEST_EQUAL(res.second->GetName(), "2", ());
-  TEST_EQUAL(res.first->GetName(), "cat2" , ());
+  TEST_NOT_EQUAL(res.second, -1, ());
+  TEST(!res.first.empty(), ());
+  TEST_EQUAL(res.second, 1, ());
+  TEST_EQUAL(res.first, "cat2" , ());
 
   res = fm.GetBookmark(m2::PointD(0, 0));
-  TEST(res.first == 0, ());
-  TEST(res.second == 0, ());
+  TEST(res.first.empty(), ());
+  TEST_EQUAL(res.second, -1, ());
   res = fm.GetBookmark(m2::PointD(800, 400));
-  TEST(res.first == 0, ());
-  TEST(res.second == 0, ());
+  TEST(res.first.empty(), ());
+  TEST_EQUAL(res.second, -1, ());
 }
 
 UNIT_TEST(Bookmarks_AddressInfo)