Modify

Opened 5 years ago

Closed 2 years ago

Last modified 23 months ago

#17906 closed defect (fixed)

[PATCH] When dragging and dropping relation members to the same position, they are removed

Reported by: taylor.smock Owned by: taylor.smock
Priority: major Milestone: 22.05
Component: Core Version: tested
Keywords: relation member dnd drag-and-drop regression Cc: simon04

Description

Steps to reproduce:

  1. Get a relation with at least one member
  2. Drag and drop any member to the same position (black bar above the currently selected element)
  3. See member list decrease by one

Note: This can be done with multiple members.

This bug exists in r15224 and later (may exist earlier, I don't have enough time right now to do a binary search).

Attachments (1)

17906.patch (8.3 KB ) - added by taylor.smock 2 years ago.
Fix deletion issue, add non-regression test, fix performance issue when moving many members

Download all attachments as: .zip

Change History (21)

comment:1 by Don-vip, 5 years ago

Summary: When dragging and dropping relations to the same position, they are removedWhen dragging and dropping relation members to the same position, they are removed

comment:2 by pl71, 5 years ago

confirmed

comment:3 by njtbusfan, 5 years ago

Is this the same bug that sees order get screwed up when you select a bunch of elements and move them up or down?

comment:4 by Don-vip, 5 years ago

@njtbusfan no this one has been fixed, please update to JOSM 15238.

comment:5 by Don-vip, 5 years ago

Milestone: 19.09
Owner: changed from team to Don-vip
Priority: normalmajor
Status: newassigned

comment:6 by Don-vip, 5 years ago

Keywords: member added

Bug already present in r14824 so not a recent regression.

comment:7 by Don-vip, 5 years ago

Cc: simon04 added
Keywords: dnd drag-and-drop added

Probably here since we introduced the feature in #12300

comment:8 by Don-vip, 5 years ago

Owner: changed from Don-vip to team
Status: assignednew

Mmm I debugged a little bit and don't see what's wrong. Simon, do you have an idea?

comment:9 by simon04, 5 years ago

When dropping one element at its position in the table, strangely, the selection inside addMembersAtIndexKeepingOldSelection to both elements (the previously selected one and the inserted one). Thus, both are deleted in org.openstreetmap.josm.gui.dialogs.relation.MemberTransferHandler#exportDone.

Not sure why the selection expands. Vincent, do you have an idea.

For debugging:

  • src/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModel.java

    diff --git a/src/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModel.java b/src/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModel.java
    index a2e4084ee..57ee139e2 100644
    a b  
    22package org.openstreetmap.josm.gui.dialogs.relation;
    33
    44import java.util.ArrayList;
     5import java.util.Arrays;
    56import java.util.BitSet;
    67import java.util.Collection;
    78import java.util.Collections;
    RelationMember getRelationMemberForPrimitive(final OsmPrimitive primitive) {  
    447448    }
    448449
    449450    void addMembersAtIndexKeepingOldSelection(final Iterable<RelationMember> newMembers, final int index) {
     451        System.out.println(Arrays.toString(getSelectedIndices()));
    450452        int idx = index;
    451453        for (RelationMember member : newMembers) {
    452454            members.add(idx++, member);
    453455        }
    454456        invalidateConnectionType();
    455457        fireTableRowsInserted(index, idx - 1);
     458        System.out.println(Arrays.toString(getSelectedIndices()));
    456459    }
    457460
    458461    public void addMembersAtBeginning(List<? extends OsmPrimitive> primitives) {

comment:10 by Don-vip, 5 years ago

When we call fireTableRowsInserted(index, idx - 1); we execute this code of JTable:

    /*
     * Invoked when rows have been inserted into the table.
     * <p>
     * Application code will not use these methods explicitly, they
     * are used internally by JTable.
     *
     * @param e the TableModelEvent encapsulating the insertion
     */
    private void tableRowsInserted(TableModelEvent e) {
        int start = e.getFirstRow();
        int end = e.getLastRow();
        if (start < 0) {
            start = 0;
        }
        if (end < 0) {
            end = getRowCount()-1;
        }

        // Adjust the selection to account for the new rows.
        int length = end - start + 1;
        selectionModel.insertIndexInterval(start, length, true);

then in DefaultListSelectionModel

    /**
     * Insert length indices beginning before/after index. If the value
     * at index is itself selected and the selection mode is not
     * SINGLE_SELECTION, set all of the newly inserted items as selected.
     * Otherwise leave them unselected. This method is typically
     * called to sync the selection model with a corresponding change
     * in the data model.
     */
    public void insertIndexInterval(int index, int length, boolean before)
    {
        /* The first new index will appear at insMinIndex and the last
         * one will appear at insMaxIndex
         */
        int insMinIndex = (before) ? index : index + 1;
        int insMaxIndex = (insMinIndex + length) - 1;

        /* Right shift the entire bitset by length, beginning with
         * index-1 if before is true, index+1 if it's false (i.e. with
         * insMinIndex).
         */
        for(int i = maxIndex; i >= insMinIndex; i--) {
            setState(i + length, value.get(i)); // <- selection changes here
        }
Last edited 5 years ago by Don-vip (previous) (diff)

comment:11 by Don-vip, 4 years ago

Milestone: 19.0919.10

comment:12 by Don-vip, 4 years ago

Milestone: 19.10

comment:13 by GerdP, 3 years ago

I cannot reproduce this with r17432. Seems it was fixed?

comment:14 by taylor.smock, 2 years ago

Ticket #21889 has been marked as a duplicate of this ticket.

comment:15 by taylor.smock, 2 years ago

This duplicates #12617 (fixed in r9993, fix probably accidentally removed in r10604).

I'll go ahead and add a non-regression test so it doesn't happen in the future.

comment:16 by taylor.smock, 2 years ago

Keywords: regression added
Owner: changed from team to taylor.smock
Status: newassigned

by taylor.smock, 2 years ago

Attachment: 17906.patch added

Fix deletion issue, add non-regression test, fix performance issue when moving many members

comment:17 by taylor.smock, 2 years ago

Milestone: 22.05
Summary: When dragging and dropping relation members to the same position, they are removed[PATCH] When dragging and dropping relation members to the same position, they are removed

Notes on attachment:17906.patch:

  • 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.

Last edited 2 years ago by taylor.smock (previous) (diff)

comment:18 by taylor.smock, 2 years ago

Resolution: fixed
Status: assignedclosed

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:19 by stoecker, 23 months ago

Milestone: 22.0522.07

Milestone renamed

comment:20 by stoecker, 23 months ago

Milestone: 22.0722.05

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain taylor.smock.
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.