Fix crashes, logic errors, and lifecycle bugs

build.py

@@ -96,7 +96,7 @@ def release():
 					settings_path, next_version + snapshot_suffix,
 					'Bump version for next development iteration'
 				)
-				git('push', '-u', 'origin', 'master')
+				git('push', '-u', 'origin', 'main')
 				try:
 					git('push', 'origin', release_tag)
 					try:
@@ -106,7 +106,7 @@ def release():
 							'    git pull\n'
 							'    git checkout %s\n'
 							'    python build.py release\n'
-							'    git checkout master\n\n'
+							'    git checkout main\n\n'
 							'on the other OSs now, then come back here and do:'
 							'\n\n'
 							'    python build.py post_release\n'
@@ -117,7 +117,7 @@ def release():
 						raise
 				except:
 					git('revert', '--no-edit', revision_before + '..HEAD' )
-					git('push', '-u', 'origin', 'master')
+					git('push', '-u', 'origin', 'main')
 					revision_before = git('rev-parse', 'HEAD').rstrip()
 					raise
 			except:
@@ -149,7 +149,7 @@ def post_release():
 	create_cloudfront_invalidation(cloudfront_items_to_invalidate)
 	record_release_on_server()
 	upload_core_to_github()
-	git('checkout', 'master')
+	git('checkout', 'main')
 
 def _prompt_for_next_version(release_version):
 	next_version = _get_suggested_next_version(release_version)

src/integrationtest/python/fman_integrationtest/plugin_tests/test_directory_pane_listener.py

@@ -1,20 +1,24 @@
+from fman.impl.plugins.plugin import ListenerWrapper
 from fman_integrationtest.plugin_tests import PluginTest
 
 class DirectoryPaneListenerTest(PluginTest):
+	def _broadcast_and_wait(self, *args):
+		self._left_pane._broadcast(*args)
+		ListenerWrapper._POOL.submit(lambda: None).result()
 	def test_on_path_changed_error(self):
-		self._left_pane._broadcast('on_path_changed')
+		self._broadcast_and_wait('on_path_changed')
 		self.assertEqual(
 			["DirectoryPaneListener 'ListenerRaisingError' raised error."],
 			self._error_handler.error_messages
 		)
 	def test_on_doubleclicked_error(self):
-		self._left_pane._broadcast('on_doubleclicked', self._settings_plugin)
+		self._broadcast_and_wait('on_doubleclicked', self._settings_plugin)
 		self.assertEqual(
 			["DirectoryPaneListener 'ListenerRaisingError' raised error."],
 			self._error_handler.error_messages
 		)
 	def test_on_name_edited_error(self):
