diff --git a/CHANGES.md b/CHANGES.md index e5e47ed0c..f7aca9400 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -78,6 +78,9 @@ v0.12.4 (unreleased) 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 diff --git a/glue/app/qt/application.py b/glue/app/qt/application.py index e029fb5bb..5facf4eb1 100644 --- a/glue/app/qt/application.py +++ b/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 @@ -1129,25 +1129,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() diff --git a/glue/app/qt/tests/test_application.py b/glue/app/qt/tests/test_application.py index 0ecaebc62..49e9aa2b8 100644 --- a/glue/app/qt/tests/test_application.py +++ b/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 diff --git a/glue/core/application_base.py b/glue/core/application_base.py index e82203cfb..b0a4e38f4 100644 --- a/glue/core/application_base.py +++ b/glue/core/application_base.py @@ -4,8 +4,8 @@ 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 from glue.core import Data, Subset from glue.core import command @@ -186,16 +186,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): @@ -239,7 +255,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) @@ -260,7 +276,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 @@ -293,7 +309,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: diff --git a/glue/core/tests/test_application_base.py b/glue/core/tests/test_application_base.py index ca66262b7..ccc07ed19 100644 --- a/glue/core/tests/test_application_base.py +++ b/glue/core/tests/test_application_base.py @@ -146,3 +146,37 @@ def test_session_paths(tmpdir): assert os.path.isabs(value['path']) is absolute MockApplication.restore_session(session_file) + + +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) diff --git a/glue/utils/qt/helpers.py b/glue/utils/qt/helpers.py index 15815e9a2..df37128fb 100644 --- a/glue/utils/qt/helpers.py +++ b/glue/utils/qt/helpers.py @@ -1,6 +1,7 @@ from __future__ import absolute_import, division, print_function import os +import sys from contextlib import contextmanager from qtpy import QtCore, QtWidgets @@ -8,7 +9,8 @@ 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