Skip to content

Commit

Permalink
Merge branch 'restore_selection_status_on_undo'
Browse files Browse the repository at this point in the history
  • Loading branch information
simonpoole committed Jan 4, 2025
2 parents 9de0896 + 142525f commit c72d071
Show file tree
Hide file tree
Showing 13 changed files with 216 additions and 114 deletions.
33 changes: 33 additions & 0 deletions src/androidTest/java/de/blau/android/easyedit/NodeTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,39 @@ public void unjoinMergeNodes() {
assertEquals(600181872L, node.getOsmId());
assertEquals(4, delegator.getCurrentStorage().getWays(node).size());
}

/**
* Select, unjoin, then undo, node should be selected again
*/
// @SdkSuppress(minSdkVersion = 26)
@Test
public void unjoinNodesUndo() {
TestUtils.zoomToLevel(device, main, 21);
TestUtils.unlock(device);
TestUtils.clickAtCoordinates(device, map, 8.3866386, 47.3904394, true);
TestUtils.clickAwayTip(device, main);
assertTrue(TestUtils.findText(device, false, context.getString(R.string.actionmode_nodeselect)));
Node node = App.getLogic().getSelectedNode();
assertNotNull(node);
assertEquals(600181872L, node.getOsmId());
final StorageDelegator delegator = App.getDelegator();
assertEquals(4, delegator.getCurrentStorage().getWays(node).size());

int apiNodeCount = delegator.getApiNodeCount();
assertTrue(TestUtils.clickMenuButton(device, context.getString(R.string.menu_unjoin), false, true));
assertEquals(apiNodeCount + 3, delegator.getApiNodeCount());

assertTrue(TestUtils.clickMenuButton(device, context.getString(R.string.undo), false, false));
TestUtils.clickAwayTip(device, context);

assertTrue(TestUtils.findText(device, false, context.getString(R.string.actionmode_nodeselect), 5000));
assertEquals(apiNodeCount, delegator.getApiNodeCount());

node = App.getLogic().getSelectedNode();
assertNotNull(node);
assertEquals(600181872L, node.getOsmId());
assertEquals(4, delegator.getCurrentStorage().getWays(node).size());
}

/**
* Select node that is member of a way, append to it
Expand Down
76 changes: 52 additions & 24 deletions src/main/java/de/blau/android/Logic.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import androidx.appcompat.app.AlertDialog;
import androidx.appcompat.app.AppCompatActivity;
import androidx.fragment.app.FragmentActivity;
import de.blau.android.Selection.Ids;
import de.blau.android.contract.HttpStatusCodes;
import de.blau.android.contract.Urls;
import de.blau.android.dialogs.AttachedObjectWarning;
Expand All @@ -64,6 +65,7 @@
import de.blau.android.dialogs.Progress;
import de.blau.android.dialogs.ProgressDialog;
import de.blau.android.dialogs.UploadConflict;
import de.blau.android.easyedit.EasyEditManager;
import de.blau.android.easyedit.ElementSelectionActionModeCallback;
import de.blau.android.exception.DataConflictException;
import de.blau.android.exception.IllegalOperationException;
Expand Down Expand Up @@ -106,6 +108,7 @@
import de.blau.android.osm.StorageDelegator;
import de.blau.android.osm.Tags;
import de.blau.android.osm.UndoStorage;
import de.blau.android.osm.UndoStorage.Checkpoint;
import de.blau.android.osm.UserDetails;
import de.blau.android.osm.ViewBox;
import de.blau.android.osm.Way;
Expand Down Expand Up @@ -470,41 +473,57 @@ public UndoStorage getUndo() {
}

/**
* Wrapper to ensure the dirty flag is set
* Undo the last checkpoint
*
* @return checkpoint name or null if none available
*/
@Nullable
public String undo() {
final StorageDelegator delegator = getDelegator();
String name = delegator.getUndo().undo();
checkClipboard(delegator);
delegator.dirty();
return name;
return undo(getDelegator(), getDelegator().getUndo().undo());
}

