Modify

Opened 2 years ago

Closed 2 years ago

Last modified 15 months ago

#21889 closed defect (fixed)

[PATCH] Java JTable adds a new row to the selection when calling fireTableRowsInserted. This causes us to remove both the new (moved) row and the old (non-moved) row

Reported by: verdyp@… Owned by: team
Priority: normal Milestone: 22.05
Component: Core Version: latest
Keywords: template_report Cc:

Description

What steps will reproduce the problem?

  1. open the relation editor with any relation containing a few items
  2. select any item
  3. start dragging them on the list: while your click is maintained you see the Drag-n-drop cursor, and a selected target as a thick cell border between rows
  4. release the click: all the selected members are NOT moved to the target selection, in fact they are just removed from the list (if the selection was all members, now all members are removed, but generally for relations with many members, you may not even notice that this removed some members from the list while just trying to select another row)
  5. try pressing CTRL+Z to undo: this has no effect in the relation editor.

Editing relations with the relation editor and using the mouse to select some items is UNRELIABLE: you can inadvertantly remove items from relations (and it's impossible to undo)

What is the expected result?

Drag-n-Drop is currently enabled, but NOT implemented at all, it NEVER works in the list of members shown the relation editor.

What happens instead?

Inadvertantly broken relations with missing members that are silently removed from relations, when one just wanted to select some item (Drag-n-Drop can occur jut because the click may initiate a small drag if the mouse just moves 1 pixel while clicking: selecting rows with the mouse requires absolutely not moving the mouse and this is very unreliable, and even invisible while editing large relations)

Please provide any additional information below. Attach a screenshot if possible.

This bug has been reported multiple times by since several years, and considered minor, systematically neglected. But it is recurrent in all versions of JOSM since many versions. It causes unexpected loss of information when using the relation editor (most of these being unnoticable by the user).

Editing relations with many members with JOSM is still VERY unreliable and dangerous for data. Please admlit that this must be fixed.

And because Drag-n-drop has in fact NEVER worked in the relation editor, it should be disabled completely (never accept a drag-n-drop action in the relation editor: so never drop members from the list (if you do that before adding them again, this does not work: the removal has the effect of deleting the references and make them invalid for their reinsertion in the list)

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2022-01-02 21:24:43 +0100 (Sun, 02 Jan 2022)
Revision:18360
Build-Date:2022-01-02 20:26:19
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (18360 fr) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (22557)
Memory Usage: 3025 MB / 6106 MB (2334 MB allocated, but free)
Java version: 11.0.13+8-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1680×1050 (scaling 1.00×1.00) \Display1 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: fr_FR
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Dicedtea-web.bin.location=C:\Program Files\OpenWebStart\javaws, -Djava.util.Arrays.useLegacyMergeSort=true, --add-exports=jdk.deploy/com.sun.deploy.config=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-reads=java.naming=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.action=ALL-UNNAMED,java.desktop, --add-reads=java.base=ALL-UNNAMED,java.desktop, --add-exports=java.naming/com.sun.jndi.toolkit.url=ALL-UNNAMED,java.desktop, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-exports=java.desktop/com.apple.eawt=ALL-UNNAMED, --add-exports=java.desktop/sun.awt=ALL-UNNAMED,java.desktop, --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED, --add-exports=java.base/sun.security.validator=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/com.sun.imageio.plugins.jpeg=ALL-UNNAMED, --add-exports=java.base/sun.net.www.protocol.jar=ALL-UNNAMED,java.desktop, --add-exports=java.base/jdk.internal.util.jar=ALL-UNNAMED,java.desktop, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, --add-exports=java.base/com.sun.net.ssl.internal.ssl=ALL-UNNAMED,java.desktop, --add-exports=javafx.graphics/com.sun.javafx.application=ALL-UNNAMED, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, --add-exports=java.desktop/sun.awt.X11=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/sun.applet=ALL-UNNAMED,java.desktop,jdk.jsobject, --add-exports=java.base/sun.net.www.protocol.http=ALL-UNNAMED,java.desktop, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-exports=java.base/sun.security.util=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED, --add-reads=java.desktop=ALL-UNNAMED,java.naming, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-exports=java.base/sun.security.x509=ALL-UNNAMED,java.desktop, --add-opens=java.desktop/javax.imageio.spi=ALL-UNNAMED, --add-exports=java.desktop/javax.jnlp=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.provider=ALL-UNNAMED,java.desktop]
Dataset consistency test: No problems found

Last errors/warnings:
- 00000.839 W: extended font config - overriding 'filename.Myanmar_Text=mmrtext.ttf' with 'MMRTEXT.TTF'
- 00000.844 W: extended font config - overriding 'filename.Mongolian_Baiti=monbaiti.ttf' with 'MONBAITI.TTF'
- 00005.048 W: Unable to request certificate of https://roottest-g3.pkioverheid.nl
- 00005.635 W: Unable to request certificate of https://roottest-g3.pkioverheid.nl
- 00008.959 W: Unable to request certificate of https://grca.nat.gov.tw

Attachments (2)

josm_relation_draganddrop-2022-03-12_22.08.47.gif (668.9 KB ) - added by Woazboat 2 years ago.
21889.patch (1.7 KB ) - added by taylor.smock 2 years ago.
Modify MemberTableModel#addMembersAtIndexKeepingOldSelection to have the implied behavior (keep the old selection)

Download all attachments as: .zip

Change History (20)

comment:1 by Woazboat, 2 years ago

To prefix this, I'm not a josm maintainer and the following is purely my own opinion and not representative of the josm project by any means.

If you want anyone to actually take you seriously, I would recommend using the bug report/defect ticket function to actually report bugs with useful and actionable information and not just for DEMANDING that SOMEONE must IMMEDIAtELy dO SomethiNG that you believe is the one and only right course of action and suggesting that the only reason it hasn't been done so far is ignorance or willful negligence by the josm maintainers.

Now let's break down some of the problems with this bug report:

First off, the random shouting/capitalization adds absolutely nothing of value and only makes you sound immature and demanding.


What steps will reproduce the problem?

Editing relations with the relation editor and using the mouse to select some items is UNRELIABLE: you can inadvertantly remove items from relations (and it's impossible to undo)

The individual sections are there for a reason and should be followed to make the ticket actually understandable. Don't put your personal opinions and conclusions into the 'how to reproduce' section.


What is the expected result?

Drag-n-Drop is currently enabled, but NOT implemented at all, it NEVER works in the list of members shown the relation editor.

This is not an 'expected result' but rather your own personal conclusion.
How did you come to this conclusion that the problem is that it's not actually implemented at all? Random guessing is not very helpful.


(Drag-n-Drop can occur jut because the click may initiate a small drag if the mouse just moves 1 pixel while clicking

This sounds like a different problem than 'drag and drop does not work'


This bug has been reported multiple times by since several years, and considered minor, systematically neglected.

Care to back up these claims with actual evidence? (Like links to these claimed 'multiple other reports')

If there is already a ticket for exactly the problem you are facing, don't create a new duplicate ticket.


But it is recurrent in all versions of JOSM since many versions.

Unspecific claims like these do not actually provide any useful information


Please admlit that this must be fixed.

You have no place asking anyone here to 'admit' something as if they were guilty of (as you claimed) intentional wrongdoing and they owed you atonement.


(if you do that before adding them again, this does not work: the removal has the effect of deleting the references and make them invalid for their reinsertion in the list)

It is completely unclear (to me) what you mean by this and I can only guess. The 'add to relation' button/action does not work for the dragged and supposedly removed elements afterwards?


Priority: critical

That's a bit overselling it


This reply may sound a bit rude but hey, you get out what you put in...

Last edited 2 years ago by Woazboat (previous) (diff)

comment:2 by Woazboat, 2 years ago

Drag & drop works completely fine by the way


try pressing CTRL+Z to undo: this has no effect in the relation editor.

That's because (unlike drag & drop) this is _actually_ not implemented in the relation editor. Feel free to submit a feature request and patches.

(Drag-n-Drop can occur jut because the click may initiate a small drag if the mouse just moves 1 pixel while clicking: selecting rows with the mouse requires absolutely not moving the mouse and this is very unreliable,

This is the opposite of my experience. Drag & drop only works _after_ the corresponding members have already been selected beforehand. Clicking and dragging starting from a previously unseleted member actually starts a selection and not a drag&drop.

Editing relations with many members with JOSM is still VERY unreliable and dangerous for data.

No OSM editor is completely flawless when it comes to relations but JOSM is undoubtedly the best suited for the job and most reliable and definitely does not pose a danger to OSM data like you claim.


Now that all of this has been said: I'm not denying that you might be facing these problems, but you need to realize that this seems to be a problem with your specific setup and you need to act accordingly if you are interested in receiving any help to get this fixed. Maybe start by actually helping to narrowing down the root cause by e.g. finding out if this is/was reproducible by anyone else (for example in other josm tickets) and looking for a common denominator.

Last edited 2 years ago by Woazboat (previous) (diff)

comment:3 by taylor.smock, 2 years ago

Priority: criticalnormal
Resolution: worksforme
Status: newclosed

comment:4 by anonymous, 2 years ago

This bug has been closed but never been corrected. Spurious deletions still occurs when clicking any row and moving it accidentally by one pixel, causing a drag-nèdrop operation that actually deletes the whole current selection (and no way to undo it).
Drag-n-drop actualyl does not work and has never worked, it first deletes the initial "dragged" selection from its current position but is unable to reinsert it at the "new" position.
Such action is not always visible, it occurs instanatly with no evident user feedback. If you save the edited relation were you did not intend to remvoe anything, members will now be missing in the relation, the relation will be broken (without necessarily implying any validation error before submitting data to the server, notably for route relations where you just intended to reorder members in the list, such as bus stops in a bus route).
And this bug is still very easy to reproduce. It causes unexpected changes in the database when the relation will be submitted, and it is a big source of loss of time to fix it again, trying to figure out what was missing and was accidentally removed.
I've never seen any working or valid use of drag-n-drop support for rows inside the same edited list of relation members. I do not understand why this spurious drag-n-drop support is there, as it has never worked.

comment:5 by anonymous, 2 years ago

Alsol sthis is not a problem of "my" setup. This affects any version of JOSM since years. in all types of relations, and independantly of the data loaded, and all OSes or version of the JRE. Your animation above is NOT the way to reproduce it:
make a selection, but start dragging it jsut a few pixels so that the drop position is still inside the current selection: you'll see that all the selected rows are deleted, but NOT just moved !
Such action (movind a few pixels) is not stupid, it just happens by accident when we perform selection of multiple rows (e.g. with CTRL+click or SHIFT+click, the subsequetnt click to change the selection may cause such spurious move by one pixel with a non-intended drag-n-drop being initiated, but not working and deleting all rows in the current selection.

comment:6 by GerdP, 2 years ago

Resolution: worksforme
Status: closedreopened

I can reproduce the error:
1) Open relation with some members (I used https://www.osm.org/relation/3382182 for my tests)
2) Select a member in the middle of the list and release mouse button so that the member stays selected
3) drag that member a few pixels to the top
4) A thicker black line is rendered between the dragged line and the one above and the mouse cursors flickers
5) If you release the mouse button in that stage the member is deleted.

I don't think this is a normal behaviour. Maybe it only happens on Windows?

Last edited 2 years ago by GerdP (previous) (diff)

comment:7 by taylor.smock, 2 years ago

It looks like it happens when you drag'n'drop to the current position.

It appears to be a selection issue problem. I'm working on tracking that down.

comment:8 by taylor.smock, 2 years ago

Summary: Please DISABLE completely the BROKEN Drag-n-Drop of selected rows inside list of members of the relation editor[PATCH] Java JTable adds a new row to the selection when calling fireTableRowsInserted. This causes us to remove both the new (moved) row and the old (non-moved) row

I'm attaching a patch that fixes the issue. I don't know if we are doing a release this weekend, so I'm not going to apply the patch. The behavior has been broken for quite some time ("[this] bug has been reported multiple times by since several years").

@verdyp:
If step 3 in the original report had something like "[just] a few pixels so that the drop position is still inside the current selection", I would not have closed it as worksforme. You also could have added the additional information after @Woazboat asked for it.

In short, your initial bug report made it sound like it happened regardless of where the relation members were dragged'n'dropped, and I was unable to reproduce the problem before I closed the issue.

by taylor.smock, 2 years ago

Attachment: 21889.patch added

Modify MemberTableModel#addMembersAtIndexKeepingOldSelection to have the implied behavior (keep the old selection)

comment:9 by anonymous, 2 years ago

You are wrong, I've already reported several times (since years) that this occured by just clicking while the mouse was moving (accidentally) by just one or a few pixels. Each time this was not considered and closed early without any form of discussion. Each time I said it was easily reproducible, but you never tested what I described, certainly making false assumptions.
And anyway, even the normal drag-n-dop (more than a few pixels, or when dropping to a position in the middle of the selection, but still NOT between two selected rows) caused the same issue (occuring when using selection of multiple rows, possibly with unselectd holes in the middle. Each time the rows were deleted by accident just because the selection click was slightly moving the mouse by a few pixels (something that can happen very easily with any mouse, because there's no threshold to the minimum mouse move occuring after a click before incorectly initiating an undesired Drag-n-drop that in fact has never worked correctly anyway).

comment:10 by anonymous, 2 years ago

Also you made this false statement, when I was clear:

(Drag-n-Drop can occur jut because the click may initiate a small drag if the mouse just moves 1 pixel while clicking: selecting rows with the mouse requires absolutely not moving the mouse and this is very unreliable,

This is the opposite of my experience. Drag & drop only works _after_ the corresponding members have already been selected beforehand. > Clicking and dragging starting from a previously unseleted member actually starts a selection and not a drag&drop.

You actually did not experience that (even if it was clear enough) and closed the report. I had said the same thing in previous reports closed repeatedly since several years without any test and without waiting for others to see it and comment that they had the same experience (but did not notice it).

And this explains many strange edits in relations that have already occured in the past with JOSM relation editor, that caused many relations to be found later as being broken (and sometimes hard to repair, because using the online history is complex, notably on large relations that have been broken repeatedly with missing members accierntally deleted, sometimes many versions before that are hard to find in the hostory)

comment:11 by GerdP, 2 years ago

@anonymous, please calm down! You write to a team of volunteers. Don't you think you should thank tailor.smock for actually implementing a fix? And please let us know the previous tickets so we can add a reference to this one. I could not find any but maybe I searched for the wrong key words.

in reply to:  11 comment:12 by taylor.smock, 2 years ago

Resolution: duplicate
Status: reopenedclosed

Closed as duplicate of #17906.

comment:13 by taylor.smock, 2 years ago

Copying comment:17:ticket:17906 here for verdyp/anonymous and Woazboat:


Notes on attachment:17906.patch:ticket:17906

  • Fixes #17906, #21889, see #12617 for the original issue
  • #12617 was partially fixed in r9993 for single selections (but not for multiple selections). Fix was probably accidentally removed in r10604.
  • Improves performance when moving large numbers of relation members (e.g., if someone decides that moving 99% of Lake Huron's members is a good idea, it won't take <some really long time> to finish the move).

I don't think we are doing a 22.04 release, but I'll merge the patch Wednesday just in case we do decide to do a 22.04 release.

comment:14 by anonymous, 2 years ago

About Huron Lake: it has so many members that we should have a way to split it in subareas, those ausbareas being then assembled to the whole lake.

Such splitting should avoid using arebitrary lines, at least it should be splitted into the two parts between US and Canada.
The same should be done as weel for Lake Superior, Lake Erie and Lake Ontario, and as well in other countries like Lake Chad, or Geneva Lake/Leman. (also for large coastal maritime zones, river banks, forests, agricultural areas?)

Why doesn't OSM define a *standardized* scheme for representing large complex relations representing areas as an assemblee of several subareas that can be edited much more easily, more locally, avoiding the limitation on the maximum number of members and avoiding the creation of holes.

At least this could be done with standard tags, but in fact it is a generic geometrical problem that should be independant of tags, using new geometric types (other than just "relations" that were not really made for that to represent areas).

Since long then, we have custom types, not used directly in OSM, but in derived databases, notably for the rendering (notably seas, but why not as well for large lakes or forests), with additional restrictions.

Such new type should be able as well to represent virtual borders, i.e. special types for border lines saying that it is shared between two adjascent parts, and not part of the effective outer or inner borders of the whole area. This would as well solve the problem for areas splitted along the international dateline, that are for now splitted arebitrarily with a fake border rendered along this line. And it would also solve the representation of areas surrounding poles with a virtual horizontal segment passing through the pole along all longitudes plus another virtual segment along the international dateline meridian.

And if needed, the editor could assist in splitting these too large and too complex areas. If we have virtual spltiting lines, why not automating their creation by using limits between geodesic "quads", the same used in tiled renderers (this would also benefor to renderers, by allowing faster searches for enclosing polygons, and reducing as well the overhead on rendering servers to refresh tiles: we could have an automatic splitting based on a maximum surface size (in degrees of longitude, and a variable size for latitude using the same formula used for effective length of the same physical distance along meridians). The editor would compute the beast exact positions where to split virtually; it would generate tiled areas represented in subobjects that are assembled into the larger area (no longer a "relation" but a new geometric type specific for areas). In this scheme, borders of virtual tiled subsares would have three types: "inner" or "outer" for real borders of the whole assemblee, plus some "splitting" type for splitting virtual lines shared by two adjascent subareas that are part of the whole inner area (delimited by "outer" and "inner" borderlines).

If a relation is small enough, this default splitting scheme may be tuned to use other existing lines (such as administrative boundaries): this would also enable faster searches for geolocation without needing complex searches with a "buffer" around the effective zone to make sure we have enough nodes to find all contained areas.

As well, this assisted tool would make sure it generates enough nodes along long segments (i.e. not longer than 1km for example) and would automatically add "virtual" nodes just to fit this constraint (virtual nodes would have no tag at all and only created as part of a "way" definition, and possibly would not even need any "node id", meaning that they would only be attached to a single way definining them; or the "node id" would take a special form like "wayid"+"."+"virtual node id").

Of course this would require an extension of the existing OSM data schema (not fully compatible with the existing one: new formats for virtual node ids, virtual subareas; new object types for "way fragments", "area fragment", and "area").

in reply to:  14 comment:15 by taylor.smock, 2 years ago

Replying to anonymous:

About Huron Lake: it has so many members that we should have a way to split it in subareas, those ausbareas being then assembled to the whole lake.

Such splitting should avoid using arebitrary lines, at least it should be splitted into the two parts between US and Canada.
The same should be done as weel for Lake Superior, Lake Erie and Lake Ontario, and as well in other countries like Lake Chad, or Geneva Lake/Leman. (also for large coastal maritime zones, river banks, forests, agricultural areas?)

Why doesn't OSM define a *standardized* scheme for representing large complex relations representing areas as an assemblee of several subareas that can be edited much more easily, more locally, avoiding the limitation on the maximum number of members and avoiding the creation of holes.

Feel free to make a proposal about this. I would not be surprised if there were already several proposals that have been rejected, so you might want to look for those and solve the problems that caused the proposals to be rejected. But OSM did recently introduce limits to the number of relation members any one relation could have (see #21826, OSM has a 32k relation member limit, Lake Huron is ~14k members, IIRC).

[...snip...]
Of course this would require an extension of the existing OSM data schema (not fully compatible with the existing one: new formats for virtual node ids, virtual subareas; new object types for "way fragments", "area fragment", and "area").

We (JOSM devs) cannot change the OSM data model. That would be something to bring up at openstreetmap-website.

With all that said, Lake Huron is a great place to look for performance issues in the relation editor, just due to its shear size (I want to say GerdP commented on a perf issue with one of my patches on it once). Quite frankly, it is probably a better idea to fix the performance issue than introduce database complexity along with additional complexity for downstream data consumers. The last time any significant change was made to the OSM data model was ~15 years ago (I believe relations were added in osmwiki:API_v0.5).
Since the base OSM data model has been effectively static for 15 years, I highly doubt it is going to change any time soon. I rather suspect they will want to maintain backwards compatibility for any data model change, which brings us back to software complexity.

comment:16 by anonymous, 2 years ago

Note that this is not just affecting JOSM. Large relations like those for the Great Lakes, large bays, forests or riverbanks of long rivers with many islands and branches, are all affecting as well the renderers, the geographic searches, and all other editors (including the online editor used on the OSM web site, which cannot load these relations at all and fail after timeout on the server, or that crash the web browser or hang it for so much time that the webbowser will want to stop loading the page).

There's a solution to do, splitting with virtual subareas can be done in the background, without necessarily changing the data model.

But the datamodel should really be improved (notably for nodes: all nodes that below to a single way and have no attributes at all are costly and could be as well represetned in a much less costly fashion, in the data model, or with an API that simplifies them in a new representation that can be automatically created on the fly; as well splitting areas in quads can be done virtually; but chaing the data model for nodes would tremendously reduce the size and the efficiency of the database. and such virtualization of the geometries using a better internal model is also possible within editors themselves, or in renderers (and yes it can greatly improve the performance and can be seen in that case as an optimization, without necessarily chaning the data model exposed by API v0.5; and this also does not prohibit the server to expose an optimized API for queries which can better group objects and reduce considerably the query volumes).

comment:17 by taylor.smock, 2 years ago

Resolution: duplicatefixed

In 18439/josm:

Fix #17906, #21889, see #12617 for the original issue

This fixes an issue where a relation member could be deleted accidentally
when a user moved the relation member to its current position. This also
affected multiple members when multiple members were selected. The user
just had to move the selected members to another selected member position.

This patch also improves relation move performance when large numbers of
relation members are moved.

comment:18 by taylor.smock, 15 months ago

Milestone: 22.05

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. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.