Editor refactoring #9242

map-per · 2024-09-07T19:23:02Z

map-per commented

2024-09-07 19:23:02 +00:00

fixes #9129, fixes #8102, fixes #2298, fixes #6260, fixes #6292, fixes #9126, fixes #6939, fixes #9508, fixes #9311

This PR supersedes #9138 and adds a Journal to explicitly store which changes were submitted by the user. See #9129 for why his information is needed when uploading to OSM.

Approach

Edits are logged in the Journal. A log contains info like e.g. Tag "website" changed from "www.organicmaps.app" to "organicmaps.app" or ObjectCreated, Type amenity-bench, Location .... When uploading, only the Journal entries are used do update the OSM object. After a successful upload, the Journal is cleared.

Saving and restoring the Journal after an app restart is implemented now.

For the transition phase to the new editing logic the old uploading code is still functional. The old code is used when the object was modified in a previous OM version.

Todo

Logging
- Add logging for all Tags
- Add logging for FeatureType
- Safe and Restore Data after App restart
New uploading logic
Transition from old OM versions

Bugs fixed

Testing

Tested with the OSM dev API, but only on Android.

Transition

The PR only affects new edits. Old edits from previous OM versions that were not uploaded yet use the old uploading code.

New features I keep in mind to not block anything

Adding "komplex" Category types (#4523)
Mark POI as disused
POI category change (#3491)
Add address POIs (#9199)
Support editing POIs mapped as areas (#6727)

Follow up issues (Issues not addressed in this PR)

#9043
#9503
Remove code of the old editor after some time (e.g. three months)

fixes #9129, fixes #8102, fixes #2298, fixes #6260, fixes #6292, fixes #9126, fixes #6939, fixes #9508, fixes #9311 This PR supersedes #9138 and adds a Journal to explicitly store which changes were submitted by the user. See #9129 for why his information is needed when uploading to OSM. #### Approach Edits are logged in the Journal. A log contains info like e.g. `Tag "website" changed from "www.organicmaps.app" to "organicmaps.app"` or `ObjectCreated, Type amenity-bench, Location ...`. When uploading, only the Journal entries are used do update the OSM object. After a successful upload, the Journal is cleared. Saving and restoring the Journal after an app restart is implemented now. For the transition phase to the new editing logic the old uploading code is still functional. The old code is used when the object was modified in a previous OM version. #### Todo - [x] Logging - [x] Add logging for all Tags - [x] Add logging for FeatureType - [x] Safe and Restore Data after App restart - [x] New uploading logic - [x] Transition from old OM versions #### Bugs fixed - #8102 - #2298 - #6260 - #6292 - #9126 #### Testing Tested with the OSM dev API, but only on Android. #### Transition The PR only affects new edits. Old edits from previous OM versions that were not uploaded yet use the old uploading code. #### New features I keep in mind to not block anything - Adding "komplex" Category types (#4523) - Mark POI as disused - POI category change (#3491) - Add address POIs (#9199) - Support editing POIs mapped as areas (#6727) #### Follow up issues (Issues not addressed in this PR) - #9043 - #9503 - Remove code of the old editor after some time (e.g. three months)

❤️ 3 🎉 5 🚀 1

RedAuburn reviewed 2024-09-07 19:23:02 +00:00

biodranik (Migrated from github.com) reviewed 2024-09-07 19:23:02 +00:00

vng (Migrated from github.com) reviewed 2024-12-01 19:34:31 +00:00

vng (Migrated from github.com) left a comment

Will finish review later.

Please:

setup your editor to use open { braces from the new line (not KR-style)
don't use braces for one-line expressions

Will finish review later. Please: - setup your editor to use open { braces from the new line (not KR-style) - don't use braces for one-line expressions

android/app/src/main/cpp/app/organicmaps/editor/Editor.cpp

					
				@ -151,2 +151,3 @@

				{

				  g_editableMapObject.SetInternet(hasWifi ? feature::Internet::Wlan : feature::Internet::Unknown);

				  if (hasWifi != (g_editableMapObject.GetInternet() == feature::Internet::Wlan))

				    g_editableMapObject.SetInternet(hasWifi ? feature::Internet::Wlan : feature::Internet::Unknown);

vng (Migrated from github.com) commented

2024-12-01 18:35:19 +00:00

Incapsulate this logic in EditableMapObject::SetInternet ?

coding/string_utf8_multilang.cpp

vng (Migrated from github.com) commented

2024-12-01 18:36:38 +00:00

name.starts_with("name:")

vng (Migrated from github.com) commented

2024-12-01 18:37:55 +00:00

Put the ASSERT here? Is it a possible case?

Put the ASSERT here? Is it a _possible_ case?

editor/osm_editor.cpp

vng (Migrated from github.com) commented

2024-12-01 18:48:23 +00:00

Is this block the same as some lines above? ("Add tags to XMLFeature"). Take out a function?

editor/osm_editor.cpp

					
				@ -717,0 +808,4 @@

				                XMLFeature const osmFeatureCopy = osmFeature;

				                osmFeature.ApplyPatch(feature);

				                // Check to avoid uploading duplicates into OSM.

				                if (osmFeature == osmFeatureCopy)

vng (Migrated from github.com) commented

2024-12-01 18:51:38 +00:00

Should support this legacy in case when we have the old edits.xml after updating the app?

editor/osm_editor.cpp

					
				@ -813,3 +944,3 @@

				  if (status == FeatureStatus::Created)

				  {

				    editor::FromXML(xml, fti.m_object);

				    if (loadFromJournal)

vng (Migrated from github.com) commented

2024-12-01 18:56:55 +00:00

No need braces here.

editor/osm_editor.cpp

vng (Migrated from github.com) commented

2024-12-01 19:00:18 +00:00

std::move(journal)

editor/xml_feature.cpp

vng (Migrated from github.com) commented

2024-12-01 19:08:52 +00:00

Call time(nullptr) once here?

editor/xml_feature.cpp

					
				@ -412,0 +495,4 @@

				          entry.data = legacyObjData;

				          break;

				        }

				      }

vng (Migrated from github.com) commented

2024-12-01 19:08:14 +00:00

What will happen if xml file is broken, say any XXX.value() does not exist? Should we have any generic error processing here? Like catch an exception and continue/skip the whole entry?

editor/xml_feature.cpp

					
				@ -412,0 +574,4 @@

				  auto xmlJournalHistory = GetRootNode().append_child("journalHistory");

				  insertEditJournalList(xmlJournalHistory, journal.GetJournalHistory());

vng (Migrated from github.com) commented

2024-12-01 19:11:22 +00:00

value.empty()

editor/xml_feature.cpp

vng (Migrated from github.com) commented

2024-12-01 19:14:27 +00:00

GetReadableObjectName(type)

```GetReadableObjectName(type)```

editor/xml_feature.cpp

vng (Migrated from github.com) commented

2024-12-01 19:16:32 +00:00

I suspect that we have more 3-arity types besides amenity-recycling-xxx
Add ASSERT/CHECK(!(++iter), ("Add an exception:", strType))

I suspect that we have more 3-arity types besides amenity-recycling-xxx Add ASSERT/CHECK(!(++iter), ("Add an exception:", strType))

indexer/edit_journal.cpp

vng (Migrated from github.com) commented

2024-12-01 19:20:12 +00:00

Braces are not needed here

vng (Migrated from github.com) commented

2024-12-01 19:24:11 +00:00

Here is how I'd prefer to see this code:

LOG(LDEBUG, ("Key ", key, "changed from \"", old_value, "\" to \"", new_value, "\""));
AddJournalEntry({JournalEntryType::TagModification, time(nullptr), {key, std::move(old_value), std::move(new_value)}});

Here is how I'd prefer to see this code: ``` LOG(LDEBUG, ("Key ", key, "changed from \"", old_value, "\" to \"", new_value, "\"")); AddJournalEntry({JournalEntryType::TagModification, time(nullptr), {key, std::move(old_value), std::move(new_value)}}); ```

vng (Migrated from github.com) commented

2024-12-01 19:24:46 +00:00

JournalEntry entry

vng (Migrated from github.com) commented

2024-12-01 19:25:00 +00:00

std::move(entry)

vng (Migrated from github.com) commented

2024-12-01 19:25:35 +00:00

for (JournalEntry & entry : journal)
  journalHistory.push_back(std::move(entry));

``` for (JournalEntry & entry : journal) journalHistory.push_back(std::move(entry)); ```

vng (Migrated from github.com) commented

2024-12-01 19:25:58 +00:00

JournalEntry entry

vng (Migrated from github.com) commented

2024-12-01 19:26:07 +00:00

std::move(entry)

indexer/edit_journal.cpp

					
				@ -0,0 +35,4 @@

				  void EditJournal::MarkAsCreated(uint32_t type, feature::GeomType geomType, m2::PointD mercator)

				  {

				    ASSERT(m_journal.empty(), ("Only empty journals can be marked as created"));

				    LOG(LDEBUG, ("Object of type ", classif().GetReadableObjectName(type), " created"));

vng (Migrated from github.com) commented

2024-12-01 19:28:54 +00:00

ASSERT(journal.empty(), ("Only empty journals can be marked as created"));
LOG(LDEBUG, ("Object of type ", classif().GetReadableObjectName(type), " created"));
AddJournalEntry({JournalEntryType::ObjectCreated, time(nullptr), {type, geomType, mercator}});

``` ASSERT(journal.empty(), ("Only empty journals can be marked as created")); LOG(LDEBUG, ("Object of type ", classif().GetReadableObjectName(type), " created")); AddJournalEntry({JournalEntryType::ObjectCreated, time(nullptr), {type, geomType, mercator}}); ```

indexer/edit_journal.cpp

					
				@ -0,0 +109,4 @@

				        return "ObjectCreated";

				      case osm::JournalEntryType::LegacyObject:

				        return "LegacyObject";

				    }

vng (Migrated from github.com) commented

2024-12-01 19:27:16 +00:00

UNREACHABLE();

indexer/edit_journal.hpp

vng (Migrated from github.com) commented

2024-12-01 19:18:47 +00:00

Please, use "m_" notation at least here.

indexer/editable_map_object.cpp

vng (Migrated from github.com) commented

2024-12-01 19:01:01 +00:00

EditJournal && editJournal

vng (Migrated from github.com) commented

2024-12-01 19:01:12 +00:00

std::move(editJournal)

vng (Migrated from github.com) commented

2024-12-01 19:29:55 +00:00

No need braces here

vng (Migrated from github.com) commented

2024-12-01 19:32:28 +00:00

Ok, so check langCode != StringUtf8Multilang:: UnsupportedLanguageCode

Ok, so check ```langCode != StringUtf8Multilang:: UnsupportedLanguageCode```

map-per reviewed 2024-12-04 09:34:43 +00:00

indexer/edit_journal.cpp

					
				@ -0,0 +109,4 @@

				        return "ObjectCreated";

				      case osm::JournalEntryType::LegacyObject:

				        return "LegacyObject";

				    }

map-per commented

2024-12-04 09:34:43 +00:00

Is UNREACHABLE() necessary? All three JournalEntryType enum values are handled here, so the compiler can figure out by itself that the end is unreachable.

map-per reviewed 2024-12-04 09:49:07 +00:00

editor/osm_editor.cpp

					
				@ -717,0 +808,4 @@

				                XMLFeature const osmFeatureCopy = osmFeature;

				                osmFeature.ApplyPatch(feature);

				                // Check to avoid uploading duplicates into OSM.

				                if (osmFeature == osmFeatureCopy)

map-per commented

2024-12-04 09:49:07 +00:00

Yes, the old editor upload code is used for all POIs that were edited before the EditJournal was introduced because these POIs don't have a full journal history of the changes.

My plan is to remove the old editor code about three months after this PR is merged.

Yes, the old editor upload code is used for all POIs that were edited before the EditJournal was introduced because these POIs don't have a full journal history of the changes. My plan is to remove the old editor code about three months after this PR is merged.

map-per reviewed 2024-12-04 11:54:19 +00:00

android/app/src/main/cpp/app/organicmaps/editor/Editor.cpp

					
				@ -151,2 +151,3 @@

				{

				  g_editableMapObject.SetInternet(hasWifi ? feature::Internet::Wlan : feature::Internet::Unknown);

				  if (hasWifi != (g_editableMapObject.GetInternet() == feature::Internet::Wlan))

				    g_editableMapObject.SetInternet(hasWifi ? feature::Internet::Wlan : feature::Internet::Unknown);

map-per commented

2024-12-04 11:54:19 +00:00

That's not a good idea, because iOS uses all Internet values and not just Wifi true/false.
(This is a fix for the Android only bug #9508)

That's not a good idea, because iOS uses all `Internet` values and not just Wifi `true`/`false`. (This is a fix for the Android only bug #9508)

map-per reviewed 2024-12-04 12:46:53 +00:00

editor/xml_feature.cpp

					
				@ -412,0 +495,4 @@

				          entry.data = legacyObjData;

				          break;

				        }

				      }

map-per commented

2024-12-04 12:46:53 +00:00

pugixml shouldn't throw exceptions. It's very fault tolerant, so e.g. calling .attribute("key").value() on an empty xml node doesn't crash, but just returns "".

The current GetEditJournal() assumes that the xml is correct. Should I add checks and use MYTHROW in case read in values are invalid like it is done in other parts of the xml_feature class?

pugixml shouldn't throw exceptions. It's very fault tolerant, so e.g. calling `.attribute("key").value()` on an empty xml node doesn't crash, but just returns "". The current `GetEditJournal()` assumes that the xml is correct. Should I add checks and use MYTHROW in case read in values are invalid like it is done in other parts of the `xml_feature` class?

map-per reviewed 2024-12-04 12:54:16 +00:00

coding/string_utf8_multilang.cpp

map-per commented

2024-12-04 12:54:15 +00:00

Added ASSERT_FAIL for that case

map-per reviewed 2024-12-04 12:54:53 +00:00

editor/osm_editor.cpp

map-per commented

2024-12-04 12:54:53 +00:00

Moved it to a new function

map-per reviewed 2024-12-04 12:57:16 +00:00

editor/xml_feature.cpp

map-per commented

2024-12-04 12:57:16 +00:00

👍 Added an assertion

:+1: Added an assertion

map-per reviewed 2024-12-04 16:17:33 +00:00

editor/xml_feature.cpp

					
				@ -412,0 +495,4 @@

				          entry.data = legacyObjData;

				          break;

				        }

				      }

map-per commented

2024-12-04 16:17:33 +00:00

I added checks to make sure only correct values are read from the xml file. (exceptions are thrown when the xml contains an invalid value)

vng (Migrated from github.com) reviewed 2024-12-05 03:39:14 +00:00

android/app/src/main/cpp/app/organicmaps/editor/Editor.cpp

					
				@ -151,2 +151,3 @@

				{

				  g_editableMapObject.SetInternet(hasWifi ? feature::Internet::Wlan : feature::Internet::Unknown);

				  if (hasWifi != (g_editableMapObject.GetInternet() == feature::Internet::Wlan))

				    g_editableMapObject.SetInternet(hasWifi ? feature::Internet::Wlan : feature::Internet::Unknown);

vng (Migrated from github.com) commented

2024-12-05 03:12:21 +00:00

ok

editor/osm_editor.cpp

vng (Migrated from github.com) commented

2024-12-05 03:14:09 +00:00

Please, all braces { here and below from the new line.

indexer/edit_journal.cpp

vng (Migrated from github.com) commented

2024-12-05 03:23:31 +00:00

std::move(key)

indexer/editable_map_object.cpp

vng (Migrated from github.com) commented

2024-12-05 03:24:44 +00:00

std::move(osmLangName)

vng (Migrated from github.com) commented

2024-12-05 03:26:33 +00:00

std::string_view value = ...
std::string_view old_value = ...

std::string_view value = ... std::string_view old_value = ...

vng (Migrated from github.com) commented

2024-12-05 03:26:57 +00:00

m_journal.AddTagChange(ToString(type), std::string(old_value), std::string(value));

vng (Migrated from github.com) commented

2024-12-05 03:30:31 +00:00

(std::vector<std::string> & cuisinesPtr, std::string_view s)

```(std::vector<std::string> & cuisinesPtr, std::string_view s)```

vng (Migrated from github.com) commented

2024-12-05 03:33:37 +00:00

if (!base::IsExist(old_cuisines, new_cuisine))
{
  cuisinesModified = true;
  break;
}

``` if (!base::IsExist(old_cuisines, new_cuisine)) { cuisinesModified = true; break; } ```

map-per reviewed 2024-12-05 20:46:26 +00:00

indexer/editable_map_object.cpp

map-per commented

2024-12-05 20:46:26 +00:00

Thanks! That makes the code way cleaner than with the goto

Thanks! That makes the code way cleaner than with the `goto`

vng (Migrated from github.com) reviewed 2024-12-22 22:02:56 +00:00

editor/osm_editor.cpp

vng (Migrated from github.com) commented

2024-12-22 22:02:52 +00:00

Take out into a separate function to avoid copy-paste-check ?

map-per reviewed 2024-12-22 22:12:55 +00:00

editor/osm_editor.cpp

map-per commented

2024-12-22 22:12:55 +00:00

It's not really worth it because I want to remove the decision logic for old/new editor in a few months anyway. This code is only needed for the transition period.