/**
* Check the clipboard for consistency post undo
* Undo a specific checkpoint
*
* @param checkpoint index of the checkpoint to undo
* @return checkpoint name or null if none available
*/
private void checkClipboard(@NonNull StorageDelegator delegator) {
if (!delegator.clipboardIsEmpty()) {
delegator.checkClipboard();
}
@Nullable
public String undo(int checkpoint) {
return undo(getDelegator(), getDelegator().getUndo().undo(checkpoint));
}

/**
* Wrapper to ensure the dirty flag is set
* Undo a checkpoint
*
* @param checkpoint index of the checkpoint to undo
* @param delegator the current StorageDelegator instance
* @param toUndo the Checkpoint to undo
* @return checkpoint name or null if none available
*/
@Nullable
public String undo(int checkpoint) {
final StorageDelegator delegator = getDelegator();
String name = delegator.getUndo().undo(checkpoint);
private String undo(@NonNull final StorageDelegator delegator, @Nullable Checkpoint toUndo) {
Selection.Ids ids = toUndo != null ? toUndo.getSelection() : null;
if (ids != null && map != null && map.getContext() instanceof Main) {
Main main = (Main) map.getContext();
final EasyEditManager easyEditManager = main.getEasyEditManager();
easyEditManager.finish();
final Selection currentSelection = selectionStack.getFirst();
currentSelection.reset();
currentSelection.fromIds(main, delegator, ids);
selectFromTop();
easyEditManager.editElements();
}
checkClipboard(delegator);
delegator.dirty();
return name;
return toUndo != null ? toUndo.getName() : null;
}

/**
* Check the clipboard for consistency post undo
*/
private void checkClipboard(@NonNull StorageDelegator delegator) {
if (!delegator.clipboardIsEmpty()) {
delegator.checkClipboard();
}
}

/**
Expand Down Expand Up @@ -656,7 +675,7 @@ public void createCheckpoint(@Nullable Activity activity, int stringId) {
Resources r = activity != null ? activity.getResources() : App.resources();
final UndoStorage undo = getDelegator().getUndo();
boolean firstCheckpoint = !undo.canUndo();
undo.createCheckpoint(r.getString(stringId));
undo.createCheckpoint(r.getString(stringId), getSelectedIds());
getDelegator().recordImagery(map);
if (firstCheckpoint && activity instanceof AppCompatActivity) {
((AppCompatActivity) activity).invalidateOptionsMenu();
Expand Down Expand Up @@ -2290,7 +2309,7 @@ public synchronized List<Result> performMerge(@Nullable final FragmentActivity a
createCheckpoint(activity, R.string.undo_action_merge_ways);
try {
displayAttachedObjectWarning(activity, mergeInto, mergeFrom, true); // needs to be done before merge
MergeAction action = new MergeAction(getDelegator(), mergeInto, mergeFrom);
MergeAction action = new MergeAction(getDelegator(), mergeInto, mergeFrom, getSelectedIds());
List<Result> result = action.mergeWays();
invalidateMap();
return result;
Expand Down Expand Up @@ -2327,7 +2346,7 @@ public synchronized List<Result> performMerge(@Nullable FragmentActivity activit
result.setElement(previousWay);
for (int i = 1; i < sortedWays.size(); i++) {
Way nextWay = (Way) sortedWays.get(i);
MergeAction action = new MergeAction(getDelegator(), previousWay, nextWay);
MergeAction action = new MergeAction(getDelegator(), previousWay, nextWay, getSelectedIds());
List<Result> tempResult = action.mergeWays();
final Result newMergeResult = tempResult.get(0);
if (!(newMergeResult.getElement() instanceof Way)) {
Expand Down Expand Up @@ -2364,7 +2383,7 @@ public synchronized List<Result> performPolygonMerge(@Nullable FragmentActivity
throw new OsmIllegalOperationException("No mergeable polygons");
}
try {
MergeAction action = new MergeAction(getDelegator(), ways.get(0), ways.get(1));
MergeAction action = new MergeAction(getDelegator(), ways.get(0), ways.get(1), getSelectedIds());
return action.mergeSimplePolygons(map);
} catch (OsmIllegalOperationException | StorageException ex) {
handleDelegatorException(activity, ex);
Expand Down Expand Up @@ -2534,7 +2553,7 @@ public synchronized List<Result> performMergeNodes(@Nullable FragmentActivity ac
throw new OsmIllegalOperationException("Trying to join node to itself");
}
displayAttachedObjectWarning(activity, element, nodeToJoin); // needs to be done before join
MergeAction action = new MergeAction(getDelegator(), element, nodeToJoin);
MergeAction action = new MergeAction(getDelegator(), element, nodeToJoin, getSelectedIds());
try {
List<Result> tempResult = action.mergeNodes();
if (overallResult.isEmpty()) {
Expand Down Expand Up @@ -2617,7 +2636,7 @@ public synchronized List<Result> performJoinNodeToWays(@Nullable FragmentActivit
} else {
displayAttachedObjectWarning(activity, node, nodeToJoin); // needs to be done before join
// merge node into target Node
MergeAction action = new MergeAction(getDelegator(), node, nodeToJoin);
MergeAction action = new MergeAction(getDelegator(), node, nodeToJoin, getSelectedIds());
tempResult = action.mergeNodes();
}
} catch (OsmIllegalOperationException | StorageException ex) {
Expand Down Expand Up @@ -4980,6 +4999,15 @@ public synchronized List<OsmElement> getSelectedElements() {
return result;
}

/**
* Get the ids of currently selected objects
*
* @return an Selection.Ids object containing the ids of currently selected objects
*/
public Ids getSelectedIds() {
return selectionStack.getFirst().getIds();
}

/**
* Check is all selected elements exist, return true if we actually had to remove something
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ public boolean onCreateActionMode(ActionMode mode, Menu menu) {
} else {
mode.setSubtitle(R.string.actionmode_createpath);
}
logic.createCheckpoint(main, R.string.undo_action_append);
snap = logic.getPrefs().isWaySnapEnabled();
logic.setSelectedWay(null);
logic.setSelectedNode(appendTargetNode);
Expand Down Expand Up @@ -373,13 +374,13 @@ private synchronized void pathCreateNode(float x, float y) {
final boolean firstNode = addedNodes.isEmpty();
Node clicked = logic.getClickedNode(x, y);
if (appendTargetNode != null) {
logic.performAppendAppend(main, x, y, firstNode, snap);
logic.performAppendAppend(main, x, y, false, snap);
appendTargetNode = logic.getSelectedNode();
if (firstNode) {
checkpointName = R.string.undo_action_append;
}
} else {
logic.performAdd(main, x, y, firstNode, snap);
logic.performAdd(main, x, y, false, snap);
if (firstNode) {
checkpointName = R.string.undo_action_add;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public boolean handleElementClick(OsmElement element) { // NOSONAR
Node toReplace = findYoungestUntaggedNode((Way) element);
logic.setTags(main, Node.NAME, target.getOsmId(), null, false);
logic.performSetPosition(main, (Node) target, toReplace.getLon(), toReplace.getLat(), false);
MergeAction merge = new MergeAction(delegator, target, toReplace, false);
MergeAction merge = new MergeAction(delegator, target, toReplace, false, logic.getSelectedIds());
List<Result> mergeResult = merge.mergeNodes();
java.util.Map<String, String> mergedTags = MergeAction.mergeTags(element, targetTags);
Result r = mergeResult.get(0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,11 @@ public boolean handleElementClick(OsmElement element) {
try {
List<Result> result = logic.performSplit(main, way, node, fromEnd);
checkSplitResult(way, result);
manager.finish();
logic.setSelectedWay((Way) result.get(0).getElement());
manager.editElements();
} catch (OsmIllegalOperationException | StorageException ex) {
// toast has already been displayed
} finally {
manager.finish();
}
});
Expand All @@ -96,7 +98,7 @@ public void saveState(SerializableState state) {
state.putLong(WAY_ID_KEY, way.getOsmId());
state.putLong(NODE_ID_KEY, node.getOsmId());
}

@Override
public void onDestroyActionMode(ActionMode mode) {
logic.setClickableElements(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,11 @@ public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
Way way = (Way) element;
switch (item.getItemId()) {
case MENUITEM_SPLIT:
deselect = false;
main.startSupportActionMode(new WaySplittingActionModeCallback(manager, way, false));
break;
case MENUITEM_MERGE:
deselect = false;
main.startSupportActionMode(new WayMergingActionModeCallback(manager, way, cachedMergeableWays));
break;
case MENUITEM_REVERSE:
Expand All @@ -247,9 +249,11 @@ public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
findConnectedWays(way);
break;
case MENUITEM_APPEND:
deselect = false;
main.startSupportActionMode(new WayAppendingActionModeCallback(manager, way, cachedAppendableNodes));
break;
case MENUITEM_RESTRICTION:
deselect = false;
main.startSupportActionMode(new FromElementActionModeCallback(manager, way, cachedViaElements));
break;
case MENUITEM_ROUTE:
Expand Down Expand Up @@ -282,6 +286,7 @@ public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
main.performTagEdit(element, null, true, false);
break;
case MENUITEM_REMOVE_NODE:
deselect = false;
main.startSupportActionMode(new RemoveNodeFromWayActionModeCallback(manager, way));
break;
case MENUITEM_UNJOIN:
Expand All @@ -292,6 +297,7 @@ public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
Util.sharePosition(main, Geometry.centroidLonLat(way), main.getMap().getZoomLevel());
break;
case MENUITEM_EXTRACT_SEGMENT:
deselect = false;
main.startSupportActionMode(new WaySegmentActionModeCallback(manager, way));
break;
case MENUITEM_SELECT_WAY_NODES:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ public boolean onCreateActionMode(ActionMode mode, Menu menu) {
}

@Override
public boolean handleElementClick(OsmElement element) { // due to clickableElements, only valid nodes can be
// clicked
public boolean handleElementClick(OsmElement element) { // due to clickableElements, only valid nodes can be clicked
super.handleElementClick(element);
// protect against race conditions
if (!(element instanceof Node)) {
Expand All @@ -102,9 +101,11 @@ public boolean handleElementClick(OsmElement element) { // due to clickableEleme
try {
List<Result> result = logic.performSplit(main, way, (Node) element, true);
checkSplitResult(way, result);
manager.finish();
logic.setSelectedWay((Way) result.get(0).getElement());
manager.editElements();
} catch (OsmIllegalOperationException | StorageException ex) {
// toast has already been displayed
} finally {
manager.finish();
}
});
Expand Down
16 changes: 12 additions & 4 deletions src/main/java/de/blau/android/osm/MergeAction.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import de.blau.android.R;
import de.blau.android.Selection;
import de.blau.android.Selection.Ids;
import de.blau.android.exception.OsmIllegalOperationException;
import de.blau.android.util.ACRAHelper;
import de.blau.android.util.Coordinates;
Expand All @@ -40,6 +42,8 @@ public class MergeAction {
private final OsmElement mergeFrom;
private final List<Result> overallResult;

private final Selection.Ids selection;

/**
* Initialize a new MergeAction
*
Expand All @@ -48,9 +52,10 @@ public class MergeAction {
* @param delegator the StorageDelegator to use
* @param mergeInto the OsmElement that we are going to merge into
* @param mergeFrom the OsmElement that is going to be removed
* @param selection the current selection in the UI
*/
public MergeAction(final @NonNull StorageDelegator delegator, @NonNull OsmElement mergeInto, @NonNull OsmElement mergeFrom) {
this(delegator, mergeInto, mergeFrom, true);
public MergeAction(final @NonNull StorageDelegator delegator, @NonNull OsmElement mergeInto, @NonNull OsmElement mergeFrom, Ids selection) {
this(delegator, mergeInto, mergeFrom, true, selection);
}

/**
Expand All @@ -62,8 +67,10 @@ public MergeAction(final @NonNull StorageDelegator delegator, @NonNull OsmElemen
* @param mergeInto the OsmElement that we are going to merge into
* @param mergeFrom the OsmElement that is going to be removed
* @param swappable if true the elements can be swapped to maintain history
* @param selection the current selection in the UI
*/
public MergeAction(final @NonNull StorageDelegator delegator, @NonNull OsmElement mergeInto, @NonNull OsmElement mergeFrom, boolean swappable) {
public MergeAction(final @NonNull StorageDelegator delegator, @NonNull OsmElement mergeInto, @NonNull OsmElement mergeFrom, boolean swappable,
Ids selection) {
this.delegator = delegator;
// first determine if one of the elements already has a valid id, if it is not and other node has valid id swap
// else check version numbers, then choose the older element. The point of this is to preserve as much history
Expand All @@ -79,6 +86,7 @@ public MergeAction(final @NonNull StorageDelegator delegator, @NonNull OsmElemen
}
this.mergeInto = mergeInto;
this.mergeFrom = mergeFrom;
this.selection = selection;
overallResult = roleConflict(mergeInto, mergeFrom);
}

Expand Down Expand Up @@ -540,7 +548,7 @@ public List<Result> mergeSimplePolygons(@NonNull de.blau.android.Map map) throws
delegator.setTags(result, tags);
try {
delegator.lock();
delegator.getUndo().createCheckpoint(map.getContext().getString(R.string.undo_action_move_tags));
delegator.getUndo().createCheckpoint(map.getContext().getString(R.string.undo_action_move_tags), selection);
delegator.recordImagery(map);
} finally {
delegator.unlock();
Expand Down
Loading

0 comments on commit c72d071

Please sign in to comment.