diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9d8dff8..b7ff905 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,8 +30,11 @@ jobs: - name: Install locked dependencies run: uv sync --locked --extra dev + # The whole repository, not just src/tests/scripts. The narrower form + # left examples/ and the four root-level helper scripts outside every + # gate -- 15 findings sat there unreported while CI showed a green lint. - name: Ruff lint - run: uv run ruff check src tests scripts + run: uv run ruff check . - name: Build distributions run: uv build diff --git a/.github/workflows/secret-scan.yml b/.github/workflows/secret-scan.yml index 41ad94f..1ec69c4 100644 --- a/.github/workflows/secret-scan.yml +++ b/.github/workflows/secret-scan.yml @@ -25,7 +25,10 @@ jobs: env: GITLEAKS_VERSION: 8.30.1 run: | - curl -sSL "https://github.com/gitleaks/gitleaks/releases/download/v${GITLEAKS_VERSION}/gitleaks_${GITLEAKS_VERSION}_linux_x64.tar.gz" \ + # pipefail: without it the pipeline reports tar's status, so a failed + # download would only be caught incidentally by the extract. + set -euo pipefail + curl -sSL --fail "https://github.com/gitleaks/gitleaks/releases/download/v${GITLEAKS_VERSION}/gitleaks_${GITLEAKS_VERSION}_linux_x64.tar.gz" \ | tar -xz -C /usr/local/bin gitleaks gitleaks version - name: Scan git history for secrets diff --git a/examples/notification_examples.py b/examples/notification_examples.py old mode 100644 new mode 100755 diff --git a/generate_screenshots.py b/generate_screenshots.py old mode 100644 new mode 100755 index 11516fb..6f98786 --- a/generate_screenshots.py +++ b/generate_screenshots.py @@ -25,8 +25,8 @@ 10. Settings/configuration (if available) """ -import time import sys +import time from pathlib import Path # Try to import the notification system @@ -187,18 +187,24 @@ def main(): print() # Countdown before action - countdown(3, f" Triggering") + countdown(3, " Triggering") - # Execute action + # Execute action. The notification helpers return whether the + # notification was actually delivered; printing a tick regardless told + # the operator to photograph a notification that was never shown. + shown = True if callable(shot['action']): - result = shot['action']() + shown = shot['action']() is not False # Wait for notification to appear and give time to screenshot - print(f" ✓ Triggered! Capture screenshot now...") + if shown: + print(" ✓ Triggered! Capture screenshot now...") + else: + print(" ✗ NOT delivered — there is nothing on screen to capture.") time.sleep(shot['wait']) # Give extra time to capture - countdown(shot['wait'] + 3, f" Time remaining") + countdown(shot['wait'] + 3, " Time remaining") print() print("-" * 80) diff --git a/rename_screenshots.py b/rename_screenshots.py index 9748e6f..5e13279 100755 --- a/rename_screenshots.py +++ b/rename_screenshots.py @@ -5,26 +5,25 @@ Usage: Just take screenshots with Cmd+Shift+4, then run this script. """ -import os import shutil +from datetime import datetime, timedelta, timezone from pathlib import Path -from datetime import datetime, timedelta def find_recent_screenshots(minutes=30): """Find screenshots from Desktop taken in last N minutes.""" desktop = Path.home() / "Desktop" - cutoff = datetime.now() - timedelta(minutes=minutes) + cutoff = datetime.now(tz=timezone.utc) - timedelta(minutes=minutes) screenshots = [] for file in desktop.glob("Screen Shot *.png"): - mtime = datetime.fromtimestamp(file.stat().st_mtime) + mtime = datetime.fromtimestamp(file.stat().st_mtime, tz=timezone.utc) if mtime > cutoff: screenshots.append(file) # Also check for Screenshot (macOS Ventura+) for file in desktop.glob("Screenshot *.png"): - mtime = datetime.fromtimestamp(file.stat().st_mtime) + mtime = datetime.fromtimestamp(file.stat().st_mtime, tz=timezone.utc) if mtime > cutoff: screenshots.append(file) @@ -50,11 +49,13 @@ def rename_and_move_screenshots(screenshots, target_dir): print(f"\nFound {len(screenshots)} recent screenshots:") for i, screenshot in enumerate(screenshots, 1): - mtime = datetime.fromtimestamp(screenshot.stat().st_mtime) + mtime = datetime.fromtimestamp( + screenshot.stat().st_mtime, tz=timezone.utc + ).astimezone() time_str = mtime.strftime("%H:%M:%S") print(f" {i}. {screenshot.name} (taken at {time_str})") - print(f"\nWill rename them to:") + print("\nWill rename them to:") for i, name in enumerate(names[:len(screenshots)], 1): print(f" {i}. {name}") diff --git a/src/openadapt_tray/app.py b/src/openadapt_tray/app.py index c3e9f19..ff6a9c8 100644 --- a/src/openadapt_tray/app.py +++ b/src/openadapt_tray/app.py @@ -16,13 +16,14 @@ import pystray -from openadapt_tray.config import TrayConfig +from openadapt_tray.config import ConfigLoadError, TrayConfig from openadapt_tray.hosted import CountResult, HostedPoller, route_break_click from openadapt_tray.icons import IconManager from openadapt_tray.ipc import IPCClient, IPCMessageType from openadapt_tray.menu import MenuBuilder from openadapt_tray.notifications import NotificationManager from openadapt_tray.platform import get_platform_handler +from openadapt_tray.platform.base import DialogUnavailableError from openadapt_tray.shortcuts import HotkeyManager from openadapt_tray.state import ( LANE_BYOC, @@ -46,7 +47,15 @@ def __init__(self, config: TrayConfig | None = None): Args: config: Optional configuration. If None, loads from file or defaults. """ - self.config = config or TrayConfig.load() + # An unreadable tray.json must not quietly become "default settings" -- + # the default lane is "cloud", and a byoc install silently moved onto + # the hosted route is exactly the kind of wrong-but-confident state the + # tray exists to expose. Keep the error and tell the user in run(). + self._config_error: ConfigLoadError | None = None + if config is not None: + self.config = config + else: + self.config, self._config_error = TrayConfig.load_or_defaults() self.state = StateManager() self.platform = get_platform_handler() @@ -254,12 +263,24 @@ def start_recording(self, name: str | None = None) -> None: # Prompt for name if not provided if name is None and self.config.use_native_dialogs: - name = self.platform.prompt_input( - "New Recording", - "Enter a name for this capture:", - ) - if not name: - return # User cancelled + try: + name = self.platform.prompt_input( + "New Recording", + "Enter a name for this capture:", + ) + except DialogUnavailableError as e: + # The user asked to record and the prompt never appeared. This + # used to look identical to "the user cancelled", so the click + # did nothing and said nothing. Honour the click with a default + # name and say why there was no prompt. + print(f"Could not show the capture-name dialog: {e}") + self.notifications.show( + "No naming dialog available", + "Recording started with a default name.", + ) + else: + if not name: + return # User cancelled # Use default name if still not set if not name: @@ -327,12 +348,24 @@ def open_cloud_dashboard(self) -> None: """Open the hosted cloud dashboard in the system browser.""" webbrowser.open(self.config.hosted_url) - def open_needs_attention(self) -> None: - """Route a needs-attention click by deployment lane (§3c).""" - route_break_click( + def open_needs_attention(self) -> bool: + """Route a needs-attention click by deployment lane (§3c). + + Returns: + True if something opened. When nothing opened the user is told, + rather than being left in front of an unchanged screen. + """ + routed = route_break_click( self.config, ipc_client=self.ipc if self.ipc.is_connected() else None, ) + if not routed: + self.notifications.show( + "Could not open needs-attention", + "Neither the desktop app nor a browser could be opened. " + f"Open {self.config.hosted_url.rstrip('/')}/dashboard manually.", + ) + return routed def login(self) -> None: """Start the hosted login flow. @@ -356,6 +389,17 @@ def resume_sync(self) -> None: if self.ipc.is_connected(): self.ipc.send_resume_sync() + def _report_config_error(self) -> None: + """Tell the user when the tray is running on defaults it did not choose.""" + if self._config_error is None: + return + self.notifications.show( + "Settings could not be read", + f"{self._config_error} — running on default settings " + f"(lane: {self.config.deployment_lane}).", + urgency="critical", + ) + # --- hosted poller callbacks -------------------------------------------- def _on_hosted_count(self, result: CountResult) -> None: @@ -457,6 +501,9 @@ def _apply_sync_state(self, name: str | None) -> None: def run(self) -> None: """Run the application.""" + # Say it out loud before anything relies on the settings. + self._report_config_error() + # Start hotkey listener self.hotkeys.start() diff --git a/src/openadapt_tray/config.py b/src/openadapt_tray/config.py index 4989157..c66e362 100644 --- a/src/openadapt_tray/config.py +++ b/src/openadapt_tray/config.py @@ -15,6 +15,29 @@ OFFLINE_POLL_INTERVAL_S = 300 +class ConfigLoadError(Exception): + """``tray.json`` exists but could not be read. + + Distinct from "no config file", which legitimately means defaults. + """ + + def __init__(self, path: Path, cause: Exception): + """Record which file failed and why. + + Args: + path: The configuration file that could not be read. + cause: The underlying parse/IO error. + """ + super().__init__(f"Could not read {path}: {cause}") + self.path = path + self.cause = cause + + @property + def defaults(self) -> "TrayConfig": + """The default configuration, for a caller that chooses to continue.""" + return TrayConfig() + + @dataclass class TrayConfig: """Tray application configuration. @@ -76,15 +99,46 @@ def config_path(cls) -> Path: @classmethod def load(cls) -> "TrayConfig": - """Load configuration from file.""" + """Load configuration from file. + + A file that is not there means "this user has no saved preferences", + and defaults are the right answer. A file that IS there but cannot be + read is a different thing entirely, and defaults are the WRONG answer: + ``deployment_lane`` defaults to ``"cloud"``, so silently substituting + defaults would move a ``byoc`` install -- where the fix must stay local + -- onto the hosted route without telling anybody. + + Returns: + The saved configuration, or defaults when no file exists. + + Raises: + ConfigLoadError: The file exists but could not be read or parsed. + It carries ``defaults`` so a caller that chooses to continue + does so knowingly. + """ path = cls.config_path() - if path.exists(): - try: - data = json.loads(path.read_text()) - return cls._from_dict(data) - except Exception as e: - print(f"Warning: Could not load config: {e}") - return cls() + if not path.exists(): + return cls() + try: + data = json.loads(path.read_text()) + return cls._from_dict(data) + except Exception as e: + raise ConfigLoadError(path, e) from e + + @classmethod + def load_or_defaults(cls) -> tuple["TrayConfig", "ConfigLoadError | None"]: + """Load the configuration, reporting an unreadable file instead of hiding it. + + Returns: + ``(config, error)``. ``error`` is ``None`` on a clean load. When it + is set, ``config`` is the default configuration AND the caller is + obliged to surface the error -- that is the whole point of handing + both back instead of one silently-defaulted object. + """ + try: + return cls.load(), None + except ConfigLoadError as e: + return e.defaults, e @classmethod def _from_dict(cls, data: dict[str, Any]) -> "TrayConfig": diff --git a/src/openadapt_tray/hosted.py b/src/openadapt_tray/hosted.py index 88e3c12..99f7ef4 100644 --- a/src/openadapt_tray/hosted.py +++ b/src/openadapt_tray/hosted.py @@ -32,6 +32,29 @@ REQUEST_TIMEOUT_S = 10.0 +class InvalidCountPayload(ValueError): + """The count endpoint returned a body we cannot read as a count. + + Raised instead of defaulting, because an unreadable body is NOT a count of + zero: zero renders as "nothing needs attention", which is the one answer we + must never invent. + """ + + +def _optional_int(payload: dict, key: str) -> int: + """Read an optional integer subfield. + + Absent means zero (documented tolerance for the display-only subfields). + Present but unreadable means the body is malformed, which is an error. + """ + if key not in payload: + return 0 + try: + return int(payload[key]) + except (TypeError, ValueError) as e: + raise InvalidCountPayload(f"{key!r} is not an integer: {payload[key]!r}") from e + + @dataclass class CountResult: """Parsed response from the needs-attention count endpoint.""" @@ -41,12 +64,47 @@ class CountResult: uncertain_dispatches: int = 0 @classmethod - def from_payload(cls, payload: dict) -> "CountResult": - """Build a result from the JSON body, tolerating missing subfields.""" + def from_payload(cls, payload: object) -> "CountResult": + """Build a result from the JSON body. + + ``count`` is the safety-critical number: it drives the badge and the + "N automations need attention" notification. A body without a readable + integer ``count`` means we do NOT know the count, so this raises + :class:`InvalidCountPayload` rather than substituting ``0`` -- an + absent field used to render as a confident all-clear. + + The display-only subfields (``halts``, ``uncertain_dispatches``) stay + tolerant of absence; see :func:`_optional_int`. + + Args: + payload: The decoded JSON body. + + Returns: + A parsed :class:`CountResult`. + + Raises: + InvalidCountPayload: The body is not a JSON object, has no + ``count``, or carries a count that is not a non-negative + integer. + """ + if not isinstance(payload, dict): + raise InvalidCountPayload( + f"expected a JSON object, got {type(payload).__name__}" + ) + if "count" not in payload: + raise InvalidCountPayload("response body has no 'count' field") + try: + count = int(payload["count"]) + except (TypeError, ValueError) as e: + raise InvalidCountPayload( + f"'count' is not an integer: {payload['count']!r}" + ) from e + if count < 0: + raise InvalidCountPayload(f"'count' is negative: {count}") return cls( - count=int(payload.get("count", 0)), - halts=int(payload.get("halts", 0)), - uncertain_dispatches=int(payload.get("uncertain_dispatches", 0)), + count=count, + halts=_optional_int(payload, "halts"), + uncertain_dispatches=_optional_int(payload, "uncertain_dispatches"), ) @@ -122,7 +180,14 @@ def poll_once(self) -> CountResult | None: if resp.status_code != 200: print(f"needs-attention count returned {resp.status_code}") return None - return CountResult.from_payload(resp.json()) + try: + return CountResult.from_payload(resp.json()) + except InvalidCountPayload as e: + # A body we cannot read is not a count of zero. Report it as a + # failed poll so the badge keeps its last known value instead of + # clearing to a confident all-clear. + print(f"needs-attention count response unusable: {e}") + return None except Exception as e: # Network error / DNS / timeout / bad JSON → offline. print(f"needs-attention poll failed: {e}") @@ -151,17 +216,32 @@ def _handle_result(self, result: CountResult | None) -> None: # Notify only when the count RISES (0→N or N→N+1), never on a decrease. if result.count > self._last_count and result.count > 0: - self._fire_notification(result.count) + if self._fire_notification(result.count): + self._last_count = result.count + # Not delivered: leave ``_last_count`` behind on purpose. Advancing + # it would record the user as informed about breaks they were never + # shown, and the count would have to rise AGAIN before we tried a + # second time. Holding it back makes the next poll retry. + else: + self._last_count = result.count + + def _fire_notification(self, count: int) -> bool: + """Fire the 'N automations need attention' notification. - self._last_count = result.count + Args: + count: The number of automations needing attention. - def _fire_notification(self, count: int) -> None: - """Fire the 'N automations need attention' notification.""" + Returns: + True only if the notifier reported the notification as delivered. + A notifier that cannot deliver (no toast backend, a dead + notification daemon) returns False from its ``show``; that answer is + now propagated instead of discarded. + """ if not self._notifier: - return + return False noun = "automation" if count == 1 else "automations" try: - self._notifier.show( + delivered = self._notifier.show( "Automations need attention", f"{count} {noun} need attention", urgency="critical", @@ -169,6 +249,14 @@ def _fire_notification(self, count: int) -> None: ) except Exception as e: print(f"Failed to show needs-attention notification: {e}") + return False + if not delivered: + print( + f"needs-attention notification was NOT delivered ({count} {noun} " + "need attention); retrying on the next poll" + ) + return False + return True def _default_click(self) -> None: """Fallback click handler (cloud-lane browser open).""" @@ -208,12 +296,18 @@ def current_interval(self) -> int: def route_break_click( config: TrayConfig, ipc_client: object | None = None, -) -> None: +) -> bool: """Route a break/needs-attention click by deployment lane. Args: config: Tray configuration (provides lane + hosted_url). ipc_client: Optional IPC client used for the byoc local-teach route. + + Returns: + True if the click was routed somewhere the user can see. False means + neither route worked and NOTHING opened -- the user clicked and the + screen did not change, so the caller must say so rather than assume the + click landed. """ # PHI stays local on the byoc lane: open the desktop teach view over IPC, # and fall through to the hosted dashboard only if the desktop is @@ -221,7 +315,12 @@ def route_break_click( if config.deployment_lane == "byoc" and ipc_client is not None: try: ipc_client.send_open_teach() - return + return True except Exception as e: print(f"Failed to route byoc break click to desktop: {e}") - webbrowser.open(f"{config.hosted_url.rstrip('/')}/dashboard") + # webbrowser.open returns False when it could not find or start a browser. + # Discarding that made a dead click indistinguishable from a served one. + opened = webbrowser.open(f"{config.hosted_url.rstrip('/')}/dashboard") + if not opened: + print("Could not open the hosted dashboard: no usable browser") + return bool(opened) diff --git a/src/openadapt_tray/menu.py b/src/openadapt_tray/menu.py index dd55d23..34defc7 100644 --- a/src/openadapt_tray/menu.py +++ b/src/openadapt_tray/menu.py @@ -13,6 +13,7 @@ if TYPE_CHECKING: from openadapt_tray.app import TrayApplication +from openadapt_tray.platform.base import DialogUnavailableError from openadapt_tray.state import TrayState @@ -132,7 +133,16 @@ def _build_captures_submenu(self) -> Item: Returns: Menu item with captures submenu. """ - captures = self._get_recent_captures() + try: + captures = self._get_recent_captures() + except OSError as e: + # "We could not look" must not render as "there is nothing there". + # An unreadable captures directory now says so in the menu. + print(f"Could not read the captures directory: {e}") + return Item( + "Recent Captures", + Menu(Item("Could not read captures", None, enabled=False)), + ) if not captures: return Item( @@ -160,41 +170,45 @@ def _build_captures_submenu(self) -> Item: def _get_recent_captures(self) -> list[CaptureInfo]: """Get list of recent captures. + An empty list means the directory was read and held no captures. It + never means the directory could not be read -- that raises, so the + caller can render the two outcomes differently. + Returns: - List of CaptureInfo objects. + List of CaptureInfo objects, newest first (at most 10). + + Raises: + OSError: The captures directory exists but could not be listed or + stat'ed (permissions, a dead mount, a broken symlink). """ - try: - captures_dir = self.app.config.get_captures_path() - if not captures_dir.exists(): - return [] - - captures = [] - for d in sorted( - captures_dir.iterdir(), - key=lambda x: x.stat().st_mtime, - reverse=True, - ): - if d.is_dir(): - # Check for metadata.json (formal capture) - # or just any directory (simpler check) - # Displayed to the user, so render in LOCAL time -- - # parse the epoch as UTC and convert, rather than - # relying on an implicit naive-local conversion. - mtime = datetime.fromtimestamp( - d.stat().st_mtime, tz=timezone.utc - ).astimezone() - captures.append( - CaptureInfo( - name=d.name, - path=str(d), - timestamp=mtime.strftime("%Y-%m-%d %H:%M"), - ) + captures_dir = self.app.config.get_captures_path() + if not captures_dir.exists(): + return [] + + captures = [] + for d in sorted( + captures_dir.iterdir(), + key=lambda x: x.stat().st_mtime, + reverse=True, + ): + if d.is_dir(): + # Check for metadata.json (formal capture) + # or just any directory (simpler check) + # Displayed to the user, so render in LOCAL time -- + # parse the epoch as UTC and convert, rather than + # relying on an implicit naive-local conversion. + mtime = datetime.fromtimestamp( + d.stat().st_mtime, tz=timezone.utc + ).astimezone() + captures.append( + CaptureInfo( + name=d.name, + path=str(d), + timestamp=mtime.strftime("%Y-%m-%d %H:%M"), ) + ) - return captures[:10] # Limit to 10 most recent - except Exception as e: - print(f"Error getting captures: {e}") - return [] + return captures[:10] # Limit to 10 most recent def _open_desktop_app(self) -> None: """Open (or focus) the local desktop app cockpit.""" @@ -223,14 +237,26 @@ def _view_capture(self, path: str) -> None: path: Path to capture directory. """ try: - # Try using openadapt CLI first - subprocess.run( + # Try using openadapt CLI first. + result = subprocess.run( ["openadapt", "visualize", path], - check=False, + check=False, # returncode is inspected directly below capture_output=True, ) except FileNotFoundError: - # Fallback: open in file browser + # No CLI at all — fall back to the file browser. + self._open_in_file_browser(path) + return + + # A CLI that exists but failed used to be indistinguishable from one + # that worked: the exit status was never read, so "View" did nothing at + # all and said nothing about it. Fall back the same way a missing CLI + # does. + if result.returncode != 0: + print( + f"openadapt visualize failed (exit {result.returncode}): " + f"{result.stderr}" + ) self._open_in_file_browser(path) def _delete_capture(self, path: str, name: str) -> None: @@ -240,10 +266,24 @@ def _delete_capture(self, path: str, name: str) -> None: path: Path to capture directory. name: Capture name for display. """ - if self.app.platform.confirm_dialog( - "Delete Capture", - f"Are you sure you want to delete this capture?\n\n{name}", - ): + try: + confirmed = self.app.platform.confirm_dialog( + "Delete Capture", + f"Are you sure you want to delete this capture?\n\n{name}", + ) + except DialogUnavailableError as e: + # Not asking is not the same as being told no. Deleting nothing is + # still the right action here, but the user gets told why their + # click did nothing instead of watching it vanish. + print(f"Could not show the delete confirmation: {e}") + self.app.notifications.show( + "Could not confirm deletion", + f"No confirmation dialog could be shown, so '{name}' was NOT " + "deleted.", + ) + return + + if confirmed: try: shutil.rmtree(path) self.app.notifications.show( diff --git a/src/openadapt_tray/platform/base.py b/src/openadapt_tray/platform/base.py index 7a0bf6f..ede26fb 100644 --- a/src/openadapt_tray/platform/base.py +++ b/src/openadapt_tray/platform/base.py @@ -7,6 +7,16 @@ from openadapt_tray.config import TrayConfig +class DialogUnavailableError(RuntimeError): + """No dialog could be shown to the user at all. + + Distinct from a dialog the user answered. "The user declined" and "we never + managed to ask" used to be the same ``False``, and "the user cancelled" and + "the prompt never appeared" used to be the same ``None`` -- so a click on + Delete or Start Recording could do nothing at all, silently, forever. + """ + + class PlatformHandler(ABC): """Abstract base class for platform-specific functionality.""" @@ -26,7 +36,12 @@ def prompt_input(self, title: str, message: str) -> str | None: message: Prompt message. Returns: - User input string, or None if cancelled. + User input string, or None if the user cancelled. ``None`` means + the user was asked and declined to answer -- never that we failed + to ask. + + Raises: + DialogUnavailableError: No dialog mechanism could be shown. """ @abstractmethod @@ -38,7 +53,11 @@ def confirm_dialog(self, title: str, message: str) -> bool: message: Confirmation message. Returns: - True if user confirmed, False otherwise. + True if the user confirmed, False if the user declined. Both + answers mean the user was actually asked. + + Raises: + DialogUnavailableError: No dialog mechanism could be shown. """ @abstractmethod diff --git a/src/openadapt_tray/platform/linux.py b/src/openadapt_tray/platform/linux.py index f9f3429..8b2f19e 100644 --- a/src/openadapt_tray/platform/linux.py +++ b/src/openadapt_tray/platform/linux.py @@ -6,11 +6,16 @@ from pathlib import Path from typing import TYPE_CHECKING -from openadapt_tray.platform.base import PlatformHandler +from openadapt_tray.platform.base import DialogUnavailableError, PlatformHandler if TYPE_CHECKING: from openadapt_tray.config import TrayConfig +# zenity and kdialog both use exit 1 for "the user said no / cancelled". Any +# other non-zero exit (zenity uses 255) is the TOOL failing -- no display, a +# broken GTK, a timeout -- which must not be read as a user's answer. +DIALOG_DECLINED_EXIT = 1 + class LinuxHandler(PlatformHandler): """Linux-specific functionality.""" @@ -27,7 +32,11 @@ def prompt_input(self, title: str, message: str) -> str | None: message: Prompt message. Returns: - User input string, or None if cancelled. + User input string, or None if the user cancelled. + + Raises: + DialogUnavailableError: zenity, kdialog and tkinter all failed to + show a dialog. """ # Try zenity first (GNOME) try: @@ -45,7 +54,12 @@ def prompt_input(self, title: str, message: str) -> str | None: ) if result.returncode == 0: return result.stdout.strip() - return None + if result.returncode == DIALOG_DECLINED_EXIT: + return None # The user cancelled. That is an answer. + # zenity ran but could not show anything (no display, exit 255). + # This used to return None, i.e. it was reported as the user + # cancelling, and it also skipped the kdialog/tkinter fallbacks. + print(f"zenity could not show a dialog (exit {result.returncode})") except FileNotFoundError: pass except Exception as e: @@ -68,7 +82,9 @@ def prompt_input(self, title: str, message: str) -> str | None: ) if result.returncode == 0: return result.stdout.strip() - return None + if result.returncode == DIALOG_DECLINED_EXIT: + return None # The user cancelled. + print(f"kdialog could not show a dialog (exit {result.returncode})") except FileNotFoundError: pass except Exception as e: @@ -85,8 +101,9 @@ def prompt_input(self, title: str, message: str) -> str | None: root.destroy() return result except Exception as e: - print(f"Error with tkinter: {e}") - return None + raise DialogUnavailableError( + f"no input dialog available (zenity, kdialog, tkinter): {e}" + ) from e def confirm_dialog(self, title: str, message: str) -> bool: """Show confirmation dialog using zenity or kdialog. @@ -96,7 +113,11 @@ def confirm_dialog(self, title: str, message: str) -> bool: message: Confirmation message. Returns: - True if user clicked OK. + True if the user clicked OK, False if the user declined. + + Raises: + DialogUnavailableError: zenity, kdialog and tkinter all failed to + show a dialog, so the user was never asked. """ # Try zenity first try: @@ -111,7 +132,11 @@ def confirm_dialog(self, title: str, message: str) -> bool: timeout=60, check=False, # returncode is inspected directly below ) - return result.returncode == 0 + if result.returncode in (0, DIALOG_DECLINED_EXIT): + return result.returncode == 0 + # zenity ran but showed nothing. Reporting that as "the user said + # no" is a guess dressed up as an answer. + print(f"zenity could not show a dialog (exit {result.returncode})") except FileNotFoundError: pass except Exception as e: @@ -131,7 +156,9 @@ def confirm_dialog(self, title: str, message: str) -> bool: timeout=60, check=False, # returncode is inspected directly below ) - return result.returncode == 0 + if result.returncode in (0, DIALOG_DECLINED_EXIT): + return result.returncode == 0 + print(f"kdialog could not show a dialog (exit {result.returncode})") except FileNotFoundError: pass except Exception as e: @@ -146,10 +173,11 @@ def confirm_dialog(self, title: str, message: str) -> bool: root.withdraw() result = messagebox.askokcancel(title, message) root.destroy() - return result + return bool(result) except Exception as e: - print(f"Error with tkinter: {e}") - return False + raise DialogUnavailableError( + f"no confirmation dialog available (zenity, kdialog, tkinter): {e}" + ) from e def open_settings_dialog(self, config: "TrayConfig") -> None: """Open settings in default browser. diff --git a/src/openadapt_tray/platform/macos.py b/src/openadapt_tray/platform/macos.py index 3d976c7..e1831f5 100644 --- a/src/openadapt_tray/platform/macos.py +++ b/src/openadapt_tray/platform/macos.py @@ -5,11 +5,37 @@ from pathlib import Path from typing import TYPE_CHECKING -from openadapt_tray.platform.base import PlatformHandler +from openadapt_tray.platform.base import DialogUnavailableError, PlatformHandler if TYPE_CHECKING: from openadapt_tray.config import TrayConfig +# osascript exits 1 both when the user cancels a dialog AND when the script +# itself fails (no Apple-events permission, no window server). The two are only +# distinguishable by stderr, which carries this marker on a real failure. +OSASCRIPT_ERROR_MARKER = "execution error" + + +def _raise_if_osascript_failed( + result: subprocess.CompletedProcess, kind: str +) -> None: + """Raise when a non-zero osascript exit was a failure, not a user decision. + + Args: + result: The finished ``osascript`` process. + kind: Dialog kind, used in the error message. + + Raises: + DialogUnavailableError: stderr carries an AppleScript execution error, + which means no dialog was shown -- as opposed to the user clicking + Cancel, which exits non-zero with a quiet stderr. + """ + stderr = result.stderr or "" + if OSASCRIPT_ERROR_MARKER in stderr.lower(): + raise DialogUnavailableError( + f"osascript could not show the {kind} dialog: {stderr.strip()}" + ) + class MacOSHandler(PlatformHandler): """macOS-specific functionality.""" @@ -34,7 +60,12 @@ def prompt_input(self, title: str, message: str) -> str | None: message: Prompt message. Returns: - User input string, or None if cancelled. + User input string, or None if the user cancelled (or did not answer + within the timeout). + + Raises: + DialogUnavailableError: osascript is missing or reported an + execution error, so no dialog ever reached the screen. """ # Escape special characters for AppleScript title_escaped = title.replace('"', '\\"').replace("\\", "\\\\") @@ -54,13 +85,16 @@ def prompt_input(self, title: str, message: str) -> str | None: timeout=60, # 1 minute timeout for user input check=False, # returncode is inspected directly below ) - if result.returncode == 0: - return result.stdout.strip() except subprocess.TimeoutExpired: - pass + # The dialog WAS shown; the user just did not answer it. + return None except Exception as e: - print(f"Error showing input dialog: {e}") - return None + raise DialogUnavailableError(f"could not run osascript: {e}") from e + + if result.returncode == 0: + return result.stdout.strip() + _raise_if_osascript_failed(result, "input") + return None # The user cancelled. def confirm_dialog(self, title: str, message: str) -> bool: """Show native macOS confirmation dialog. @@ -70,7 +104,12 @@ def confirm_dialog(self, title: str, message: str) -> bool: message: Confirmation message. Returns: - True if user clicked OK. + True if the user clicked OK, False if the user declined (or did not + answer within the timeout). + + Raises: + DialogUnavailableError: osascript is missing or reported an + execution error, so the user was never asked. """ # Escape special characters for AppleScript title_escaped = title.replace('"', '\\"').replace("\\", "\\\\") @@ -90,12 +129,16 @@ def confirm_dialog(self, title: str, message: str) -> bool: timeout=60, check=False, # returncode is inspected directly below ) - return result.returncode == 0 and "OK" in result.stdout except subprocess.TimeoutExpired: + # The dialog WAS shown; the user just did not answer it. return False except Exception as e: - print(f"Error showing confirm dialog: {e}") - return False + raise DialogUnavailableError(f"could not run osascript: {e}") from e + + if result.returncode == 0: + return "OK" in result.stdout + _raise_if_osascript_failed(result, "confirmation") + return False # The user clicked Cancel. def open_settings_dialog(self, config: "TrayConfig") -> None: """Open settings in default browser. diff --git a/src/openadapt_tray/platform/windows.py b/src/openadapt_tray/platform/windows.py index 74af57b..552a345 100644 --- a/src/openadapt_tray/platform/windows.py +++ b/src/openadapt_tray/platform/windows.py @@ -3,7 +3,7 @@ import webbrowser from typing import TYPE_CHECKING -from openadapt_tray.platform.base import PlatformHandler +from openadapt_tray.platform.base import DialogUnavailableError, PlatformHandler if TYPE_CHECKING: from openadapt_tray.config import TrayConfig @@ -24,7 +24,12 @@ def prompt_input(self, title: str, message: str) -> str | None: message: Prompt message. Returns: - User input string, or None if cancelled. + User input string, or None if the user cancelled. + + Raises: + DialogUnavailableError: tkinter could not show a dialog, so the + user was never prompted. This used to return ``None``, which + the caller reads as "the user cancelled". """ try: import tkinter as tk @@ -38,8 +43,7 @@ def prompt_input(self, title: str, message: str) -> str | None: root.destroy() return result except Exception as e: - print(f"Error showing input dialog: {e}") - return None + raise DialogUnavailableError(f"no input dialog available: {e}") from e def confirm_dialog(self, title: str, message: str) -> bool: """Show Windows confirmation dialog. @@ -49,7 +53,13 @@ def confirm_dialog(self, title: str, message: str) -> bool: message: Confirmation message. Returns: - True if user clicked OK. + True if the user clicked OK, False if the user declined. + + Raises: + DialogUnavailableError: Neither MessageBoxW nor tkinter could show + a dialog. This used to return ``False``, which the caller reads + as "the user said no" -- so a destructive action would appear + to be declined by a user who was never asked. """ try: import ctypes @@ -61,6 +71,9 @@ def confirm_dialog(self, title: str, message: str) -> bool: result = ctypes.windll.user32.MessageBoxW( 0, message, title, MB_OKCANCEL | MB_ICONQUESTION ) + # MessageBoxW returns 0 when it could not create the box at all. + if result == 0: + raise OSError("MessageBoxW returned 0 (dialog not created)") return result == IDOK except Exception as e: print(f"Error showing confirm dialog: {e}") @@ -73,9 +86,12 @@ def confirm_dialog(self, title: str, message: str) -> bool: root.withdraw() result = messagebox.askokcancel(title, message) root.destroy() - return result - except Exception: - return False + return bool(result) + except Exception as fallback_error: + raise DialogUnavailableError( + "no confirmation dialog available " + f"(MessageBoxW: {e}; tkinter: {fallback_error})" + ) from fallback_error def open_settings_dialog(self, config: "TrayConfig") -> None: """Open settings in default browser. diff --git a/test_notification_simple.py b/test_notification_simple.py old mode 100644 new mode 100755 diff --git a/tests/test_app.py b/tests/test_app.py index aa25bb0..5dd4f58 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -1,8 +1,10 @@ """Tests for the main TrayApplication.""" +from pathlib import Path from unittest.mock import MagicMock, patch -from openadapt_tray.config import TrayConfig +from openadapt_tray.config import ConfigLoadError, TrayConfig +from openadapt_tray.platform.base import DialogUnavailableError from openadapt_tray.state import TrayState @@ -239,3 +241,131 @@ def test_quit_stops_recording_if_active(self, mock_platform, mock_pystray): with patch.object(app, "stop_recording") as mock_stop: app.quit() mock_stop.assert_called_once() + + +class TestUnreadableConfigIsReported: + """Running on settings the user never chose must not look like a clean start.""" + + @patch("openadapt_tray.app.pystray") + @patch("openadapt_tray.app.get_platform_handler") + def test_run_tells_the_user_the_settings_could_not_be_read( + self, mock_platform, mock_pystray + ): + from openadapt_tray.app import TrayApplication + + mock_platform.return_value = MagicMock() + mock_pystray.Icon.return_value = MagicMock() + + error = ConfigLoadError(Path("/tmp/tray.json"), ValueError("bad json")) + with patch.object( + TrayConfig, "load_or_defaults", return_value=(TrayConfig(), error) + ): + app = TrayApplication() + app.notifications = MagicMock() + app.hotkeys = MagicMock() + app.ipc = MagicMock() + app.hosted = MagicMock() + + app.run() + + app.notifications.show.assert_called_once() + title = app.notifications.show.call_args[0][0] + assert "settings" in title.lower() + + @patch("openadapt_tray.app.pystray") + @patch("openadapt_tray.app.get_platform_handler") + def test_a_clean_load_says_nothing(self, mock_platform, mock_pystray): + from openadapt_tray.app import TrayApplication + + mock_platform.return_value = MagicMock() + mock_pystray.Icon.return_value = MagicMock() + + with patch.object( + TrayConfig, "load_or_defaults", return_value=(TrayConfig(), None) + ): + app = TrayApplication() + app.notifications = MagicMock() + app.hotkeys = MagicMock() + app.ipc = MagicMock() + app.hosted = MagicMock() + + app.run() + + app.notifications.show.assert_not_called() + + +class TestStartRecordingWhenNoDialogCanBeShown: + """"The prompt never appeared" used to be indistinguishable from "cancelled".""" + + def _app(self, mock_platform, mock_pystray): + from openadapt_tray.app import TrayApplication + + mock_pystray.Icon.return_value = MagicMock() + app = TrayApplication(config=TrayConfig(use_native_dialogs=True)) + app.notifications = MagicMock() + return app + + @patch("openadapt_tray.app.pystray") + @patch("openadapt_tray.app.get_platform_handler") + def test_unavailable_dialog_still_starts_the_recording( + self, mock_platform, mock_pystray + ): + platform = MagicMock() + platform.prompt_input.side_effect = DialogUnavailableError("no display") + mock_platform.return_value = platform + + app = self._app(mock_platform, mock_pystray) + with patch.object(app, "_dispatch_start_recording"): + app.start_recording() + + assert app.state.current.state == TrayState.RECORDING_STARTING + # And the user is told why no naming prompt appeared. + app.notifications.show.assert_called_once() + + @patch("openadapt_tray.app.pystray") + @patch("openadapt_tray.app.get_platform_handler") + def test_a_real_cancel_still_cancels(self, mock_platform, mock_pystray): + platform = MagicMock() + platform.prompt_input.return_value = None # the user pressed Cancel + mock_platform.return_value = platform + + app = self._app(mock_platform, mock_pystray) + with patch.object(app, "_dispatch_start_recording"): + app.start_recording() + + assert app.state.current.state == TrayState.IDLE + app.notifications.show.assert_not_called() + + +class TestNeedsAttentionClickReportsDeadEnds: + @patch("openadapt_tray.app.pystray") + @patch("openadapt_tray.app.get_platform_handler") + def test_click_that_opened_nothing_tells_the_user( + self, mock_platform, mock_pystray + ): + from openadapt_tray.app import TrayApplication + + mock_platform.return_value = MagicMock() + mock_pystray.Icon.return_value = MagicMock() + app = TrayApplication(config=TrayConfig()) + app.notifications = MagicMock() + + with patch("openadapt_tray.app.route_break_click", return_value=False): + assert app.open_needs_attention() is False + app.notifications.show.assert_called_once() + + @patch("openadapt_tray.app.pystray") + @patch("openadapt_tray.app.get_platform_handler") + def test_click_that_opened_something_stays_quiet( + self, mock_platform, mock_pystray + ): + from openadapt_tray.app import TrayApplication + + mock_platform.return_value = MagicMock() + mock_pystray.Icon.return_value = MagicMock() + app = TrayApplication(config=TrayConfig()) + app.notifications = MagicMock() + + with patch("openadapt_tray.app.route_break_click", return_value=True): + assert app.open_needs_attention() is True + app.notifications.show.assert_not_called() diff --git a/tests/test_config.py b/tests/test_config.py index 80b47b1..bd650d9 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,9 +1,12 @@ """Tests for configuration management.""" +import json from pathlib import Path from unittest.mock import patch -from openadapt_tray.config import TrayConfig +import pytest + +from openadapt_tray.config import ConfigLoadError, TrayConfig from openadapt_tray.shortcuts import HotkeyConfig @@ -173,12 +176,53 @@ def test_load_missing_file_returns_defaults(self, tmp_path): assert config.dashboard_port == 8080 assert config.show_notifications is True - def test_load_invalid_json_returns_defaults(self, tmp_path): - """Test that loading invalid JSON returns defaults.""" + def test_load_invalid_json_raises(self, tmp_path): + """An unreadable config file must NOT be reported as "no preferences". + + This used to return defaults with only a printed warning. The default + ``deployment_lane`` is ``"cloud"``, so a byoc install whose tray.json + went bad was silently moved onto the hosted break-click route -- a + failure rendered as a perfectly ordinary successful startup. + """ + config_file = tmp_path / "tray.json" + config_file.write_text("invalid json {{{") + + with patch.object( + TrayConfig, "config_path", return_value=config_file + ), pytest.raises(ConfigLoadError) as excinfo: + TrayConfig.load() + + assert excinfo.value.path == config_file + + def test_load_or_defaults_reports_the_error_alongside_the_defaults( + self, tmp_path + ): + """``load_or_defaults`` hands back BOTH, so the caller cannot miss it.""" config_file = tmp_path / "tray.json" config_file.write_text("invalid json {{{") with patch.object(TrayConfig, "config_path", return_value=config_file): - config = TrayConfig.load() + config, error = TrayConfig.load_or_defaults() - assert config.dashboard_port == 8080 + assert isinstance(error, ConfigLoadError) + assert config.dashboard_port == 8080 + + def test_load_or_defaults_has_no_error_on_a_clean_load(self, tmp_path): + config_file = tmp_path / "tray.json" + config_file.write_text(json.dumps({"dashboard_port": 9100})) + + with patch.object(TrayConfig, "config_path", return_value=config_file): + config, error = TrayConfig.load_or_defaults() + + assert error is None + assert config.dashboard_port == 9100 + + def test_load_or_defaults_has_no_error_when_no_file_exists(self, tmp_path): + """A missing file is not a failure -- defaults are the right answer.""" + config_file = tmp_path / "nonexistent" / "tray.json" + + with patch.object(TrayConfig, "config_path", return_value=config_file): + config, error = TrayConfig.load_or_defaults() + + assert error is None + assert config.dashboard_port == 8080 diff --git a/tests/test_hosted.py b/tests/test_hosted.py index b2fbb6e..51e5bc0 100644 --- a/tests/test_hosted.py +++ b/tests/test_hosted.py @@ -2,6 +2,8 @@ from unittest.mock import MagicMock, patch +import pytest + from openadapt_tray.config import ( MIN_POLL_INTERVAL_S, OFFLINE_POLL_INTERVAL_S, @@ -10,6 +12,7 @@ from openadapt_tray.hosted import ( CountResult, HostedPoller, + InvalidCountPayload, count_url, route_break_click, ) @@ -240,3 +243,144 @@ def test_byoc_falls_back_to_dashboard_when_no_desktop(self): with patch("openadapt_tray.hosted.webbrowser.open") as mock_open: route_break_click(cfg, ipc_client=None) mock_open.assert_called_once() + + +class TestUnreadableCountPayload: + """A body we cannot read is NOT a count of zero. + + ``count`` drives the badge and the "N automations need attention" + notification. ``payload.get("count", 0)`` used to turn any response that + lost or renamed the field into a confident all-clear -- the single most + dangerous value this module can invent. + """ + + def test_missing_count_raises_instead_of_reporting_zero(self): + with pytest.raises(InvalidCountPayload) as excinfo: + CountResult.from_payload({"halts": 0}) + assert "count" in str(excinfo.value) + + def test_null_count_raises(self): + with pytest.raises(InvalidCountPayload): + CountResult.from_payload({"count": None}) + + def test_non_numeric_count_raises(self): + with pytest.raises(InvalidCountPayload): + CountResult.from_payload({"count": "lots"}) + + def test_negative_count_raises(self): + with pytest.raises(InvalidCountPayload): + CountResult.from_payload({"count": -1}) + + def test_non_object_body_raises(self): + with pytest.raises(InvalidCountPayload): + CountResult.from_payload([1, 2, 3]) + + def test_unreadable_subfield_raises(self): + with pytest.raises(InvalidCountPayload): + CountResult.from_payload({"count": 1, "halts": "two"}) + + def test_poll_once_reports_failure_not_zero(self): + """The poller must return None, never ``CountResult(count=0)``.""" + cfg = make_config() + poller = HostedPoller( + cfg, on_count=lambda r: None, token_provider=lambda: "t" + ) + fake = _FakeClient(_FakeResponse(200, {"total": 7})) # no "count" + with patch("openadapt_tray.hosted.httpx.Client", return_value=fake): + assert poller.poll_once() is None + + def test_unreadable_body_never_clears_the_badge(self): + """The badge keeps its last known value rather than dropping to 0.""" + cfg = make_config() + counts = [] + poller = HostedPoller( + cfg, on_count=lambda r: counts.append(r.count), + token_provider=lambda: "t", + ) + good = _FakeClient(_FakeResponse(200, {"count": 4})) + with patch("openadapt_tray.hosted.httpx.Client", return_value=good): + poller._handle_result(poller.poll_once()) + bad = _FakeClient(_FakeResponse(200, {"total": 7})) + with patch("openadapt_tray.hosted.httpx.Client", return_value=bad): + poller._handle_result(poller.poll_once()) + # 4 was reported once; the unreadable body reported nothing at all. + assert counts == [4] + + +class TestNotificationDelivery: + """The notifier's answer about delivery must be acted on, not discarded. + + PR #29 made ``_show_windows`` return False when the PowerShell toast never + appeared. That honest answer was then thrown away here: ``_last_count`` was + advanced regardless, recording the user as informed about breaks they were + never shown, and suppressing every retry until the count rose again. + """ + + def _poller(self, notifier): + return HostedPoller( + make_config(), + on_count=lambda r: None, + notifier=notifier, + token_provider=lambda: "t", + ) + + def test_undelivered_notification_is_retried_on_the_next_poll(self): + notifier = MagicMock() + notifier.show.return_value = False # e.g. WinRT toast unavailable + poller = self._poller(notifier) + + poller._handle_result(CountResult(count=3)) + assert notifier.show.call_count == 1 + + # Same count again: the user still has not been told, so try again. + poller._handle_result(CountResult(count=3)) + assert notifier.show.call_count == 2 + + def test_delivered_notification_is_not_repeated(self): + notifier = MagicMock() + notifier.show.return_value = True + poller = self._poller(notifier) + + poller._handle_result(CountResult(count=3)) + poller._handle_result(CountResult(count=3)) + assert notifier.show.call_count == 1 + + def test_raising_notifier_is_treated_as_undelivered(self): + notifier = MagicMock() + notifier.show.side_effect = RuntimeError("no notification daemon") + poller = self._poller(notifier) + + poller._handle_result(CountResult(count=2)) + poller._handle_result(CountResult(count=2)) + assert notifier.show.call_count == 2 + + def test_fire_notification_reports_delivery(self): + notifier = MagicMock() + notifier.show.return_value = False + assert self._poller(notifier)._fire_notification(1) is False + notifier.show.return_value = True + assert self._poller(notifier)._fire_notification(1) is True + + def test_no_notifier_is_not_a_delivery(self): + poller = HostedPoller( + make_config(), on_count=lambda r: None, token_provider=lambda: "t" + ) + assert poller._fire_notification(1) is False + + +class TestBreakClickReportsFailure: + """A click that opened nothing must not report itself as routed.""" + + def test_returns_false_when_no_browser_could_be_opened(self): + cfg = make_config(deployment_lane="cloud") + with patch("openadapt_tray.hosted.webbrowser.open", return_value=False): + assert route_break_click(cfg, ipc_client=None) is False + + def test_returns_true_when_the_browser_opened(self): + cfg = make_config(deployment_lane="cloud") + with patch("openadapt_tray.hosted.webbrowser.open", return_value=True): + assert route_break_click(cfg, ipc_client=None) is True + + def test_byoc_desktop_route_returns_true(self): + cfg = make_config(deployment_lane="byoc") + assert route_break_click(cfg, ipc_client=MagicMock()) is True diff --git a/tests/test_menu.py b/tests/test_menu.py index a6a3ce0..8ed5fc0 100644 --- a/tests/test_menu.py +++ b/tests/test_menu.py @@ -1,8 +1,12 @@ """Tests for menu construction.""" -from unittest.mock import MagicMock +import subprocess +from unittest.mock import MagicMock, patch + +import pytest from openadapt_tray.menu import CaptureInfo, MenuBuilder +from openadapt_tray.platform.base import DialogUnavailableError from openadapt_tray.state import AppState, TrayState @@ -175,3 +179,103 @@ def test_quit_calls_app_quit(self): builder._quit() app.quit.assert_called_once() + + +class TestCapturesSubmenuDistinguishesEmptyFromUnreadable: + """"Nothing there" and "could not look" must not render the same. + + ``_get_recent_captures`` used to swallow every exception and return ``[]``, + so an unreadable captures directory (permissions, a dead mount) produced + the same reassuring "No captures" entry as a directory that really was + empty. + """ + + def _app_with_captures_dir(self, captures_dir): + app = MagicMock() + app.config.get_captures_path.return_value = captures_dir + return app + + def test_empty_directory_says_no_captures(self, tmp_path): + builder = MenuBuilder(self._app_with_captures_dir(tmp_path)) + item = builder._build_captures_submenu() + labels = [str(i.text) for i in item.submenu.items] + assert "No captures" in labels + + def test_unreadable_directory_says_so(self): + captures_dir = MagicMock() + captures_dir.exists.return_value = True + captures_dir.iterdir.side_effect = PermissionError("permission denied") + builder = MenuBuilder(self._app_with_captures_dir(captures_dir)) + + item = builder._build_captures_submenu() + + labels = [str(i.text) for i in item.submenu.items] + assert "Could not read captures" in labels + assert "No captures" not in labels + + def test_get_recent_captures_raises_rather_than_returning_empty(self): + captures_dir = MagicMock() + captures_dir.exists.return_value = True + captures_dir.iterdir.side_effect = PermissionError("permission denied") + builder = MenuBuilder(self._app_with_captures_dir(captures_dir)) + + with pytest.raises(OSError): + builder._get_recent_captures() + + +class TestViewCaptureChecksExitStatus: + """A CLI that ran and failed used to be indistinguishable from one that worked.""" + + def test_nonzero_exit_falls_back_to_file_browser(self): + builder = MenuBuilder(MagicMock()) + with patch("subprocess.run") as run, patch.object( + builder, "_open_in_file_browser" + ) as fallback: + run.return_value = subprocess.CompletedProcess([], 2, b"", b"boom") + builder._view_capture("/tmp/capture") + fallback.assert_called_once_with("/tmp/capture") + + def test_zero_exit_does_not_fall_back(self): + builder = MenuBuilder(MagicMock()) + with patch("subprocess.run") as run, patch.object( + builder, "_open_in_file_browser" + ) as fallback: + run.return_value = subprocess.CompletedProcess([], 0, b"", b"") + builder._view_capture("/tmp/capture") + fallback.assert_not_called() + + def test_missing_cli_falls_back_once(self): + builder = MenuBuilder(MagicMock()) + with patch("subprocess.run", side_effect=FileNotFoundError), patch.object( + builder, "_open_in_file_browser" + ) as fallback: + builder._view_capture("/tmp/capture") + fallback.assert_called_once_with("/tmp/capture") + + +class TestDeleteCaptureWhenNoDialogCanBeShown: + """"We never asked" must not be silently read as "the user said no".""" + + def test_unavailable_dialog_deletes_nothing_and_says_so(self): + app = MagicMock() + app.platform.confirm_dialog.side_effect = DialogUnavailableError("no display") + builder = MenuBuilder(app) + + with patch("shutil.rmtree") as rmtree: + builder._delete_capture("/tmp/capture", "cap") + + rmtree.assert_not_called() + app.notifications.show.assert_called_once() + title = app.notifications.show.call_args[0][0] + assert "confirm" in title.lower() + + def test_declining_deletes_nothing_and_stays_quiet(self): + app = MagicMock() + app.platform.confirm_dialog.return_value = False + builder = MenuBuilder(app) + + with patch("shutil.rmtree") as rmtree: + builder._delete_capture("/tmp/capture", "cap") + + rmtree.assert_not_called() + app.notifications.show.assert_not_called() diff --git a/tests/test_platform.py b/tests/test_platform.py index 02429f8..cd50215 100644 --- a/tests/test_platform.py +++ b/tests/test_platform.py @@ -1,12 +1,16 @@ """Tests for platform detection and handlers.""" +import subprocess import sys from unittest.mock import MagicMock, patch import pytest from openadapt_tray.platform import get_platform_handler -from openadapt_tray.platform.base import PlatformHandler +from openadapt_tray.platform.base import DialogUnavailableError, PlatformHandler +from openadapt_tray.platform.linux import LinuxHandler +from openadapt_tray.platform.macos import MacOSHandler +from openadapt_tray.platform.windows import WindowsHandler class TestPlatformDetection: @@ -186,3 +190,125 @@ def test_setup_does_nothing(self): handler = LinuxHandler() handler.setup() # Should not raise + + +class TestLinuxDialogsSeparateFailureFromAnswer: + """A dialog tool that could not run must not speak for the user. + + zenity/kdialog exit 1 when the user cancels and 255 when the tool itself + fails (no DISPLAY, broken GTK). Treating every non-zero exit as the user's + answer both invented an answer AND skipped the remaining fallbacks. + """ + + def _result(self, returncode, stdout=""): + return subprocess.CompletedProcess([], returncode, stdout, "") + + def test_prompt_input_exit_1_is_a_real_cancel(self): + handler = LinuxHandler() + with patch("subprocess.run", return_value=self._result(1)) as run: + assert handler.prompt_input("t", "m") is None + # Cancel is an answer: no point asking kdialog the same question. + assert run.call_count == 1 + + def test_prompt_input_tool_failure_tries_the_next_tool(self): + handler = LinuxHandler() + calls = [] + + def fake_run(cmd, **kwargs): + calls.append(cmd[0]) + if cmd[0] == "zenity": + return self._result(255) # zenity could not show anything + return self._result(0, "typed name") + + with patch("subprocess.run", side_effect=fake_run): + assert handler.prompt_input("t", "m") == "typed name" + assert calls == ["zenity", "kdialog"] + + def test_prompt_input_raises_when_nothing_can_ask(self): + handler = LinuxHandler() + with patch("subprocess.run", return_value=self._result(255)), patch.dict( + sys.modules, {"tkinter": None} + ), pytest.raises(DialogUnavailableError): + handler.prompt_input("t", "m") + + def test_confirm_dialog_exit_1_is_a_real_no(self): + handler = LinuxHandler() + with patch("subprocess.run", return_value=self._result(1)) as run: + assert handler.confirm_dialog("t", "m") is False + assert run.call_count == 1 + + def test_confirm_dialog_tool_failure_tries_the_next_tool(self): + handler = LinuxHandler() + calls = [] + + def fake_run(cmd, **kwargs): + calls.append(cmd[0]) + if cmd[0] == "zenity": + return self._result(255) + return self._result(0) + + with patch("subprocess.run", side_effect=fake_run): + assert handler.confirm_dialog("t", "m") is True + assert calls == ["zenity", "kdialog"] + + def test_confirm_dialog_raises_rather_than_answering_no_for_the_user(self): + handler = LinuxHandler() + with patch("subprocess.run", return_value=self._result(255)), patch.dict( + sys.modules, {"tkinter": None} + ), pytest.raises(DialogUnavailableError): + handler.confirm_dialog("t", "m") + + +class TestWindowsDialogsSeparateFailureFromAnswer: + def test_confirm_dialog_raises_when_no_dialog_can_be_shown(self): + handler = WindowsHandler() + # Block BOTH mechanisms, on every OS: blocking ctypes matters on + # Windows runners, where the real MessageBoxW would block forever. + with patch.dict( + sys.modules, {"ctypes": None, "tkinter": None} + ), pytest.raises(DialogUnavailableError): + handler.confirm_dialog("t", "m") + + def test_prompt_input_raises_when_no_dialog_can_be_shown(self): + handler = WindowsHandler() + with patch.dict(sys.modules, {"tkinter": None}), pytest.raises( + DialogUnavailableError + ): + handler.prompt_input("t", "m") + + +class TestMacOSDialogsSeparateFailureFromAnswer: + """osascript exits 1 for a user cancel AND for an execution error.""" + + def _result(self, returncode, stdout="", stderr=""): + return subprocess.CompletedProcess([], returncode, stdout, stderr) + + def test_cancel_is_an_answer(self): + handler = MacOSHandler() + with patch("subprocess.run", return_value=self._result(1, "", "")): + assert handler.confirm_dialog("t", "m") is False + assert handler.prompt_input("t", "m") is None + + def test_execution_error_raises_instead_of_answering_for_the_user(self): + handler = MacOSHandler() + failure = self._result( + 1, "", "execution error: Not authorized to send Apple events (-1743)" + ) + with patch("subprocess.run", return_value=failure): + with pytest.raises(DialogUnavailableError): + handler.confirm_dialog("t", "m") + with pytest.raises(DialogUnavailableError): + handler.prompt_input("t", "m") + + def test_missing_osascript_raises(self): + handler = MacOSHandler() + with patch("subprocess.run", side_effect=FileNotFoundError("osascript")): + with pytest.raises(DialogUnavailableError): + handler.confirm_dialog("t", "m") + with pytest.raises(DialogUnavailableError): + handler.prompt_input("t", "m") + + def test_ok_is_still_ok(self): + handler = MacOSHandler() + with patch("subprocess.run", return_value=self._result(0, "OK", "")): + assert handler.confirm_dialog("t", "m") is True