Modify

Opened 7 months ago

Closed 6 months ago

#19121 closed enhancement (fixed)

[Patch] JosmAction.checkAndConfirmOutlyingOperation() should also handle dataset without download area

Reported by: GerdP Owned by: team
Priority: normal Milestone: 20.05
Component: Core Version:
Keywords: Cc: Skyper

Description (last modified by GerdP)

We have some actions (unglue, delete, join area) which call this method to ask for user confirmation with texts like

"You are about to delete nodes outside of the area you have downloaded."

This message doesn't cover the case that the user didn't donwload any area when e.g. an overpass query was used or maybe download object.
I think the text should be changed to

"You are about to delete nodes not inside any area you have downloaded."

and the test should be changed to detect the corresponding case.

Attachments (1)

19121.patch (4.5 KB) - added by GerdP 7 months ago.

Download all attachments as: .zip

Change History (18)

Changed 7 months ago by GerdP

Attachment: 19121.patch added

comment:1 Changed 7 months ago by GerdP

Cc: Skyper added; skyper removed
Description: modified (diff)

The dialog should probably be used by a lot more actions. In a later step I want to replace it by a dialog which asks the user if downloading more data is OK.

If you don't like the new text please suggest an alternative.

comment:2 Changed 7 months ago by skyper

How about, You are about to delete nodes outside of an area you have completely downloaded. ?

comment:3 in reply to:  2 Changed 7 months ago by GerdP

Replying to skyper:

How about, You are about to delete nodes outside of an area you have completely downloaded. ?

I think this still implies that there is an area which was downloaded.
Maybe You are about to delete nodes which can have other referrers not yet downloaded.?

comment:4 Changed 7 months ago by skyper

Yes, better and in sync with possible future warning You are about to move nodes which can have other referrers not yet downloaded.

comment:5 Changed 7 months ago by GerdP

Resolution: fixed
Status: newclosed

In 16365/josm:

fix #19121: JosmAction.checkAndConfirmOutlyingOperation() should also handle dataset without download area

  • adjust message texts
  • also check if object is old and no download area exists

comment:6 Changed 7 months ago by skyper

I am not sure about unglue and ways with id:0:

  1. split a way with at least one node inside downloaded area
  2. unglue node of just created new way

Think here a warning is still useful.

If you "unglue" or "extract node" in advance, similar can happen with nodes with id:0.

comment:7 Changed 7 months ago by GerdP

Not related to this ticket. I did not suppress any warning. I've only added one for the case that no download area exists.

comment:8 in reply to:  7 Changed 7 months ago by skyper

Replying to GerdP:

I've only added one for the case that no download area exists.

Exactly, I might be reading this wrong but, in my eyes, we'll miss some cases with ways with id:0 :

  • trunk/src/org/openstreetmap/josm/command/Command.java

    a b  
    229229                res |= IS_INCOMPLETE;
    230230            } else if (osm.isOutsideDownloadArea()
    231                     && (ignore == null || !ignore.contains(osm))) {
     231                    || (!osm.isNew() && osm.getDataSet() != null && osm.getDataSet().getDataSourceBounds().isEmpty())
     232                            && (ignore == null || !ignore.contains(osm))) {
    232233                res |= IS_OUTSIDE;
    233234            }
Last edited 7 months ago by skyper (previous) (diff)

comment:9 Changed 7 months ago by GerdP

!osm.isNew() is equivalent to id:0. I don't like to use osm.getId() == 0 because one always has to check twice to make sure that osm.getId() always returns 0 for negative ids.

comment:10 Changed 7 months ago by skyper

Thanks for explanation, guess I have to train my eyes/brain to better read/understand diffs.

Last edited 7 months ago by skyper (previous) (diff)

comment:11 Changed 7 months ago by GerdP

In fact I should have written osm.isNew() is equivalent to osm.getId() == 0. For a new object we cannot download parents.

Looking at the patch again I think a pair of() is missing :(

comment:12 Changed 7 months ago by GerdP

In 16369/josm:

see #19121: JosmAction.checkAndConfirmOutlyingOperation() should also handle dataset without download area

  • add missing brackets so that possible ignore list works

comment:13 Changed 7 months ago by GerdP

In 16371/josm:

see #19121: fix unit test

comment:14 Changed 7 months ago by GerdP

Milestone: 20.04

comment:15 Changed 7 months ago by Klumbumbus

Milestone: 20.0420.05

Milestone renamed

comment:16 Changed 6 months ago by GerdP

Resolution: fixed
Status: closedreopened

r16365 introduced a regression. When an empty relation is deleted and there is no download area a popup "You are about to delete nodes which can have other referrers not yet downloaded..." is shown.

Last edited 6 months ago by GerdP (previous) (diff)

comment:17 Changed 6 months ago by GerdP

Resolution: fixed
Status: reopenedclosed

In 16513/josm:

fix #19121: JosmAction.checkAndConfirmOutlyingOperation() should also handle dataset without download area

  • restrict additional check if dataset has a download area to nodes

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.