Merge pull request #1564 from astrofrog/merge-prompt

Avoid prompting user multiple times for merging after drag-and-drop

CHANGES.md

@@ -9,6 +9,9 @@ v0.12.4 (2017-01-09)
   resulted in all axes being returned as dependent axes even though this
   isn't necessary. [#1552]
 
+* Avoid prompting users multiple times to merge data when dragging
+  and dropping multiple data files onto glue. [#1564]
+
 * Improve error message in PV slicer when _slice_index fails. [#1536]
 
 * Fixed a bug that caused an error when trying to save a session that

glue/app/qt/application.py

@@ -29,7 +29,7 @@
 from glue.viewers.scatter.qt import ScatterViewer
 from glue.viewers.image.qt import ImageViewer
 from glue.utils import nonpartial, defer_draw
-from glue.utils.qt import (pick_class, GlueTabBar,
+from glue.utils.qt import (pick_class, GlueTabBar, qurl_to_path,
                            set_cursor_cm, messagebox_on_error, load_ui)
 
 from glue.app.qt.feedback import submit_bug_report, submit_feedback
@@ -1047,25 +1047,23 @@ def dragEnterEvent(self, event):
         else:
             event.ignore()
 
+    @messagebox_on_error("Failed to load files")
     def dropEvent(self, event):
 
         urls = event.mimeData().urls()
 
-        for url in urls:
+        paths = [qurl_to_path(url) for url in urls]
 
-            # Get path to file
-            path = url.path()
-
-            # Workaround for a Qt bug that causes paths to start with a /
-            # on Windows: https://bugreports.qt.io/browse/QTBUG-46417
-            if sys.platform.startswith('win'):
-                if path.startswith('/') and path[2] == ':':
-                    path = path[1:]
-
-            if path.endswith('.glu'):
-                self.restore_session_and_close(path)
+        if any(path.endswith('.glu') for path in paths):
+            if len(paths) != 1:
+                raise Exception("When dragging and dropping files onto glue, "
+                                "only a single .glu session file can be "
+                                "dropped at a time, or multiple datasets, but "
+                                "not a mix of both.")
             else:
-                self.load_data(path)
+                self.restore_session_and_close(paths[0])
+        else:
+            self.load_data(paths)
 
         event.accept()
 

glue/app/qt/tests/test_application.py

@@ -5,6 +5,8 @@
 import os
 import sys
 
+import pytest
+from qtpy import QtWidgets
 import numpy as np
 from mock import patch, MagicMock
 
@@ -169,14 +171,49 @@ def test_new_data_defaults(self):
             assert kwargs['default'] is ImageViewer
 
     def test_drop_load_data(self):
+
+        load_data = MagicMock()
+        load_session = MagicMock()
+        self.app.load_data = load_data
+        self.app.restore_session_and_close = load_session
+
+        e = MagicMock()
+
         m = QtCore.QMimeData()
         m.setUrls([QtCore.QUrl('test.fits')])
-        e = MagicMock()
         e.mimeData.return_value = m
-        load = MagicMock()
-        self.app.load_data = load
+
+        self.app.dropEvent(e)
+        assert load_data.called_once_with('test.fits')
+        assert load_session.call_count == 0
+
+        load_data.reset_mock()
+
+        m = QtCore.QMimeData()
+        m.setUrls([QtCore.QUrl('test1.fits'), QtCore.QUrl('test2.fits')])
+        e.mimeData.return_value = m
+        self.app.dropEvent(e)
+        assert load_data.called_once_with(['test1.fits', 'test2.fits'])
+        assert load_session.call_count == 0
+
+        load_data.reset_mock()
+
+        m = QtCore.QMimeData()
+        m.setUrls([QtCore.QUrl('test.glu')])
+        e.mimeData.return_value = m
         self.app.dropEvent(e)
-        assert load.call_count == 1
+        assert load_data.call_count == 0
+        assert load_session.called_once_with(['test.glu'])
+
+        load_data.reset_mock()
+
+        m = QtCore.QMimeData()
+        m.setUrls([QtCore.QUrl('test.glu'), QtCore.QUrl('test.fits')])
+        e.mimeData.return_value = m
+        with patch('qtpy.QtWidgets.QMessageBox') as mb:
+            self.app.dropEvent(e)
+            assert mb.call_count == 1
+            assert "When dragging and dropping files" in mb.call_args[0][2]
 
     def test_subset_facet(self):
         # regression test for 335

glue/core/application_base.py

@@ -3,6 +3,7 @@
 import traceback
 from functools import wraps
 
+from glue.external.six import string_types
 from glue.core.session import Session
 from glue.core.edit_subset_mode import EditSubsetMode
 from glue.core.hub import HubListener
@@ -159,16 +160,32 @@ def settings(self):
             yield key, value
 
     @catch_error("Could not load data")
-    def load_data(self, path):
+    def load_data(self, paths, skip_merge=False, auto_merge=False):
         """
         Given a path to a file, load the file as a Data object and add it to
         the current session.
 
         This returns the added `Data` object.
         """
-        d = load_data(path)
-        self.add_datasets(self.data_collection, d)
-        return d
+
+        if isinstance(paths, string_types):
+            paths = [paths]
+
+        datasets = []
+        for path in paths:
+            result = load_data(path)
+            if isinstance(result, Data):
+                datasets.append(result)
+            else:
+                datasets.extend(result)
+
+        self.add_datasets(self.data_collection, datasets,
+                          skip_merge=skip_merge, auto_merge=auto_merge)
+
+        if len(datasets) == 1:
+            return datasets[0]
+        else:
+            return datasets
 
     @catch_error("Could not add data")
     def add_data(self, *args, **kwargs):
@@ -212,7 +229,7 @@ def report_error(self, message, detail):
         detail : str
             Longer context about the error
         """
-        raise NotImplementedError()
+        raise Exception(message + "(" + detail + ")")
 
     def do(self, command):
         return self._cmds.do(command)
@@ -233,7 +250,7 @@ def _update_undo_redo_enabled(self):
         raise NotImplementedError()
 
     @classmethod
-    def add_datasets(cls, data_collection, datasets, auto_merge=False):
+    def add_datasets(cls, data_collection, datasets, skip_merge=False, auto_merge=False):
         """ Utility method to interactively add datasets to a
         data_collection
 
@@ -266,7 +283,7 @@ def add_datasets(cls, data_collection, datasets, auto_merge=False):
                      if d.shape == shp and d is not data]
 
             # If no other datasets have the same shape, we go to the next one
-            if not other:
+            if not other or skip_merge:
                 continue
 
             if auto_merge:

glue/core/tests/test_application_base.py

@@ -1,5 +1,6 @@
 from __future__ import absolute_import, division, print_function
 
+import os
 from mock import MagicMock
 
 from .. import Data
@@ -111,3 +112,37 @@ def test_nested_data():
     assert a in app.data_collection
     assert b in app.data_collection
     assert c in app.data_collection
+
+
+def test_load_data(tmpdir):
+
+    os.chdir(tmpdir.strpath)
+
+    with open('data1.csv', 'w') as f:
+        f.write('a, b\n1, 2\n3, 4\n')
+
+    with open('data2.csv', 'w') as f:
+        f.write('a, b\n1, 2\n3, 4\n')
+
+    app1 = Application()
+    data = app1.load_data('data1.csv')
+
+    assert len(app1.data_collection) == 1
+    assert isinstance(data, Data)
+
+    app2 = Application()
+    datasets = app2.load_data(['data1.csv', 'data2.csv'], skip_merge=True)
+
+    assert len(app2.data_collection) == 2
+    assert len(datasets) == 2
+    assert isinstance(datasets[0], Data)
+    assert isinstance(datasets[1], Data)
+
+    app3 = Application()
+    data = app3.load_data(['data1.csv', 'data2.csv'], auto_merge=True)
+
+    assert len(app3.data_collection) == 1
+    # NOTE: for now this still returns the individual datasets
+    assert len(datasets) == 2
+    assert isinstance(datasets[0], Data)
+    assert isinstance(datasets[1], Data)

glue/utils/qt/helpers.py

@@ -1,14 +1,16 @@
 from __future__ import absolute_import, division, print_function
 
 import os
+import sys
 from contextlib import contextmanager
 
 from qtpy import QtCore, QtWidgets
 from qtpy.QtCore import Qt
 from qtpy.uic import loadUi
 from glue.utils.qt import get_text
 
-__all__ = ['update_combobox', 'GlueTabBar', 'load_ui', 'process_dialog', 'combo_as_string']
+__all__ = ['update_combobox', 'GlueTabBar', 'load_ui', 'process_dialog',
+           'combo_as_string', 'qurl_to_path']
 
 
 def update_combobox(combo, labeldata, default_index=0):
@@ -208,3 +210,20 @@ def combo_as_string(combo):
     """
     items = [combo.itemText(i) for i in range(combo.count())]
     return ":".join(items)
+
+
+def qurl_to_path(url):
+    """
+    Convert a local QUrl to a normal path
+    """
+
+    # Get path to file
+    path = url.path()
+
+    # Workaround for a Qt bug that causes paths to start with a /
+    # on Windows: https://bugreports.qt.io/browse/QTBUG-46417
+    if sys.platform.startswith('win'):
+        if path.startswith('/') and path[2] == ':':
+            path = path[1:]
+
+    return path