Maint: Normalize row index in TabularModel

traitsui/qt4/tabular_model.py

@@ -20,7 +20,7 @@
 
 
 
-
+import logging
 
 from pyface.qt import QtCore, QtGui
 
@@ -39,6 +39,8 @@
 # MIME type for internal table drag/drop operations
 tabular_mime_type = "traits-ui-tabular-editor"
 
+logger = logging.getLogger(__name__)
+
 
 class TabularModel(QtCore.QAbstractTableModel):
     """ The model for tabular data."""
@@ -275,6 +277,15 @@ def dropMimeData(self, mime_data, action, row, column, parent):
         if action == QtCore.Qt.IgnoreAction:
             return False
 
+        # If dropped directly onto the parent, both row and column are -1.
+        # See https://doc.qt.io/qt-5/qabstractitemmodel.html#dropMimeData
+        # When dropped below the list, the "parent" is invalid.
+        if row == -1:
+            if parent.isValid():
+                row = parent.row()
+            else:
+                row = max(0, self.rowCount(None) - 1)
+
         # this is a drag from a tabular model
         data = mime_data.data(tabular_mime_type)
         if not data.isNull() and action == QtCore.Qt.MoveAction:
@@ -285,7 +296,7 @@ def dropMimeData(self, mime_data, action, row, column, parent):
             # is it from ourself?
             if table_id == id(self):
                 current_rows = id_and_rows[1:]
-                self.moveRows(current_rows, parent.row())
+                self.moveRows(current_rows, row)
                 return True
 
         # this is an external drag
@@ -297,12 +308,6 @@ def dropMimeData(self, mime_data, action, row, column, parent):
             object = editor.object
             name = editor.name
             adapter = editor.adapter
-            if row == -1 and parent.isValid():
-                # find correct row number
-                row = parent.row()
-            if row == -1 and adapter.len(object, name) == 0:
-                # if empty list, target is after end of list
-                row = 0
             if all(
                 adapter.get_can_drop(object, name, row, item) for item in data
             ):
@@ -345,11 +350,13 @@ def moveRows(self, current_rows, new_row):
         editor = self._editor
 
         if new_row == -1:
-            # In some cases, the new row may be reported as -1 (e.g. when
-            # dragging and dropping a row at the bottom of existing rows). In
-            # that case, adjust to the number of existing rows.
-            new_row = self.rowCount(None)
-
+            # row should be nonnegative and less than the row count for this
+            # model. See ``QAbstractItemModel.checkIndex``` for Qt 5.11+
+            # This is a last resort to prevent segmentation faults.
+            logger.debug(
+                "Received invalid row %d. Adjusting to the last row.", new_row)
+            new_row = max(0, self.rowCount(None) - 1)
+
         # Sort rows in descending order so they can be removed without
         # invalidating the indices.
         current_rows.sort()

traitsui/qt4/tests/test_tabular_model.py

