diff --git a/blueman/main/Sendto.py b/blueman/main/Sendto.py index d23ea80f9..b6bb39b01 100644 --- a/blueman/main/Sendto.py +++ b/blueman/main/Sendto.py @@ -1,6 +1,7 @@ from gettext import gettext as _ import atexit import os +import sys import time import logging from argparse import Namespace @@ -44,15 +45,21 @@ def __init__(self, parsed_args: Namespace) -> None: self.device: Device | None = None self._manager = manager = Manager() - self._manager.connect_signal("adapter-added", self.__on_manager_signal, "adapter-added") - self._manager.connect_signal("adapter-removed", self.__on_manager_signal, "adapter-removed") - self._manager.connect_signal("device-created", self.__on_manager_signal, "device-added") - self._manager.connect_signal("device-removed", self.__on_manager_signal, "device-removed") + self._setup_signal_handlers(self._manager, { + "adapter-added": (self.__on_manager_signal, "adapter-added"), + "adapter-removed": (self.__on_manager_signal, "adapter-removed"), + "device-created": (self.__on_manager_signal, "device-added"), + "device-removed": (self.__on_manager_signal, "device-removed"), + }) self.__any_adapter = AnyAdapter() - self.__any_adapter.connect_signal("property-changed", self.__on_adapter_property_changed) + self._setup_signal_handlers(self.__any_adapter, { + "property-changed": (self.__on_adapter_property_changed,), + }) self.__any_device = AnyDevice() - self.__any_device.connect_signal("property-changed", self.__on_device_property_changed) + self._setup_signal_handlers(self.__any_device, { + "property-changed": (self.__on_device_property_changed,), + }) adapter: Adapter | None = None adapters = manager.get_adapters() @@ -60,12 +67,22 @@ def __init__(self, parsed_args: Namespace) -> None: if len(adapters) == 0: logging.error("Error: No Adapters present") + dialog = ErrorDialog( + _("No Bluetooth adapters found"), + _("Connect or enable a Bluetooth adapter and try sending the file(s) again."), + icon_name="blueman") + dialog.run() + dialog.destroy() bmexit() if parsed_args.source is not None: try: adapter = manager.get_adapter(parsed_args.source) except DBusNoSuchAdapterError: + # Tell the CLI user their -s/--source choice was not found and + # is being ignored, instead of only logging to the console. + print(f"Unknown adapter {parsed_args.source!r}, falling back to the first available adapter", + file=sys.stderr) logging.error("Unknown adapter, trying first available") if adapter is None: @@ -106,6 +123,14 @@ def delete_files() -> None: self.do_send() self.__cleanup() + @staticmethod + def _setup_signal_handlers(source: GObject.Object, handlers: dict[str, tuple[Any, ...]]) -> None: + """Connect each signal to its (callback, *args), replacing repeated + connect_signal boilerplate. connect_signal is an alias of + GObject.connect, so the connection semantics are unchanged.""" + for signal_name, (callback, *args) in handlers.items(): + source.connect(signal_name, callback, *args) + def __on_manager_signal(self, _manager: Manager, object_path: ObjectPath, signal_name: str) -> None: logging.debug(f"{object_path} {signal_name}") match signal_name: @@ -137,10 +162,7 @@ def __on_device_property_changed(self, _: AnyDevice, key: str, value: Any, objec def _has_objpush(self, object_path: ObjectPath) -> bool: device = Device(obj_path=object_path) - for uuid in device["UUIDs"]: - if ServiceUUID(uuid).short_uuid == OBEX_OBJPUSH_SVCLASS_ID: - return True - return False + return any(ServiceUUID(uuid).short_uuid == OBEX_OBJPUSH_SVCLASS_ID for uuid in device["UUIDs"]) def _start_discovery(self, from_timer: bool = False) -> bool: for adapter in self._manager.get_adapters(): @@ -282,8 +304,8 @@ def __init__(self, device: Device, adapter_path: ObjectPath, files: Iterable[str self.obex_manager.connect_signal('session-added', self.on_session_added) self.obex_manager.connect_signal('session-removed', self.on_session_removed) except GLib.Error as e: - if 'StartServiceByName' in e.message: - logging.debug(e.message) + if 'StartServiceByName' in str(e): + logging.debug(str(e)) parent = self.get_toplevel() assert isinstance(parent, Gtk.Container) d = ErrorDialog(_("obexd not available"), _("Failed to autostart obex service. Make sure the obex " @@ -304,15 +326,20 @@ def __init__(self, device: Device, adapter_path: ObjectPath, files: Iterable[str logging.info(f"Sending to {device['Address']}") self.l_dest.props.label = device.display_name - # Stop discovery if discovering and let adapter settle for a second + # Stop discovery if discovering and let the adapter settle for a second + # before creating the session, without blocking the UI thread. if self.adapter["Discovering"]: self.adapter.stop_discovery() - time.sleep(1) - - self.create_session() + GLib.timeout_add_seconds(1, self._create_session_timeout) + else: + self.create_session() self.show() + def _create_session_timeout(self) -> bool: + self.create_session() + return False # one-shot timer + def create_session(self) -> None: self.client.create_session(self.device['Address'], self.adapter["Address"]) @@ -357,24 +384,28 @@ def on_transfer_progress(self, _transfer: Transfer, progress: int) -> None: self._last_bytes = progress - tm = time.time() + # Monotonic clock: a wall-clock step (NTP/manual) must not stall or spam + # the speed/ETA throttle. + tm = time.monotonic() if tm - self._last_update > 0.5: spd = self.speed.calc(self.total_transferred) (size, units) = format_bytes(spd) - try: + if spd > 0: x = ((self.total_bytes - self.total_transferred) / spd) + 1 if x > 60: x /= 60 eta = ngettext("%(minutes)d Minute", "%(minutes)d Minutes", round(x)) % {"minutes": round(x)} else: eta = ngettext("%(seconds)d Second", "%(seconds)d Seconds", round(x)) % {"seconds": round(x)} - except ZeroDivisionError: + else: + logging.debug("Speed is zero, cannot estimate ETA") eta = None self._update_pb_text(size, units, eta) self._last_update = tm - self.pb.props.fraction = float(self.total_transferred) / self.total_bytes + if self.total_bytes > 0: + self.pb.props.fraction = float(self.total_transferred) / self.total_bytes def on_transfer_completed(self, _transfer: Transfer) -> None: del self.files[-1] diff --git a/test/main/Makefile.am b/test/main/Makefile.am index b18e2673a..0d810dd1f 100644 --- a/test/main/Makefile.am +++ b/test/main/Makefile.am @@ -9,4 +9,5 @@ EXTRA_DIST = \ test_gdbus_error.py \ test_imports.py \ test_netconf.py \ - test_pulseaudio_utils.py + test_pulseaudio_utils.py \ + test_sendto.py diff --git a/test/main/test_sendto.py b/test/main/test_sendto.py new file mode 100644 index 000000000..fdde16d1d --- /dev/null +++ b/test/main/test_sendto.py @@ -0,0 +1,257 @@ +from unittest import TestCase +from unittest.mock import Mock, patch + +from blueman.main.Sendto import Sender, SendTo + + +def make_sender() -> Sender: + """Build a Sender without running its GTK/D-Bus __init__. + + Only the attributes touched by the method under test are populated; the + progress bar and speed calculator are mocks. + """ + s = Sender.__new__(Sender) + s.pb = Mock() + s.speed = Mock() + s.speed.calc.return_value = 1000.0 + s.num_files = 1 + s.files = [Mock()] + s.total_bytes = 10000 + s.total_transferred = 0 + s.transferred = 0 + s._last_bytes = 0 + s._last_update = 0.0 + return s + + +class TestOnTransferProgressClock(TestCase): + @patch("blueman.main.Sendto.time.monotonic") + def test_throttle_uses_monotonic(self, monotonic: Mock) -> None: + s = make_sender() + # First tick at t=100: _last_update is 0.0 so the update fires. + monotonic.return_value = 100.0 + s.on_transfer_progress(None, 500) + self.assertEqual(s._last_update, 100.0) + self.assertEqual(s.speed.calc.call_count, 1) + + # 0.2s later: below the 0.5s threshold, no further speed/ETA update. + monotonic.return_value = 100.2 + s.on_transfer_progress(None, 800) + self.assertEqual(s.speed.calc.call_count, 1) + self.assertEqual(s._last_update, 100.0) + + # 0.6s after the last update: fires again. + monotonic.return_value = 100.6 + s.on_transfer_progress(None, 1200) + self.assertEqual(s.speed.calc.call_count, 2) + self.assertEqual(s._last_update, 100.6) + + @patch("blueman.main.Sendto.time.monotonic", return_value=5.0) + def test_progress_accumulates_and_sets_fraction(self, _monotonic: Mock) -> None: + s = make_sender() + s.on_transfer_progress(None, 2500) + self.assertEqual(s.total_transferred, 2500) + self.assertEqual(s._last_bytes, 2500) + self.assertAlmostEqual(s.pb.props.fraction, 0.25) # 2500 / 10000 + + +def make_sendto() -> SendTo: + s = SendTo.__new__(SendTo) + s._device_selector = Mock() + s._manager = Mock() + return s + + +class TestManagerSignalDispatch(TestCase): + @patch("blueman.main.Sendto.GLib.timeout_add_seconds") + def test_adapter_added(self, timeout: Mock) -> None: + s = make_sendto() + s._SendTo__on_manager_signal(None, "/org/bluez/hci0", "adapter-added") + s._device_selector.add_adapter.assert_called_once_with("/org/bluez/hci0") + timeout.assert_called_once() + + def test_adapter_removed(self) -> None: + s = make_sendto() + s._SendTo__on_manager_signal(None, "/org/bluez/hci0", "adapter-removed") + s._device_selector.remove_adapter.assert_called_once_with("/org/bluez/hci0") + + def test_device_added_sets_warning(self) -> None: + s = make_sendto() + s._has_objpush = Mock(return_value=False) # type: ignore[method-assign] + s._SendTo__on_manager_signal(None, "/dev/x", "device-added") + s._device_selector.add_device.assert_called_once_with("/dev/x", True) + + def test_device_removed(self) -> None: + s = make_sendto() + s._SendTo__on_manager_signal(None, "/dev/x", "device-removed") + s._device_selector.remove_device.assert_called_once_with("/dev/x") + + def test_unknown_signal_raises(self) -> None: + s = make_sendto() + with self.assertRaises(ValueError): + s._SendTo__on_manager_signal(None, "/dev/x", "bogus") + + +class TestPropertyChangedDispatch(TestCase): + def test_adapter_discovering(self) -> None: + s = make_sendto() + s._SendTo__on_adapter_property_changed(None, "Discovering", True, "/org/bluez/hci0") + s._device_selector.set_discovering.assert_called_once_with(True) + + def test_adapter_other_key_ignored(self) -> None: + s = make_sendto() + s._SendTo__on_adapter_property_changed(None, "Powered", True, "/org/bluez/hci0") + s._device_selector.set_discovering.assert_not_called() + + def test_device_alias(self) -> None: + s = make_sendto() + s._SendTo__on_device_property_changed(None, "Alias", "Phone", "/dev/x") + s._device_selector.update_row.assert_called_once_with("/dev/x", "description", "Phone") + + def test_device_uuids_updates_warning(self) -> None: + s = make_sendto() + s._has_objpush = Mock(return_value=True) # type: ignore[method-assign] + s._SendTo__on_device_property_changed(None, "UUIDs", ["x"], "/dev/x") + s._device_selector.update_row.assert_called_once_with("/dev/x", "warning", False) + + +class TestStartDiscovery(TestCase): + def test_starts_only_idle_adapters_and_return_value(self) -> None: + s = make_sendto() + a_idle = Mock() + a_idle.__getitem__ = Mock(return_value=False) + a_busy = Mock() + a_busy.__getitem__ = Mock(return_value=True) + s._manager.get_adapters.return_value = [a_idle, a_busy] + + self.assertTrue(s._start_discovery()) # not from timer -> repeat + a_idle.start_discovery.assert_called_once_with() + a_busy.start_discovery.assert_not_called() + + self.assertFalse(s._start_discovery(from_timer=True)) # one-shot + + +class TestSenderQueueAndText(TestCase): + def test_update_pb_text_format(self) -> None: + s = make_sender() + s.num_files = 3 + s.files = [Mock(), Mock()] # one already sent -> num = 3-2+1 = 2 + s._update_pb_text(1.5, "KB", "5 Seconds") + text = s.pb.set_text.call_args.args[0] + self.assertIn("2/3", text) + self.assertIn("1.50", text) + self.assertIn("KB", text) + self.assertIn("5 Seconds", text) + + def test_update_pb_text_infinite_eta(self) -> None: + s = make_sender() + s.num_files = 1 + s.files = [Mock()] + s._update_pb_text(0.0, "B") + self.assertIn("∞", s.pb.set_text.call_args.args[0]) + + def test_process_queue_sends_next(self) -> None: + s = Sender.__new__(Sender) + f = Mock() + f.get_path.return_value = "/tmp/file" + s.files = [f] + s.send_file = Mock() # type: ignore[method-assign] + s.process_queue() + s.send_file.assert_called_once_with("/tmp/file") + + def test_process_queue_emits_result_when_done(self) -> None: + s = Sender.__new__(Sender) + s.files = [] + s.emit = Mock() # type: ignore[method-assign] + s.process_queue() + s.emit.assert_called_once_with("result", True) + + def test_on_transfer_completed_pops_and_continues(self) -> None: + s = Sender.__new__(Sender) + s.files = [Mock(), Mock()] + s.process_queue = Mock() # type: ignore[method-assign] + s.on_transfer_completed(None) + self.assertEqual(len(s.files), 1) + self.assertIsNone(s.transfer) + s.process_queue.assert_called_once_with() + + +class TestCreateSessionTimeout(TestCase): + def test_creates_session_once_and_stops(self) -> None: + s = Sender.__new__(Sender) + s.create_session = Mock() # type: ignore[method-assign] + result = s._create_session_timeout() + s.create_session.assert_called_once_with() + # Must return False so the GLib timeout does not repeat. + self.assertFalse(result) + + +class TestSetupSignalHandlers(TestCase): + def test_connects_each_signal_with_args(self) -> None: + source = Mock() + cb1, cb2 = Mock(), Mock() + SendTo._setup_signal_handlers(source, { + "adapter-added": (cb1, "adapter-added"), + "property-changed": (cb2,), + }) + source.connect.assert_any_call("adapter-added", cb1, "adapter-added") + source.connect.assert_any_call("property-changed", cb2) + self.assertEqual(source.connect.call_count, 2) + + def test_empty_handlers_no_calls(self) -> None: + source = Mock() + SendTo._setup_signal_handlers(source, {}) + source.connect.assert_not_called() + + +OBJPUSH_UUID = "00001105-0000-1000-8000-00805f9b34fb" +GATT_UUID = "00001801-0000-1000-8000-00805f9b34fb" + + +class TestHasObjpush(TestCase): + @patch("blueman.main.Sendto.Device") + def test_true_when_objpush_present(self, device_cls: Mock) -> None: + device_cls.return_value = {"UUIDs": [GATT_UUID, OBJPUSH_UUID]} + s = SendTo.__new__(SendTo) + self.assertTrue(s._has_objpush("/org/bluez/hci0/dev_AA")) + + @patch("blueman.main.Sendto.Device") + def test_false_when_absent(self, device_cls: Mock) -> None: + device_cls.return_value = {"UUIDs": [GATT_UUID]} + s = SendTo.__new__(SendTo) + self.assertFalse(s._has_objpush("/org/bluez/hci0/dev_AA")) + + @patch("blueman.main.Sendto.Device") + def test_false_when_empty(self, device_cls: Mock) -> None: + device_cls.return_value = {"UUIDs": []} + s = SendTo.__new__(SendTo) + self.assertFalse(s._has_objpush("/org/bluez/hci0/dev_AA")) + + +class TestOnTransferProgressGuards(TestCase): + @patch("blueman.main.Sendto.time.monotonic", return_value=1.0) + def test_zero_speed_no_zero_division(self, _monotonic: Mock) -> None: + s = make_sender() + s.speed.calc.return_value = 0.0 + # ETA cannot be computed; must not raise, must log, must still render. + with self.assertLogs(level="DEBUG"): + s.on_transfer_progress(None, 100) + self.assertTrue(s.pb.set_text.called) + + @patch("blueman.main.Sendto.time.monotonic", return_value=1.0) + def test_zero_total_bytes_no_zero_division(self, _monotonic: Mock) -> None: + s = make_sender() + s.total_bytes = 0 + s.speed.calc.return_value = 0.0 + # Must not raise ZeroDivisionError on the fraction update. + s.on_transfer_progress(None, 0) + # fraction left untouched (never divided by zero). + + @patch("blueman.main.Sendto.time.monotonic", return_value=1.0) + def test_positive_speed_computes_eta(self, _monotonic: Mock) -> None: + s = make_sender() + s.total_bytes = 100000 + s.speed.calc.return_value = 1000.0 + s.on_transfer_progress(None, 1000) + # ETA computed, progress bar text updated. + self.assertTrue(s.pb.set_text.called)