-		self._left_pane._broadcast(
+		self._broadcast_and_wait(
 			'on_name_edited', self._settings_plugin, 'New name'
 		)
 		self.assertEqual(

src/main/python/fman/__init__.py

@@ -41,6 +41,8 @@
 	DATA_DIRECTORY = expanduser('~/Library/Application Support/fman')
 elif PLATFORM == 'Linux':
 	DATA_DIRECTORY = expanduser('~/.config/fman')
+else:
+	raise NotImplementedError('Unsupported platform: %s' % PLATFORM)
 
 class ApplicationCommand:
 	def __init__(self, window):

src/main/python/fman/fs.py

@@ -146,7 +146,7 @@ def prepare_trash(self, path):
 			raise self._operation_not_implemented()
 		return [Task(
 			'Deleting ' + path.rsplit('/', 1)[-1],
-			fn=self.delete, args=(path,), size=1
+			fn=self.move_to_trash, args=(path,), size=1
 		)]
 	def touch(self, path):
 		raise self._operation_not_implemented()

src/main/python/fman/impl/application_context.py

@@ -120,8 +120,13 @@ def _load():
 		Thread(target=_load, daemon=True).start()
 	def on_main_window_close(self):
 		self.session_manager.on_close(self.main_window)
+		for pane in self.main_window.get_panes():
+			pane._model.shutdown()
 	def on_quit(self):
+		from fman.impl.plugins.plugin import ListenerWrapper
+		ListenerWrapper.shutdown_pool(wait=False)
 		self.config.on_quit()
+		self.metrics.shutdown()
 		if self.metrics_logging_enabled:
 			log_dir = dirname(self._get_metrics_json_path())
 			log_file_path = join(log_dir, 'Metrics.log')
@@ -431,29 +436,11 @@ def init_logging(self):
 		logging.basicConfig(level=logging.CRITICAL)
 	def on_main_window_shown(self):
 		if PLATFORM == 'Linux':
-			"""
-			PyInstaller sets LD_LIBRARY_PATH to /opt/fman. Processes we spawn,
-			be it via Popen(...) or QDesktopServices.openUrl(...), inherit this
-			value. This leads to problems, especially when the app we launch is
-			based on Qt. The reason is that the OS then attempts to load our 
-			libraries, which are most likely incompatible with those of the app.
-			An example where this happens is VLC, which errors out with 'This 
-			application failed to start because it could not find or load the Qt
-			platform plugin "xcb"'. Plugin developers have also encountered this
-			unexpected behaviour when trying to launch apps.
-
-			To fix the problem, we restore LD_LIBRARY_PATH to its original value
-			here. According to the docs [1], PyInstaller stores this value in a
-			separate environment variable.
-
-			A drawback of unsetting the environment variable here is that
-			libraries from PyInstaller's search path cannot be loaded after this
-			method was called. In other words, we assume that all required
-			libraries have been loaded once we reach here. This assumption may
-			turn out to be wrong in the future.
-
-			[1]: http://pyinstaller.readthedocs.io/en/stable/runtime-information.html#ld-library-path-libpath-considerations
-			"""
+			# PyInstaller sets LD_LIBRARY_PATH to /opt/fman. Processes we spawn
+			# inherit this value, causing problems when the launched app is
+			# Qt-based (it tries to load our incompatible libraries).
+			# We restore LD_LIBRARY_PATH to its original value here.
+			# See: https://pyinstaller.org/en/stable/runtime-information.html
 			lp_orig = os.environ.pop('LD_LIBRARY_PATH_ORIG', None)
 			if lp_orig is not None:
 				os.environ['LD_LIBRARY_PATH'] = lp_orig

src/main/python/fman/impl/model/__init__.py

@@ -20,6 +20,8 @@ class SortedFileSystemModel(QSortFilterProxyModel):
 	sort_order_changed = pyqtSignal(int, int)
 	transaction_ended = pyqtSignal()
 
+	_MAX_VISITED = 512
+
 	def __init__(self, parent, fs, null_location):
 		super().__init__(parent)
 		self._fs = fs
@@ -105,7 +107,9 @@ def _set_location_main(
 		)
 		self.setSourceModel(new_model)
 		self._connect_signals(new_model)
-		self._already_visited.add(url)
+		if len(self._already_visited) > self._MAX_VISITED:
+			self._already_visited.clear()
+			self._already_visited.add(url)
 		self.location_changed.emit(url)
 		order = Qt.AscendingOrder if ascending else Qt.DescendingOrder
 		self.sort_order_changed.emit(sort_col_index, order)
@@ -146,21 +150,19 @@ def url(self, index):
 	def find(self, url):
 		return self.mapFromSource(self.sourceModel().find(url))
 	def _on_file_removed(self, url):
+		if not self.sourceModel():
+			return
 		if is_pardir(url, self.get_location()):
-			dir_ = dirname(url)
-			if dir_ == url:
-				self.set_location(self._null_location)
-			else:
+			while True:
+				dir_ = dirname(url)
+				if dir_ == url:
+					self.set_location(self._null_location)
+					return
 				try:
 					self.set_location(dir_)
+					return
 				except OSError:
-					# In a perfect world, would like to only handle
-					# FileNotFoundError here. But there can of course also be
-					# other reasons. For example, when on a network share on
-					# Windows, we may get a PermissionError trying to list a
-					# parent directory we don't have access to. So catch all
-					# OSErrors and in the worst case go to null://.
-					self._on_file_removed(dir_)
+					url = dir_
 	def _connect_signals(self, model):
 		# Would prefer signal.connect(self.signal.emit) here. But PyQt doesn't
 		# support it. So we need Python wrappers "_emit_...":
@@ -189,5 +191,11 @@ def _emit_sort_order_changed(self, column, order):
 		self.sort_order_changed.emit(column, order)
 	def _emit_transaction_ended(self):
 		self.transaction_ended.emit()
+	def shutdown(self):
+		self._fs.file_removed.remove_callback(self._on_file_removed)
+		model = self.sourceModel()
+		if model:
+			self._disconnect_signals(model)
+			model.shutdown()
 	def __str__(self):
 		return '<%s: %s>' % (self.__class__.__name__, self.get_location())

src/main/python/fman/impl/model/drag_and_drop.py

@@ -42,7 +42,7 @@ def mimeTypes(self):
 	def mimeData(self, indexes):
 		result = QMimeData()
 		result.setUrls([as_qurl(self.url(index)) for index in indexes])
-		# The Qt documentation (http://doc.qt.io/qt-5/dnd.html) states that the
+		# The Qt documentation (https://doc.qt.io/archives/qt-5.15/dnd.html) states that the
 		# QMimeData should not be deleted, because the target of the drag and
 		# drop operation takes ownership of it. We must therefore tell SIP not
 		# to garbage-collect `result` once this method returns. Without this
@@ -55,7 +55,14 @@ def dropMimeData(self, data, action, row, column, parent):
 			return True
 		if not data.hasUrls():
 			return False
-		urls = [from_qurl(qurl) for qurl in data.urls()]
+		urls = []
+		for qurl in data.urls():
+			try:
+				urls.append(from_qurl(qurl))
+			except ValueError:
+				continue
+		if not urls:
+			return False
 		dest = self._get_drop_dest(parent)
 		if action in (MoveAction, CopyAction):
 			self.files_dropped.emit(urls, dest, action == CopyAction)

src/main/python/fman/impl/model/table.py

@@ -151,7 +151,7 @@ def insert(self, rows, first_rownum):
 		new_keys = {row.key: first_rownum + i for i, row in enumerate(rows)}
 		with self._lock:
 			# Perform this check here, once we have the lock:
-			if first_rownum < 0 or first_rownum > len(self._rows) + 1:
+			if first_rownum < 0 or first_rownum > len(self._rows):
 				raise ValueError('Invalid first_rownum: %d' % first_rownum)
 			num_rows = len(rows)
 			for row in self._rows[first_rownum:]:

src/main/python/fman/impl/model/worker.py

@@ -21,7 +21,7 @@ def submit(self, priority, fn, *args, **kwargs):
 		with self._shutdown_lock:
 			if self._shutdown:
 				return
-			self._queue.put(WorkItem(priority, fn, *args, *kwargs))
+			self._queue.put(WorkItem(priority, fn, *args, **kwargs))
 	def shutdown(self):
 		with self._shutdown_lock:
 			self._shutdown = True
@@ -56,7 +56,7 @@ def __lt__(self, other):
 			return NotImplemented
 	def __eq__(self, other):
 		try:
-			return self._fn, self._args, self._kwargs, self._priority == \
-				   other._fn, other._args, other._kwargs, other._priority
+			return (self._fn, self._args, self._kwargs, self._priority) == \
+				   (other._fn, other._args, other._kwargs, other._priority)
 		except AttributeError:
 			return NotImplemented

src/main/python/fman/impl/onboarding/__init__.py

@@ -159,4 +159,6 @@ def _get_body_html(self):
 			line = re.subn(r'\*([^*]+)\*', highlight(r'\1'), line)[0]
 			line = re.subn(r'_([^_]+)_', underline(r'\1'), line)[0]
 			result += line
+		if is_list:
+			result += '</ul>'
 		return result

src/main/python/fman/impl/plugins/command_registry.py

@@ -3,8 +3,11 @@
 from threading import Thread, get_ident
 from weakref import WeakKeyDictionary
 
+import logging
 import re
 
+_LOG = logging.getLogger(__name__)
+
 class CommandRegistry:
 	def __init__(self, main_thread_id=None):
 		if main_thread_id is None:
@@ -61,11 +64,10 @@ def _execute_command(self, command, args):
 		class_name = command.__class__.__name__
 		self._callback.before_command(class_name)
 		msg_on_err = 'Command %r raised error.' % class_name
-		try:
-			with ReportExceptions(self._error_handler, msg_on_err):
-				command(**args)
-		except Exception:
-			pass
+		with ReportExceptions(self._error_handler, msg_on_err) as cm:
+			command(**args)
+		if cm.exception:
+			_LOG.debug('Command %s failed', class_name, exc_info=True)
 		else:
 			self._callback.after_command(class_name)
 
@@ -123,11 +125,10 @@ def _execute_command(self, command, args, pane, file_under_cursor):
 		self._callback.before_command(class_name)
 		with self._set_context(pane, file_under_cursor):
 			msg_on_err = 'Command %r raised error.' % class_name
-			try:
-				with ReportExceptions(self._error_handler, msg_on_err):
-					command(**args)
-			except Exception:
-				pass
+			with ReportExceptions(self._error_handler, msg_on_err) as cm:
+				command(**args)
+			if cm.exception:
+				_LOG.debug('Command %s failed', class_name, exc_info=True)
 			else:
 				self._callback.after_command(class_name)
 	def _get_command(self, pane, name):
@@ -153,9 +154,11 @@ def _set_context(self, pane, file_under_cursor=_DEFAULT):
 		if file_under_cursor is not self._DEFAULT:
 			cm = pane._override_file_under_cursor(file_under_cursor)
 			cm.__enter__()
-		yield
-		if file_under_cursor is not self._DEFAULT:
-			cm.__exit__(None, None, None)
+		try:
+			yield
+		finally:
+			if file_under_cursor is not self._DEFAULT:
+				cm.__exit__(None, None, None)
 
 def _get_default_aliases(cmd_class):
 	return re.sub(r'([a-z])([A-Z])', r'\1 \2', cmd_class.__name__)\

src/main/python/fman/impl/plugins/error.py

@@ -46,8 +46,8 @@ def handle_system_exit(self, code=0):
 		self._app.exit(code)
 	def on_main_window_shown(self, main_window):
 		self._main_window = main_window
-		if self._pending_error_messages:
-			self._main_window.show_alert(self._pending_error_messages[0])
+		for message in self._pending_error_messages:
+			self._main_window.show_alert(message)
 	def _get_plugin_traceback(self, exc):
 		if isinstance(exc, ThemeError):
 			return exc.description

src/main/python/fman/impl/plugins/key_bindings.py

@@ -62,7 +62,7 @@ def sanitize_key_bindings(bindings, available_commands):
 					'Error: A key binding\'s "keys" must be a list ["..."], '
 					'not %s.' % describe_type(keys)
 				)
-			if not keys:
+			elif not keys:
 				this_binding_errors.append(
 					'Error: A key binding\'s "keys" must be a non-empty list '
 					'["..."], not [].'