@@ -0,0 +1,172 @@
+#  Copyright (c) 2020, Enthought, Inc.
+#  All rights reserved.
+#
+#  This software is provided without warranty under the terms of the BSD
+#  license included in LICENSE.txt and may be redistributed only
+#  under the conditions described in the aforementioned license.  The license
+#  is also available online at http://www.enthought.com/licenses/BSD.txt
+#
+#  Thanks for using Enthought open source!
+#
+
+""" Tests for TabularModel (an implementation of QAbstractTableModel)
+"""
+
+import unittest
+
+from traits.api import HasTraits, List, Str
+from traitsui.api import Item, TabularEditor, View
+from traitsui.tabular_adapter import TabularAdapter
+
+from traitsui.tests._tools import (
+    create_ui,
+    is_current_backend_qt4,
+    skip_if_not_qt4,
+    store_exceptions_on_all_threads,
+)
+try:
+    from pyface.qt import QtCore
+except ImportError:
+    # The entire test case should be skipped if the current backend is not Qt
+    # But if it is Qt, then re-raise
+    if is_current_backend_qt4():
+        raise
+
+
+class DummyHasTraits(HasTraits):
+    names = List(Str)
+
+
+def get_view(adapter):
+    return View(
+        Item(
+            "names",
+            editor=TabularEditor(
+                adapter=adapter,
+            ),
+        )
+    )
+
+
+@skip_if_not_qt4
+class TestTabularModel(unittest.TestCase):
+
+    def test_drop_mime_data_below_list(self):
+        # Test dragging an item in the list and drop it below the last item
+        obj = DummyHasTraits(names=["A", "B", "C", "D"])
+        view = get_view(TabularAdapter(columns=["Name"]))
+        with store_exceptions_on_all_threads(), \
+                create_ui(obj, dict(view=view)) as ui:
+            editor, = ui.get_editors("names")
+
+            model = editor.model
+            # sanity check
+            self.assertEqual(model.rowCount(None), 4)
+
+            # drag and drop row=1 from within the table.
+            # drag creates a PyMimeData object for dropMimeData to consume.
+            index = model.createIndex(1, 0)
+            mime_data = model.mimeData([index])
+
+            # when
+            # dropped below the list, the "parent" is invalid.
+            parent = QtCore.QModelIndex()   # invalid index object
+            model.dropMimeData(mime_data, QtCore.Qt.MoveAction, -1, -1, parent)
+
+            # then
+            mime_data = model.mimeData(
+                [model.createIndex(i, 0) for i in range(model.rowCount(None),)]
+            )
+            content = mime_data.instance()
+            self.assertEqual(content, ["A", "C", "D", "B"])
+            self.assertEqual(obj.names, content)
+
+    def test_drop_mime_data_within_list(self):
+        # Test dragging an item in the list and drop it somewhere within the
+        # list
+        obj = DummyHasTraits(names=["A", "B", "C", "D"])
+        view = get_view(TabularAdapter(columns=["Name"]))
+
+        with store_exceptions_on_all_threads(), \
+                create_ui(obj, dict(view=view)) as ui:
+            editor, = ui.get_editors("names")
+
+            model = editor.model
+            # sanity check
+            self.assertEqual(model.rowCount(None), 4)
+
+            # drag and drop from within the table.
+            # drag row index 0
+            index = model.createIndex(0, 0)
+            mime_data = model.mimeData([index])
+
+            # when
+            # drop it to row index 2
+            parent = model.createIndex(2, 0)
+            model.dropMimeData(mime_data, QtCore.Qt.MoveAction, -1, -1, parent)
+
+            # then
+            mime_data = model.mimeData(
+                [model.createIndex(i, 0) for i in range(model.rowCount(None),)]
+            )
+            content = mime_data.instance()
+            self.assertEqual(content, ["B", "C", "A", "D"])
+            self.assertEqual(obj.names, content)
+
+    def test_copy_item(self):
+        # Test copy 'A' to the row after 'C'
+        obj = DummyHasTraits(names=["A", "B", "C"])
+        view = get_view(TabularAdapter(columns=["Name"], can_drop=True))
+
+        with store_exceptions_on_all_threads(), \
+                create_ui(obj, dict(view=view)) as ui:
+            editor, = ui.get_editors("names")
+
+            model = editor.model
+            # sanity check
+            self.assertEqual(model.rowCount(None), 3)
+
+            # drag and drop from within the table for copy action.
+            # drag index 0
+            index = model.createIndex(0, 0)
+            mime_data = model.mimeData([index])
+
+            # when
+            # drop to index 2
+            parent = model.createIndex(2, 0)
+            model.dropMimeData(mime_data, QtCore.Qt.CopyAction, -1, -1, parent)
+
+            # then
+            self.assertEqual(model.rowCount(None), 4)
+            mime_data = model.mimeData(
+                [model.createIndex(i, 0) for i in range(model.rowCount(None),)]
+            )
+            content = mime_data.instance()
+            self.assertEqual(content, ["A", "B", "C", "A"])
+            self.assertEqual(obj.names, content)
+
+    def test_move_rows_invalid_index(self):
+        # Test the last resort to prevent segfault
+
+        obj = DummyHasTraits(names=["A", "B", "C"])
+        view = get_view(TabularAdapter(columns=["Name"]))
+
+        with store_exceptions_on_all_threads(), \
+                create_ui(obj, dict(view=view)) as ui:
+            editor, = ui.get_editors("names")
+
+            model = editor.model
+            # sanity check
+            self.assertEqual(model.rowCount(None), 3)
+
+            # when
+            # -1 is an invalid row. This should not cause segfault.
+            model.moveRows([1], -1)
+
+            # then
+            mime_data = model.mimeData(
+                [model.createIndex(i, 0) for i in range(model.rowCount(None),)]
+            )
+            content = mime_data.instance()
+            self.assertEqual(content, ["A", "C", "B"])
+            self.assertEqual(obj.names, content)