Fix Import / Export bugs with Online Routing

Code improvements

OsmAnd/src/net/osmand/plus/onlinerouting/OnlineRoutingHelper.java

@@ -3,31 +3,24 @@ package net.osmand.plus.onlinerouting;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 
-import com.google.gson.Gson;
-import com.google.gson.reflect.TypeToken;
-
 import net.osmand.PlatformUtil;
 import net.osmand.data.LatLon;
 import net.osmand.osm.io.NetworkUtils;
 import net.osmand.plus.OsmandApplication;
 import net.osmand.plus.Version;
-import net.osmand.plus.onlinerouting.engine.EngineType;
 import net.osmand.plus.onlinerouting.engine.OnlineRoutingEngine;
 import net.osmand.plus.settings.backend.OsmandSettings;
 import net.osmand.util.Algorithms;
 
 import org.apache.commons.logging.Log;
-import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
-import java.lang.reflect.Type;
 import java.net.HttpURLConnection;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -36,10 +29,6 @@ public class OnlineRoutingHelper {
 
 	private static final Log LOG = PlatformUtil.getLog(OnlineRoutingHelper.class);
 
-	private static final String ITEMS = "items";
-	private static final String TYPE = "type";
-	private static final String PARAMS = "params";
-
 	private OsmandApplication app;
 	private OsmandSettings settings;
 	private Map<String, OnlineRoutingEngine> cachedEngines;
@@ -56,7 +45,7 @@ public class OnlineRoutingHelper {
 	}
 
 	@NonNull
-	public List<OnlineRoutingEngine> getEnginesExceptMentioned(@Nullable String ... excludeKeys) {
+	public List<OnlineRoutingEngine> getEnginesExceptMentionedKeys(@Nullable String... excludeKeys) {
 		List<OnlineRoutingEngine> engines = getEngines();
 		if (excludeKeys != null) {
 			for (String key : excludeKeys) {
@@ -72,6 +61,16 @@ public class OnlineRoutingHelper {
 		return cachedEngines.get(stringKey);
 	}
 
+	@Nullable
+	public OnlineRoutingEngine getEngineByName(@Nullable String name) {
+		for (OnlineRoutingEngine engine : getEngines()) {
+			if (Algorithms.objectEquals(engine.getName(app), name)) {
+				return engine;
+			}
+		}
+		return null;
+	}
+
 	@NonNull
 	public List<LatLon> calculateRouteOnline(@NonNull OnlineRoutingEngine engine,
 	                                         @NonNull List<LatLon> path) throws IOException, JSONException {
@@ -155,7 +154,7 @@ public class OnlineRoutingHelper {
 		if (!Algorithms.isEmpty(jsonString)) {
 			try {
 				JSONObject json = new JSONObject(jsonString);
-				readFromJson(json, engines);
+				OnlineRoutingUtils.readFromJson(json, engines);
 			} catch (JSONException | IllegalArgumentException e) {
 				LOG.debug("Error when reading engines from JSON ", e);
 			}
@@ -167,7 +166,7 @@ public class OnlineRoutingHelper {
 		if (!Algorithms.isEmpty(cachedEngines)) {
 			try {
 				JSONObject json = new JSONObject();
-				writeToJson(json, getEngines());
+				OnlineRoutingUtils.writeToJson(json, getEngines());
 				settings.ONLINE_ROUTING_ENGINES.set(json.toString());
 			} catch (JSONException e) {
 				LOG.debug("Error when writing engines to JSON ", e);
@@ -176,45 +175,4 @@ public class OnlineRoutingHelper {
 			settings.ONLINE_ROUTING_ENGINES.set(null);
 		}
 	}
-
-	public static void readFromJson(@NonNull JSONObject json,
-	                                @NonNull List<OnlineRoutingEngine> engines) throws JSONException {
-		if (!json.has("items")) {
-			return;
-		}
-		Gson gson = new Gson();
-		Type typeToken = new TypeToken<HashMap<String, String>>() {
-		}.getType();
-		JSONArray itemsJson = json.getJSONArray(ITEMS);
-		for (int i = 0; i < itemsJson.length(); i++) {
-			JSONObject object = itemsJson.getJSONObject(i);
-			if (object.has(TYPE) && object.has(PARAMS)) {
-				EngineType type = EngineType.getTypeByName(object.getString(TYPE));
-				String paramsString = object.getString(PARAMS);
-				HashMap<String, String> params = gson.fromJson(paramsString, typeToken);
-				OnlineRoutingEngine engine = OnlineRoutingFactory.createEngine(type, params);
-				if (!Algorithms.isEmpty(engine.getStringKey())) {
-					engines.add(engine);
-				}
-			}
-		}
-	}
-
-	public static void writeToJson(@NonNull JSONObject json,
-	                               @NonNull List<OnlineRoutingEngine> engines) throws JSONException {
-		JSONArray jsonArray = new JSONArray();
-		Gson gson = new Gson();
-		Type type = new TypeToken<HashMap<String, String>>() {
-		}.getType();
-		for (OnlineRoutingEngine engine : engines) {
-			if (Algorithms.isEmpty(engine.getStringKey())) {
-				continue;
-			}
-			JSONObject jsonObject = new JSONObject();
-			jsonObject.put(TYPE, engine.getType().name());
-			jsonObject.put(PARAMS, gson.toJson(engine.getParams(), type));
-			jsonArray.put(jsonObject);
-		}
-		json.put(ITEMS, jsonArray);
-	}
 }

OsmAnd/src/net/osmand/plus/onlinerouting/OnlineRoutingUtils.java

@@ -0,0 +1,91 @@
+package net.osmand.plus.onlinerouting;
+
+import android.content.Context;
+
+import androidx.annotation.NonNull;
+
+import com.google.gson.Gson;
+import com.google.gson.reflect.TypeToken;
+
+import net.osmand.plus.onlinerouting.engine.EngineType;
+import net.osmand.plus.onlinerouting.engine.OnlineRoutingEngine;
+import net.osmand.util.Algorithms;
+
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+
+import java.lang.reflect.Type;
+import java.util.HashMap;
+import java.util.List;
+
+public class OnlineRoutingUtils {
+
+	private static final String ITEMS = "items";
+	private static final String TYPE = "type";
+	private static final String PARAMS = "params";
+
+	public static void generateUniqueName(@NonNull Context ctx,
+	                                      @NonNull OnlineRoutingEngine engine,
+	                                      @NonNull List<OnlineRoutingEngine> otherEngines) {
+		engine.remove(EngineParameter.NAME_INDEX);
+		if (hasNameDuplicate(ctx, engine.getName(ctx), otherEngines)) {
+			int index = 0;
+			do {
+				engine.put(EngineParameter.NAME_INDEX, String.valueOf(++index));
+			} while (hasNameDuplicate(ctx, engine.getName(ctx), otherEngines));
+		}
+	}
+
+	public static boolean hasNameDuplicate(@NonNull Context ctx,
+	                                       @NonNull String engineName,
+	                                       @NonNull List<OnlineRoutingEngine> otherEngines) {
+		for (OnlineRoutingEngine engine : otherEngines) {
+			if (Algorithms.objectEquals(engine.getName(ctx), engineName)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	public static void readFromJson(@NonNull JSONObject json,
+	                                @NonNull List<OnlineRoutingEngine> engines) throws JSONException {
+		if (!json.has("items")) {
+			return;
+		}
+		Gson gson = new Gson();
+		Type typeToken = new TypeToken<HashMap<String, String>>() {
+		}.getType();
+		JSONArray itemsJson = json.getJSONArray(ITEMS);
+		for (int i = 0; i < itemsJson.length(); i++) {
+			JSONObject object = itemsJson.getJSONObject(i);
+			if (object.has(TYPE) && object.has(PARAMS)) {
+				EngineType type = EngineType.getTypeByName(object.getString(TYPE));
+				String paramsString = object.getString(PARAMS);
+				HashMap<String, String> params = gson.fromJson(paramsString, typeToken);
+				OnlineRoutingEngine engine = OnlineRoutingFactory.createEngine(type, params);
+				if (!Algorithms.isEmpty(engine.getStringKey())) {
+					engines.add(engine);
+				}
+			}
+		}
+	}
+
+	public static void writeToJson(@NonNull JSONObject json,
+	                               @NonNull List<OnlineRoutingEngine> engines) throws JSONException {
+		JSONArray jsonArray = new JSONArray();
+		Gson gson = new Gson();
+		Type type = new TypeToken<HashMap<String, String>>() {
+		}.getType();
+		for (OnlineRoutingEngine engine : engines) {
+			if (Algorithms.isEmpty(engine.getStringKey())) {
+				continue;
+			}
+			JSONObject jsonObject = new JSONObject();
+			jsonObject.put(TYPE, engine.getType().name());
+			jsonObject.put(PARAMS, gson.toJson(engine.getParams(), type));
+			jsonArray.put(jsonObject);
+		}
+		json.put(ITEMS, jsonArray);
+	}
+}

OsmAnd/src/net/osmand/plus/onlinerouting/engine/GraphhopperEngine.java

@@ -64,11 +64,11 @@ public class GraphhopperEngine extends OnlineRoutingEngine {
 					.append('&');
 		}
 		String vehicle = get(EngineParameter.VEHICLE_KEY);
-		if (isEmpty(vehicle)) {
+		if (!isEmpty(vehicle)) {
 			sb.append("vehicle=").append(vehicle);
 		}
 		String apiKey = get(EngineParameter.API_KEY);
-		if (isEmpty(apiKey)) {
+		if (!isEmpty(apiKey)) {
 			sb.append('&').append("key=").append(apiKey);
 		}
 	}

OsmAnd/src/net/osmand/plus/onlinerouting/engine/OnlineRoutingEngine.java

@@ -52,23 +52,16 @@ public abstract class OnlineRoutingEngine implements Cloneable {
 
 	@NonNull
 	public String getName(@NonNull Context ctx) {
-		String customName = get(EngineParameter.CUSTOM_NAME);
+		String name = get(EngineParameter.CUSTOM_NAME);
-		if (customName != null) {
+		if (name == null) {
-			return customName;
+			name = getStandardName(ctx);
-		} else {
-			return getStandardName(ctx);
 		}
-	}
-
-	@NonNull
-	private String getStandardName(@NonNull Context ctx) {
-		String base = getBaseName(ctx);
 		String index = get(EngineParameter.NAME_INDEX);
-		return !isEmpty(index) ? base + " " + index : base;
+		return !isEmpty(index) ? name + " " + index : name;
 	}
 
 	@NonNull
-	public String getBaseName(@NonNull Context ctx) {
+	public String getStandardName(@NonNull Context ctx) {
 		String vehicleTitle = getSelectedVehicleName(ctx);
 		if (isEmpty(vehicleTitle)) {
 			return getType().getTitle();

OsmAnd/src/net/osmand/plus/onlinerouting/ui/OnlineRoutingEngineFragment.java

@@ -40,6 +40,7 @@ import net.osmand.plus.mapcontextmenu.other.HorizontalSelectionAdapter.Horizonta
 import net.osmand.plus.onlinerouting.EngineParameter;
 import net.osmand.plus.onlinerouting.OnlineRoutingFactory;
 import net.osmand.plus.onlinerouting.OnlineRoutingHelper;
+import net.osmand.plus.onlinerouting.OnlineRoutingUtils;
 import net.osmand.plus.onlinerouting.VehicleType;
 import net.osmand.plus.onlinerouting.ui.OnlineRoutingCard.OnTextChangedListener;
 import net.osmand.plus.onlinerouting.engine.EngineType;
@@ -243,6 +244,7 @@ public class OnlineRoutingEngineFragment extends BaseOsmAndFragment {
 			public void onTextChanged(boolean changedByUser, @NonNull String text) {
 				if (changedByUser) {
 					engine.put(EngineParameter.CUSTOM_NAME, text);
+					engine.remove(EngineParameter.NAME_INDEX);
 					checkCustomNameUnique(engine);
 				}
 			}
@@ -448,18 +450,13 @@ public class OnlineRoutingEngineFragment extends BaseOsmAndFragment {
 
 	private void generateUniqueNameIfNeeded() {
 		if (engine.get(EngineParameter.CUSTOM_NAME) == null) {
-			engine.remove(EngineParameter.NAME_INDEX);
+			List<OnlineRoutingEngine> cachedEngines = helper.getEnginesExceptMentionedKeys(editedEngineKey);
-			if (hasNameDuplicate(engine.getName(app))) {
+			OnlineRoutingUtils.generateUniqueName(app, engine, cachedEngines);
-				int index = 0;
-				do {
-					engine.put(EngineParameter.NAME_INDEX, String.valueOf(++index));
-				} while (hasNameDuplicate(engine.getName(app)));
-			}
 		}
 	}
 
 	private void checkCustomNameUnique(@NonNull OnlineRoutingEngine engine) {
-		if (hasNameDuplicate(engine.getName(app))) {
+		if (hasNameDuplicate(engine)) {
 			nameCard.showFieldBoxError(getString(R.string.message_name_is_already_exists));
 			saveButton.setEnabled(false);
 		} else {
@@ -468,13 +465,9 @@ public class OnlineRoutingEngineFragment extends BaseOsmAndFragment {
 		}
 	}
 
-	private boolean hasNameDuplicate(@NonNull String name) {
+	private boolean hasNameDuplicate(@NonNull OnlineRoutingEngine engine) {
-		for (OnlineRoutingEngine engine : helper.getEnginesExceptMentioned(editedEngineKey)) {
+		List<OnlineRoutingEngine> cachedEngines = helper.getEnginesExceptMentionedKeys(editedEngineKey);
-			if (Algorithms.objectEquals(engine.getName(app), name)) {
+		return OnlineRoutingUtils.hasNameDuplicate(app, engine.getName(app), cachedEngines);
-				return true;
-			}
-		}
-		return false;
 	}
 
 	private void onSaveEngine() {

OsmAnd/src/net/osmand/plus/settings/backend/backup/OnlineRoutingSettingsItem.java

@@ -8,9 +8,10 @@ import androidx.annotation.Nullable;
 import net.osmand.plus.OsmandApplication;
 import net.osmand.plus.R;
 import net.osmand.plus.onlinerouting.EngineParameter;
-import net.osmand.plus.onlinerouting.OnlineRoutingFactory;
+import net.osmand.plus.onlinerouting.OnlineRoutingUtils;
 import net.osmand.plus.onlinerouting.engine.OnlineRoutingEngine;
 import net.osmand.plus.onlinerouting.OnlineRoutingHelper;
+import net.osmand.util.Algorithms;
 
 import org.json.JSONException;
 import org.json.JSONObject;
@@ -20,7 +21,8 @@ import java.util.List;
 
 public class OnlineRoutingSettingsItem extends CollectionSettingsItem<OnlineRoutingEngine> {
 
-	private OnlineRoutingHelper onlineRoutingHelper;
+	private OnlineRoutingHelper helper;
+	private List<OnlineRoutingEngine> otherEngines;
 
 	public OnlineRoutingSettingsItem(@NonNull OsmandApplication app, @NonNull List<OnlineRoutingEngine> items) {
 		super(app, null, items);
@@ -37,8 +39,9 @@ public class OnlineRoutingSettingsItem extends CollectionSettingsItem<OnlineRout
 	@Override
 	protected void init() {
 		super.init();
-		onlineRoutingHelper = app.getOnlineRoutingHelper();
+		helper = app.getOnlineRoutingHelper();
-		existingItems = new ArrayList<>(onlineRoutingHelper.getEngines());
+		existingItems = new ArrayList<>(helper.getEngines());
+		otherEngines = new ArrayList<>(existingItems);
 	}
 
 	@NonNull
@@ -67,12 +70,18 @@ public class OnlineRoutingSettingsItem extends CollectionSettingsItem<OnlineRout
 
 			for (OnlineRoutingEngine duplicate : duplicateItems) {
 				if (shouldReplace) {
-					onlineRoutingHelper.deleteEngine(duplicate.getStringKey());
+					OnlineRoutingEngine cachedEngine = helper.getEngineByKey(duplicate.getStringKey());
+					if (cachedEngine == null) {
+						cachedEngine = helper.getEngineByName(duplicate.getName(app));
+					}
+					if (cachedEngine != null) {
+						helper.deleteEngine(cachedEngine);
+					}
 				}
 				appliedItems.add(shouldReplace ? duplicate : renameItem(duplicate));
 			}
 			for (OnlineRoutingEngine engine : appliedItems) {
-				onlineRoutingHelper.saveEngine(engine);
+				helper.saveEngine(engine);
 			}
 		}
 	}
@@ -80,8 +89,8 @@ public class OnlineRoutingSettingsItem extends CollectionSettingsItem<OnlineRout
 	@Override
 	public boolean isDuplicate(@NonNull OnlineRoutingEngine routingEngine) {
 		for (OnlineRoutingEngine engine : existingItems) {
-			if (engine.getStringKey().equals(routingEngine.getStringKey())
+			if (Algorithms.objectEquals(engine.getStringKey(), routingEngine.getStringKey())
-					|| engine.getName(app).equals(routingEngine.getName(app))) {
+					|| Algorithms.objectEquals(engine.getName(app), routingEngine.getName(app))) {
 				return true;
 			}
 		}
@@ -91,21 +100,17 @@ public class OnlineRoutingSettingsItem extends CollectionSettingsItem<OnlineRout
 	@NonNull
 	@Override
 	public OnlineRoutingEngine renameItem(@NonNull OnlineRoutingEngine item) {
-		int number = 0;
+		OnlineRoutingEngine renamedItem = (OnlineRoutingEngine) item.clone();
-		while (true) {
+		OnlineRoutingUtils.generateUniqueName(app, renamedItem, otherEngines);
-			number++;
+		renamedItem.remove(EngineParameter.KEY);
-			OnlineRoutingEngine renamedItem = OnlineRoutingFactory.createEngine(item.getType(), item.getParams());
+		otherEngines.add(renamedItem);
-			renamedItem.put(EngineParameter.CUSTOM_NAME, renamedItem.getName(app) + "_" + number);
+		return renamedItem;
-			if (!isDuplicate(renamedItem)) {
-				return renamedItem;
-			}
-		}
 	}
 
 	@Override
 	void readItemsFromJson(@NonNull JSONObject json) throws IllegalArgumentException {
 		try {
-			OnlineRoutingHelper.readFromJson(json, items);
+			OnlineRoutingUtils.readFromJson(json, items);
 		} catch (JSONException | IllegalArgumentException e) {
 			warnings.add(app.getString(R.string.settings_item_read_error, String.valueOf(getType())));
 			throw new IllegalArgumentException("Json parse error", e);
@@ -116,7 +121,7 @@ public class OnlineRoutingSettingsItem extends CollectionSettingsItem<OnlineRout
 	void writeItemsToJson(@NonNull JSONObject json) {
 		if (!items.isEmpty()) {
 			try {
-				OnlineRoutingHelper.writeToJson(json, items);
+				OnlineRoutingUtils.writeToJson(json, items);
 			} catch (JSONException e) {
 				warnings.add(app.getString(R.string.settings_item_write_error, String.valueOf(getType())));
 				SettingsHelper.LOG.error("Failed write to json", e);