From 994873c081cf1a0e37f19d3544c2487f2ee13ab8 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Wed, 4 Feb 2015 11:24:30 +0300 Subject: [PATCH] Fixed bookmarks order: least recently added bookmark is displayed first in the list. --- map/bookmark.cpp | 28 ++++++++++++++++--- map/bookmark.hpp | 6 +++- map/bookmark_manager.cpp | 4 +-- map/map_tests/bookmarks_test.cpp | 32 ++++++++++++---------- map/map_tests/kmz_unarchive_test.cpp | 4 +-- map/map_tests/mwm_url_tests.cpp | 15 +++++----- map/user_mark_container.cpp | 41 +++++++++++++--------------- map/user_mark_container.hpp | 10 ++++++- 8 files changed, 86 insertions(+), 54 deletions(-) diff --git a/map/bookmark.cpp b/map/bookmark.cpp index f42292aff2..edda1f5cad 100644 --- a/map/bookmark.cpp +++ b/map/bookmark.cpp @@ -63,10 +63,11 @@ Track const * BookmarkCategory::GetTrack(size_t index) const return (index < m_tracks.size() ? m_tracks[index] : 0); } -void BookmarkCategory::AddBookmark(m2::PointD const & ptOrg, BookmarkData const & bm) +Bookmark * BookmarkCategory::AddBookmark(m2::PointD const & ptOrg, BookmarkData const & bm) { - UserMark * mark = base_t::GetController().CreateUserMark(ptOrg); - static_cast(mark)->SetData(bm); + Bookmark * bookmark = static_cast(base_t::GetController().CreateUserMark(ptOrg)); + bookmark->SetData(bm); + return bookmark; } void BookmarkCategory::ReplaceBookmark(size_t index, BookmarkData const & bm) @@ -168,6 +169,11 @@ Bookmark * BookmarkCategory::GetBookmark(size_t index) return static_cast(index < c.GetUserMarkCount() ? c.GetUserMarkForEdit(index) : 0); } +size_t BookmarkCategory::FindBookmark(Bookmark const * bookmark) const +{ + return base_t::GetController().FindUserMark(bookmark); +} + namespace { @@ -703,7 +709,21 @@ void BookmarkCategory::SaveToKML(ostream & s) s << " " << (IsVisible() ? "1" : "0") <<"\n"; - for (size_t i = 0; i < GetBookmarksCount(); ++i) + // Bookmarks are stored to KML file in reverse order, so, least + // recently added bookmark will be stored last. The reason is that + // when bookmarks will be loaded from the KML file, most recently + // added bookmark will be loaded last and in accordance with current + // logic will added to the beginning of the bookmarks list. Thus, + // this method preserves LRU bookmarks order after store -> load + // actions. + // + // Loop invariant: on each iteration count means number of already + // stored bookmarks and i means index of the bookmark that should be + // processed during the iteration. That's why i is initially set to + // GetBookmarksCount() - 1, i.e. to the last bookmark in the + // bookmarks list. + for (size_t count = 0, i = GetBookmarksCount() - 1; + count < GetBookmarksCount(); ++count, --i) { Bookmark const * bm = GetBookmark(i); s << " \n"; diff --git a/map/bookmark.hpp b/map/bookmark.hpp index 4a3ada3bf9..fde150be50 100644 --- a/map/bookmark.hpp +++ b/map/bookmark.hpp @@ -142,7 +142,7 @@ public: /// @name Theese functions are called from Framework only. //@{ - void AddBookmark(m2::PointD const & ptOrg, BookmarkData const & bm); + Bookmark* AddBookmark(m2::PointD const & ptOrg, BookmarkData const & bm); void ReplaceBookmark(size_t index, BookmarkData const & bm); //@} @@ -165,6 +165,10 @@ public: Bookmark * GetBookmark(size_t index); void DeleteBookmark(size_t index); + // Returns index of the bookmark if exists, otherwise returns + // total number of bookmarks. + size_t FindBookmark(Bookmark const * bookmark) const; + /// @name Theese fuctions are public for unit tests only. /// You don't need to call them from client code. //@{ diff --git a/map/bookmark_manager.cpp b/map/bookmark_manager.cpp index e193c02616..9c211bfeb1 100644 --- a/map/bookmark_manager.cpp +++ b/map/bookmark_manager.cpp @@ -184,7 +184,7 @@ size_t BookmarkManager::AddBookmark(size_t categoryIndex, const m2::PointD & ptO bm.SetScale(m_framework.GetDrawScale()); BookmarkCategory * pCat = m_categories[categoryIndex]; - pCat->AddBookmark(ptOrg, bm); + Bookmark * bookmark = pCat->AddBookmark(ptOrg, bm); pCat->SetVisible(true); pCat->SaveToKMLFile(); @@ -192,7 +192,7 @@ size_t BookmarkManager::AddBookmark(size_t categoryIndex, const m2::PointD & ptO m_lastType = bm.GetType(); SaveState(); - return (pCat->GetBookmarksCount() - 1); + return pCat->FindBookmark(bookmark); } size_t BookmarkManager::MoveBookmark(size_t bmIndex, size_t curCatIndex, size_t newCatIndex) diff --git a/map/map_tests/bookmarks_test.cpp b/map/map_tests/bookmarks_test.cpp index a179ae4e2a..6b0b75f8e0 100644 --- a/map/map_tests/bookmarks_test.cpp +++ b/map/map_tests/bookmarks_test.cpp @@ -120,19 +120,19 @@ char const * kmlString = { TEST_EQUAL(cat.GetBookmarksCount(), 4, ()); - Bookmark const * bm = cat.GetBookmark(0); + Bookmark const * bm = cat.GetBookmark(3); TEST_EQUAL(bm->GetName(), "Nebraska", ()); TEST_EQUAL(bm->GetType(), "placemark-red", ()); TEST_EQUAL(bm->GetDescription(), "", ()); TEST_EQUAL(bm->GetTimeStamp(), my::INVALID_TIME_STAMP, ()); - bm = cat.GetBookmark(1); + bm = cat.GetBookmark(2); TEST_EQUAL(bm->GetName(), "Monongahela National Forest", ()); TEST_EQUAL(bm->GetType(), "placemark-pink", ()); TEST_EQUAL(bm->GetDescription(), "Huttonsville, WV 26273
", ()); TEST_EQUAL(bm->GetTimeStamp(), 524214643, ()); - bm = cat.GetBookmark(2); + bm = cat.GetBookmark(1); m2::PointD org = bm->GetOrg(); TEST_ALMOST_EQUAL(MercatorBounds::XToLon(org.x), 27.566765, ()); TEST_ALMOST_EQUAL(MercatorBounds::YToLat(org.y), 53.900047, ()); @@ -141,7 +141,7 @@ char const * kmlString = TEST_EQUAL(bm->GetDescription(), "", ()); TEST_EQUAL(bm->GetTimeStamp(), 888888888, ()); - bm = cat.GetBookmark(3); + bm = cat.GetBookmark(0); org = bm->GetOrg(); TEST_ALMOST_EQUAL(MercatorBounds::XToLon(org.x), 27.551532, ()); TEST_ALMOST_EQUAL(MercatorBounds::YToLat(org.y), 53.89306, ()); @@ -286,11 +286,11 @@ UNIT_TEST(Bookmarks_Timestamp) fm.AddCategory(arrCat[1]); fm.AddBookmark(1, orgPoint, b3); - TEST_EQUAL(fm.GetBmCategory(0)->GetBookmark(0)->GetName(), "name", ()); - TEST_EQUAL(fm.GetBmCategory(0)->GetBookmark(0)->GetType(), "type", ()); + TEST_EQUAL(fm.GetBmCategory(0)->GetBookmark(1)->GetName(), "name", ()); + TEST_EQUAL(fm.GetBmCategory(0)->GetBookmark(1)->GetType(), "type", ()); - TEST_EQUAL(fm.GetBmCategory(0)->GetBookmark(1)->GetName(), "newName", ()); - TEST_EQUAL(fm.GetBmCategory(0)->GetBookmark(1)->GetType(), "newType", ()); + TEST_EQUAL(fm.GetBmCategory(0)->GetBookmark(0)->GetName(), "newName", ()); + TEST_EQUAL(fm.GetBmCategory(0)->GetBookmark(0)->GetType(), "newType", ()); TEST_EQUAL(fm.GetBmCategory(1)->GetBookmark(0)->GetName(), "newName", ()); TEST_EQUAL(fm.GetBmCategory(1)->GetBookmark(0)->GetType(), "newType", ()); @@ -356,9 +356,11 @@ UNIT_TEST(Bookmarks_Getting) mark = GetBookmark(fm, m2::PointD(41, 40)); cat = GetCategory(mark); - //should find first valid result, there two results with the same coordinates 3 and 4 - TEST_EQUAL(mark->GetName(), "3", ()); - TEST_EQUAL(mark->GetType(), "placemark-red", ()); + + // Should find last added valid result, there two results with the + // same coordinates 3 and 4, but 4 was added later. + TEST_EQUAL(mark->GetName(), "4", ()); + TEST_EQUAL(mark->GetType(), "placemark-blue", ()); TEST_EQUAL(cat->GetBookmarksCount(), 2, ()); @@ -504,8 +506,8 @@ UNIT_TEST(Bookmarks_AddingMoving) mark = GetBookmarkPxPoint(fm, pixelPoint); cat = GetCategory(mark); TEST_EQUAL(cat->GetName(), arrCat[0], ()); - TEST_EQUAL(mark->GetName(), "name", ()); - TEST_EQUAL(mark->GetType(), "placemark-red", ()); + TEST_EQUAL(mark->GetName(), "name2", ()); + TEST_EQUAL(mark->GetType(), "placemark-blue", ()); // Edit name, type and category of bookmark bm = BookmarkData("name3", "placemark-green"); @@ -518,8 +520,8 @@ UNIT_TEST(Bookmarks_AddingMoving) TEST_EQUAL(cat->GetName(), arrCat[0], ()); TEST_EQUAL(fm.GetBmCategory(0)->GetBookmarksCount(), 2, ("Bookmark wasn't moved from one category to another")); - TEST_EQUAL(mark->GetName(), "name", ()); - TEST_EQUAL(mark->GetType(), "placemark-red", ()); + TEST_EQUAL(mark->GetName(), "name2", ()); + TEST_EQUAL(mark->GetType(), "placemark-blue", ()); DeleteCategoryFiles(arrCat); } diff --git a/map/map_tests/kmz_unarchive_test.cpp b/map/map_tests/kmz_unarchive_test.cpp index f3c5a0d408..879c7fb3f6 100644 --- a/map/map_tests/kmz_unarchive_test.cpp +++ b/map/map_tests/kmz_unarchive_test.cpp @@ -40,7 +40,7 @@ UNIT_TEST(Open_KMZ_Test) TEST_EQUAL(cat.GetBookmarksCount(), 6, ("Category wrong number of bookmarks")); { - Bookmark const * bm = cat.GetBookmark(0); + Bookmark const * bm = cat.GetBookmark(5); TEST_EQUAL(bm->GetName(), ("Lahaina Breakwall"), ("KML wrong name!")); TEST_EQUAL(bm->GetType(), "placemark-red", ("KML wrong type!")); TEST_ALMOST_EQUAL(bm->GetOrg().x, -156.6777046791284, ("KML wrong org x!")); @@ -48,7 +48,7 @@ UNIT_TEST(Open_KMZ_Test) TEST_EQUAL(bm->GetScale(), -1, ("KML wrong scale!")); } { - Bookmark const * bm = cat.GetBookmark(1); + Bookmark const * bm = cat.GetBookmark(4); TEST_EQUAL(bm->GetName(), ("Seven Sacred Pools, Kipahulu"), ("KML wrong name!")); TEST_EQUAL(bm->GetType(), "placemark-red", ("KML wrong type!")); TEST_ALMOST_EQUAL(bm->GetOrg().x, -156.0405130750025, ("KML wrong org x!")); diff --git a/map/map_tests/mwm_url_tests.cpp b/map/map_tests/mwm_url_tests.cpp index 260552328d..162ecd40a0 100644 --- a/map/map_tests/mwm_url_tests.cpp +++ b/map/map_tests/mwm_url_tests.cpp @@ -142,12 +142,12 @@ UNIT_TEST(MapApiMultiplePoints) ApiTest api("mwm://map?ll=1.1,1.2&n=A&LL=2.1,2.2&ll=-3.1,-3.2&n=C"); TEST(api.IsValid(), ()); TEST_EQUAL(api.GetPointCount(), 3, ()); - TEST(api.TestLatLon(0, 1.1, 1.2), ()); - TEST(api.TestName(0, "A"), ()); + TEST(api.TestLatLon(2, 1.1, 1.2), ()); + TEST(api.TestName(2, "A"), ()); TEST(api.TestLatLon(1, 2.1, 2.2), ()); TEST(api.TestName(1, ""), ()); - TEST(api.TestLatLon(2, -3.1, -3.2), ()); - TEST(api.TestName(2, "C"), ()); + TEST(api.TestLatLon(0, -3.1, -3.2), ()); + TEST(api.TestName(0, "C"), ()); } UNIT_TEST(MapApiInvalidPointLatLonButValidOtherParts) @@ -298,9 +298,10 @@ void generateRandomTest(size_t numberOfPoints, size_t stringLength) double lat = vect[i].m_lat; double lon = vect[i].m_lon; ToMercatoToLatLon(lat, lon); - TEST(api.TestLatLon(i, lat, lon), ()); - TEST(api.TestName(i, vect[i].m_name), ()); - TEST(api.TestID(i, vect[i].m_id), ()); + size_t const ix = vect.size() - i - 1; + TEST(api.TestLatLon(ix, lat, lon), ()); + TEST(api.TestName(ix, vect[i].m_name), ()); + TEST(api.TestID(ix, vect[i].m_id), ()); } TEST_EQUAL(api.GetApiVersion(), 1, ()); } diff --git a/map/user_mark_container.cpp b/map/user_mark_container.cpp index 6ed109aba5..9a8c665c22 100644 --- a/map/user_mark_container.cpp +++ b/map/user_mark_container.cpp @@ -202,8 +202,9 @@ MyPositionMarkPoint * UserMarkContainer::UserMarkForMyPostion() UserMark * UserMarkContainer::CreateUserMark(m2::PointD const & ptOrg) { - m_userMarks.push_back(AllocateUserMark(ptOrg)); - return m_userMarks.back(); + unique_ptr mark(AllocateUserMark(ptOrg)); + m_userMarks.push_front(mark.get()); + return mark.release(); } size_t UserMarkContainer::GetUserMarkCount() const @@ -223,37 +224,33 @@ UserMark * UserMarkContainer::GetUserMark(size_t index) return m_userMarks[index]; } -namespace -{ - -template void DeleteItem(vector & v, size_t i) -{ - if (i < v.size()) - { - delete v[i]; - v.erase(v.begin() + i); - } - else - { - LOG(LWARNING, ("Trying to delete non-existing item at index", i)); - } -} - -} - void UserMarkContainer::DeleteUserMark(size_t index) { ASSERT_LESS(index, m_userMarks.size(), ()); - DeleteItem(m_userMarks, index); + if (index < m_userMarks.size()) + { + delete m_userMarks[index]; + m_userMarks.erase(m_userMarks.begin() + index); + } + else + { + LOG(LWARNING, ("Trying to delete non-existing item at index", index)); + } } void UserMarkContainer::DeleteUserMark(UserMark const * mark) { - vector::iterator it = find(m_userMarks.begin(), m_userMarks.end(), mark); + UserMarksListT::iterator it = find(m_userMarks.begin(), m_userMarks.end(), mark); if (it != m_userMarks.end()) DeleteUserMark(distance(m_userMarks.begin(), it)); } +size_t UserMarkContainer::FindUserMark(UserMark const * mark) +{ + return distance(m_userMarks.begin(), + find(m_userMarks.begin(), m_userMarks.end(), mark)); +} + SearchUserMarkContainer::SearchUserMarkContainer(double layerDepth, Framework & framework) : UserMarkContainer(layerDepth, framework) { diff --git a/map/user_mark_container.hpp b/map/user_mark_container.hpp index becfc3cd38..b09a7b3b06 100644 --- a/map/user_mark_container.hpp +++ b/map/user_mark_container.hpp @@ -7,6 +7,7 @@ #include "../geometry/point2d.hpp" #include "../geometry/rect2d.hpp" +#include "../std/deque.hpp" #include "../std/noncopyable.hpp" class Framework; @@ -25,6 +26,8 @@ namespace graphics class UserMarkContainer : private noncopyable { public: + using UserMarksListT = deque; + class Controller { public: @@ -38,6 +41,10 @@ public: void DeleteUserMark(size_t index) { m_container->DeleteUserMark(index); } void DeleteUserMark(UserMark const * mark) { m_container->DeleteUserMark(mark); } + // Returns index of the mark if exists, otherwise returns + // number of user marks. + size_t FindUserMark(UserMark const * mark) const { return m_container->FindUserMark(mark); } + private: UserMarkContainer * m_container; }; @@ -95,6 +102,7 @@ private: UserMark * GetUserMark(size_t index); void DeleteUserMark(size_t index); void DeleteUserMark(UserMark const * mark); + size_t FindUserMark(UserMark const * mark); template void ForEachInRect(m2::RectD const & rect, ToDo toDo) const; @@ -106,7 +114,7 @@ private: bool m_isVisible; bool m_isDrawable; double m_layerDepth; - vector m_userMarks; + UserMarksListT m_userMarks; }; class SearchUserMarkContainer : public UserMarkContainer