From 077008f5c1704ea5caf2493dd17e2e83efd13c6e Mon Sep 17 00:00:00 2001 From: Alex Zolotarev Date: Tue, 11 Sep 2012 22:44:56 +0300 Subject: [PATCH] [ios] Safe implementation of bookmark/category pair, fixes crash --- iphone/Maps/Bookmarks/BalloonView.h | 2 +- iphone/Maps/Bookmarks/BalloonView.mm | 23 +++++++------------ iphone/Maps/Classes/MapViewController.mm | 28 ++++++++++++++++-------- map/bookmark.cpp | 4 ++++ map/bookmark.hpp | 12 ++++++---- map/framework.cpp | 8 +++---- map/map_tests/bookmarks_test.cpp | 16 +++++++------- 7 files changed, 52 insertions(+), 41 deletions(-) diff --git a/iphone/Maps/Bookmarks/BalloonView.h b/iphone/Maps/Bookmarks/BalloonView.h index aaaa3d7cdb..4aafdcd9b9 100644 --- a/iphone/Maps/Bookmarks/BalloonView.h +++ b/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; diff --git a/iphone/Maps/Bookmarks/BalloonView.mm b/iphone/Maps/Bookmarks/BalloonView.mm index 88baefad17..8fa0972dc8 100644 --- a/iphone/Maps/Bookmarks/BalloonView.mm +++ b/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 diff --git a/iphone/Maps/Classes/MapViewController.mm b/iphone/Maps/Classes/MapViewController.mm index cad86acf60..22e43e9aec 100644 --- a/iphone/Maps/Classes/MapViewController.mm +++ b/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]; + } + } } } diff --git a/map/bookmark.cpp b/map/bookmark.cpp index 0bd89e965d..093ee50869 100644 --- a/map/bookmark.cpp +++ b/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 diff --git a/map/bookmark.hpp b/map/bookmark.hpp index af162f7060..bdeb4de83b 100644 --- a/map/bookmark.hpp +++ b/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 BookmarkAndCategory; +/// +typedef pair BookmarkAndCategory; inline BookmarkAndCategory MakeEmptyBookmarkAndCategory() { - return BookmarkAndCategory(reinterpret_cast(0), - reinterpret_cast(0)); + return BookmarkAndCategory(string(), int(-1)); +} + +inline bool IsValid(BookmarkAndCategory const & bmc) +{ + return (!bmc.first.empty() && bmc.second >= 0); } diff --git a/map/framework.cpp b/map/framework.cpp index ca2242552b..c2f29b3eff 100644 --- a/map/framework.cpp +++ b/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::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(j); + retBookmarkCategory = m_bookmarks[i]->GetName(); minD = d; } } diff --git a/map/map_tests/bookmarks_test.cpp b/map/map_tests/bookmarks_test.cpp index 16d2e860a4..c93ffafcd3 100644 --- a/map/map_tests/bookmarks_test.cpp +++ b/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)