Add macOS app bundle with double-click file associations - #54
Conversation
Package the viewer as cdisplayagain.app so Finder can open .cbz/.cbr on double-click, matching the Linux desktop-entry workflow. macOS delivers a double-clicked file through an openDocument Apple Event rather than argv, so main() registers a ::tk::mac::OpenDocument handler and waits briefly for it before falling back to the file dialog. The handler is re-registered against the running viewer, so opening a second comic from Finder loads it into the open window. All of this is guarded on darwin; the Linux argv path is unchanged. Fix two bugs this exposed: - _init_logging created a relative "logs" directory. Finder launches apps with the working directory set to "/", so a packaged build died with "Errno 30: Read-only file system" before showing a window. A frozen build now logs under ~/Library/Logs on macOS and $XDG_STATE_HOME on Linux, and an unwritable log root degrades to console logging instead of aborting. CDISPLAYAGAIN_LOG_DIR still overrides everything, as the CI and compatibility scripts rely on it. - A file:// argument was truncated to a single character by raw[7] instead of raw[7:]. The spec grows a BUNDLE step behind a sys.platform check, declaring CFBundleDocumentTypes for cbz/cbr/cbt/cba at LSHandlerRank Owner plus exported UTIs, and builds windowed so Finder does not open a Terminal alongside the viewer. The Linux build path is untouched. make install now builds first, so a stale dist/ cannot be installed, and install-macos.sh removes prior installs before copying. With no explicit MACOS_APPDIR it sweeps /Applications and ~/Applications; with one set it stays scoped to that directory and leaves system default handlers alone. scripts/tk-env.sh points Tk at the Tcl/Tk that uv's python-build-standalone keeps outside the virtualenv, which otherwise breaks every Tk() call in a source run on macOS.
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe pull request adds macOS app-bundle support, Finder document opening, platform-aware logging, installation and packaging scripts, Tk environment handling, and native or Docker-based test execution. ChangesmacOS runtime integration
macOS packaging and distribution
Documentation and containerized testing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
cdisplayagain.py (1)
1743-1758: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a precise callback type and note the queue reset.
Two points:
Callablehas no parameter or return types. The coding guidelines require precise annotations on public functions. UseCallable[..., None].register_open_document_handlerclears_PENDING_OPEN_DOCUMENTSon every call. The second registration inmain(Line 1849) therefore drops any paths that Finder queued during startup but thatawait_open_documentdid not consume. If a user selects several comics and opens them together, the extra paths are lost silently. If that is intended for a single-window viewer, keep it. If not, drain the queue into the new callback after registering.As per coding guidelines: "Keep public functions annotated with precise types instead of using Any (ruff ANN401 rule)".
♻️ Proposed annotation change
-def register_open_document_handler(root: tk.Tk, callback: Callable | None = None) -> bool: +def register_open_document_handler( + root: tk.Tk, callback: Callable[..., None] | None = None +) -> bool:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cdisplayagain.py` around lines 1743 - 1758, Update the public register_open_document_handler signature to use Callable[..., None] | None instead of untyped Callable. Avoid clearing _PENDING_OPEN_DOCUMENTS during repeated registration; after installing the callback, preserve and deliver any queued paths to the selected callback so startup events are not lost.Source: Coding guidelines
Makefile (1)
175-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun the container test as the host UID:GID.
docker rundefaults to root while binding$(CURDIR)into/app, so pytest creates root-owned.pytest_cache/__pycache__entries in the checkout. Add--user "$$(id -u):$$(id -g)"before running pytest.The image tag matches the compose service
cdisplayagain-ci:13, so no tag mismatch remains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 175 - 191, Update the pytest-container docker run invocation to execute as the host user by adding the escaped host UID:GID via --user before the image command. Keep the existing cdisplayagain-ci:13 image, mounts, environment, and pytest command unchanged.scripts/tk-env.sh (1)
20-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDiscover the Tcl/Tk directory instead of hardcoding 8.6.
python-build-standalone 3.13/3.14 can ship Tcl/Tk 9.0, so
lib/tcl8.6andlib/tk8.6may not exist. Ifscripts/tk-env.shprints nothing,Makefile’sTK_ENVstays empty and Tk still reports “Can’t find a usable init.tcl”. Resolve the install by globbinglib/tcl*/lib/tk*or readingtkinter.TkVersion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tk-env.sh` around lines 20 - 27, The Tcl/Tk discovery logic using tcl_library and tk_library must stop hardcoding 8.6. Update the Python code in scripts/tk-env.sh to discover matching lib/tcl* and lib/tk* directories, or derive the version via tkinter.TkVersion, then emit TCL_LIBRARY and TK_LIBRARY when init.tcl and the Tk directory exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cdisplayagain.spec`:
- Around line 105-118: Update the UTTypeIdentifier values in
UTExportedTypeDeclarations to app-owned identifiers under
io.github.joshclwren.cdisplayagain for both CBZ and CBR, and update the
corresponding LSItemContentTypes entries to use those exact same identifiers
while preserving the existing descriptions, conformances, and filename
extensions.
In `@README.md`:
- Line 205: Update the README entry for make pytest to state that it runs tests
with real Tk windows on macOS, and direct users to make pytest-container for the
headless Docker execution path.
In `@scripts/install-macos.sh`:
- Around line 119-123: Update scripts/install-macos.sh lines 119-123 around the
codesign invocation to capture its exit status instead of suppressing failure,
and print a warning naming the manual codesign --force --sign - command when
signing fails. Update lines 137-141 to collect extensions whose duti
registration fails, suppress per-extension errors only while recording them, and
report success only for extensions that succeeded.
- Around line 137-141: Update the duti-handling branch to track whether any
invocation in the extension loop fails instead of unconditionally suppressing
errors. Use that result to report success only when all handlers were set, and
otherwise emit a failure message indicating the default-handler configuration
did not complete.
In `@scripts/package-macos.sh`:
- Around line 37-40: Update install-macos.sh so its bundle discovery checks
script_dir in addition to source_root, allowing the packaged install.sh and
cdisplayagain.app siblings to be found. Preserve the existing source_root
comparison in warn_about_stray_copies, and also exclude the script_dir copy from
stray-copy warnings.
In `@tests/test_macos_open_document.py`:
- Around line 148-156: Replace the permissive ComicViewer mocks in
tests/test_macos_open_document.py at lines 148-156 and 170-178 with one shared
interface fake defining the constructor, _fullscreen, _set_cursor_hidden(), and
_request_focus(). Use the fake in both tests and assert each instance captured
the expected comic_path; no direct change is needed elsewhere.
- Around line 148-156: Patch cdisplayagain._init_logging in both main() test
contexts to prevent real logging initialization:
tests/test_macos_open_document.py lines 148-156 and lines 170-178. Add the mock
alongside the existing tkinter, viewer, document, dialog, and argv patches; both
sites require the same direct change.
---
Nitpick comments:
In `@cdisplayagain.py`:
- Around line 1743-1758: Update the public register_open_document_handler
signature to use Callable[..., None] | None instead of untyped Callable. Avoid
clearing _PENDING_OPEN_DOCUMENTS during repeated registration; after installing
the callback, preserve and deliver any queued paths to the selected callback so
startup events are not lost.
In `@Makefile`:
- Around line 175-191: Update the pytest-container docker run invocation to
execute as the host user by adding the escaped host UID:GID via --user before
the image command. Keep the existing cdisplayagain-ci:13 image, mounts,
environment, and pytest command unchanged.
In `@scripts/tk-env.sh`:
- Around line 20-27: The Tcl/Tk discovery logic using tcl_library and tk_library
must stop hardcoding 8.6. Update the Python code in scripts/tk-env.sh to
discover matching lib/tcl* and lib/tk* directories, or derive the version via
tkinter.TkVersion, then emit TCL_LIBRARY and TK_LIBRARY when init.tcl and the Tk
directory exist.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 477a8877-9acf-4c58-a456-f39dedcfc98c
📒 Files selected for processing (12)
MakefileREADME.mdcdisplayagain.icnscdisplayagain.pycdisplayagain.specscripts/install-macos.shscripts/make-icns.shscripts/package-macos.shscripts/tk-env.shtests/test_log_location.pytests/test_macos_open_document.pytests/test_parity_tasks.py
| "UTExportedTypeDeclarations": [ | ||
| { | ||
| "UTTypeIdentifier": "public.cbz-archive", | ||
| "UTTypeDescription": "Comic Book ZIP Archive", | ||
| "UTTypeConformsTo": ["public.zip-archive"], | ||
| "UTTypeTagSpecification": {"public.filename-extension": ["cbz"]}, | ||
| }, | ||
| { | ||
| "UTTypeIdentifier": "public.cbr-archive", | ||
| "UTTypeDescription": "Comic Book RAR Archive", | ||
| "UTTypeConformsTo": ["public.archive"], | ||
| "UTTypeTagSpecification": {"public.filename-extension": ["cbr"]}, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Are public.cbz-archive and public.cbr-archive Apple-defined uniform type identifiers, and may third-party apps declare them in UTExportedTypeDeclarations?
💡 Result:
No, public.cbz-archive and public.cbr-archive are not Apple-defined Uniform Type Identifiers (UTIs) [1][2][3][4]. Uniform Type Identifiers that start with the public domain (e.g., public.*) are reserved for use by Apple [2][5]. Because these specific identifiers are not part of Apple's system-declared UTI registry, they should not be used as if they were official system types [1][2][6]. Regarding the declaration of these identifiers: 1. UTExportedTypeDeclarations: You may use the UTExportedTypeDeclarations key in your app's Info.plist to define your own UTIs for proprietary or unsupported file formats [6]. However, if you are defining a UTI for a format that is already established by another organization (such as comic book archive formats), it is best practice to use a reverse-DNS identifier owned by your organization (e.g., com.yourcompany.cbz-archive) rather than attempting to claim a public-domain identifier [6]. 2. Ownership and Conflicts: Because public-domain UTIs are reserved, if you declare one in your UTExportedTypeDeclarations, you may encounter conflicts with the system or other applications that attempt to define or utilize the same identifier [2][6]. It is technically possible for an app to include these strings in its Info.plist, but it is incorrect to treat them as Apple-defined standards, and doing so is discouraged as it violates the intended naming conventions for third-party UTIs [2][6]. For supporting standard comic book formats like CBZ or CBR, developers typically define their own unique, reverse-DNS-style UTIs in their app's Info.plist and map them to the appropriate file extensions [7][6].
Citations:
- 1: https://developer.apple.com/library/archive/documentation/Miscellaneous/Reference/UTIRef/Articles/System-DeclaredUniformTypeIdentifiers.html
- 2: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/understanding_utis/understand_utis_conc/understand_utis_conc.html
- 3: https://leopard-adc.pepas.com/documentation/Carbon/Conceptual/understanding_utis/utilist/UTIlist.html
- 4: https://leopard-adc.pepas.com/documentation/Carbon/Conceptual/understanding_utis/understanding_utis.pdf
- 5: https://leopard-adc.pepas.com/documentation/Carbon/Conceptual/understanding_utis/understand_utis_conc/understand_utis_conc.html
- 6: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/understanding_utis/understand_utis_declare/understand_utis_declare.html
- 7: https://github.com/comictagger/comictagger/blob/f04c3481/build-tools/comictagger.spec
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate cdisplayagain.spec =="
fd -a 'cdisplayagain\.spec$' . || true
echo "== relevant snippets =="
if [ -f cdisplayagain.spec ]; then
sed -n '1,150p' cdisplayagain.spec | cat -n
fi
echo "== UTImportedTypeDeclarations and public.* declarations in repo =="
rg -n "UTExportedTypeDeclarations|UTImportedTypeDeclarations|public\.(cbz-archive|cbr-archive|public\.archive|zip-archive)|LSItemContentTypes" . || true
echo "== spec metadata values around app id/bundle/zip if present =="
python3 - <<'PY'
from pathlib import Path
p=Path('cdisplayagain.spec')
if p.exists():
text=p.read_text()
for needle in ['id:', 'name:', 'summary:', 'summary[en]:', 'description[en]:', 'Version', 'PackageGroup']:
for i,line in enumerate(text.splitlines(),1):
if needle in line or (needle=='Version' and 'Version:' in line) or (needle.startswith('description[') and 'description[' in line):
if (needle=='description[en]:' and line.startswith('description')) or needle in line:
print(f'{i}:{line}')
PYRepository: JoshCLWren/cdisplayagain
Length of output: 6006
🌐 Web query:
Apple CFBundleDocumentTypes LSItemContentTypes imported UTI UTImportedTypeDeclarations public.domain reserved bundle identifier namespace
💡 Result:
In Apple development, Uniform Type Identifiers (UTIs) are used to uniquely identify file types, data types, and bundles [1][2][3]. When your app needs to support or declare specific file types, you manage these definitions within your app's Info.plist using specific keys [4][5]. CFBundleDocumentTypes and LSItemContentTypes The CFBundleDocumentTypes key is an array of dictionaries that informs the system about the document types your app supports [6][4][7]. Within these dictionaries, the LSItemContentTypes key holds an array of strings, where each string is a UTI representing a supported file type [8][4][7]. UTImportedTypeDeclarations vs. UTExportedTypeDeclarations If your app utilizes a file type that is not owned by your app (e.g., one designed by another company or another app), you should include it in the UTImportedTypeDeclarations array in your Info.plist [9][5][10]. Conversely, if you are the creator or the authoritative source for a proprietary file format, you should include the definition in the UTExportedTypeDeclarations array [5][10]. UTI Namespace and Reserved Domains UTIs follow a reverse-DNS format (e.g., com.companyname.app.documenttype) to ensure global uniqueness [1][11][2]. Regarding reserved namespaces: - The public domain (e.g., public.text, public.jpeg) is reserved strictly for Apple-defined types that are broadly used across the system [1][2]. Developers should not create new UTIs within the public domain [1]. - The dyn domain is reserved for dynamic UTIs, which the system generates automatically when no other identifier is available [1][2]. - Third-party developers should use their own reverse-DNS domain (e.g., com.yourcompany) to define their unique UTI namespace [1][5]. To define a new UTI, you provide metadata such as the UTI identifier, a description, and the UTI's conformance (e.g., public.data or com.apple.package) to ensure the system understands how to handle the file [5][12].
Citations:
- 1: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/understanding_utis/understand_utis_conc/understand_utis_conc.html
- 2: https://developer.apple.com/library/archive/documentation/General/Conceptual/DevPedia-CocoaCore/UniformTypeIdentifier.html
- 3: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/understanding_utis/understand_utis_intro/understand_utis_intro.html
- 4: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/DocumentInteraction_TopicsForIOS/Articles/RegisteringtheFileTypesYourAppSupports.html
- 5: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/understanding_utis/understand_utis_declare/understand_utis_declare.html
- 6: https://developer.apple.com/documentation/bundleresources/information-property-list/cfbundledocumenttypes
- 7: https://developer.apple.com/library/archive/documentation/General/Reference/InfoPlistKeyReference/Articles/CoreFoundationKeys.html
- 8: https://developer.apple.com/documentation/bundleresources/information-property-list/cfbundledocumenttypes/lsitemcontenttypes
- 9: https://developer.apple.com/documentation/bundleresources/information-property-list/utimportedtypedeclarations
- 10: https://developer.apple.com/videos/play/tech-talks/10696/
- 11: https://developer.apple.com/help/glossary/bundle-id/
- 12: https://developer.apple.com/library/archive/qa/qa1587/_index.html
🌐 Web query:
"public.cbz-archive" UTI system declared uniform type identifier
💡 Result:
The string public.cbz-archive is not a system-declared Uniform Type Identifier (UTI) defined by Apple [1][2]. UTIs that begin with the public. prefix are reserved for system-declared identifiers defined exclusively by Apple [1]. Third-party developers who wish to support the CBZ (Comic Book ZIP) format must declare their own UTIs or use established custom identifiers within their application bundles [3]. In practice, public.cbz-archive is often used by third-party comic book reader applications as a convention to identify CBZ files [4]. Because there is no single official standard UTI for this format, various applications define their own unique identifiers, such as com.simplecomic.cbz-archive, com.yacreader.yacreader.cbz, or com.bitcartel.comicbooklover.cbz [4]. When configuring software to support CBZ files, developers typically include a list of these common identifiers in their Info.plist file to ensure interoperability across different tools [4][3].
Citations:
- 1: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/understanding_utis/understand_utis_conc/understand_utis_conc.html
- 2: https://developer.apple.com/library/archive/documentation/Miscellaneous/Reference/UTIRef/Articles/System-DeclaredUniformTypeIdentifiers.html
- 3: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/understanding_utis/understand_utis_declare/understand_utis_declare.html
- 4: https://github.com/comictagger/comictagger/blob/f04c3481/build-tools/comictagger.spec
Use app-owned UTIs for CBZ and CBR support.
public.cbz-archive and public.cbr-archive do not match Apple-defined UTIs, so these UTExportedTypeDeclarations claim reserved and potentially conflicting identifiers. Declare the formats with identifiers under io.github.joshclwren.cdisplayagain, such as io.github.joshclwren.cdisplayagain.cbz-archive and io.github.joshclwren.cdisplayagain.cbr-archive, and keep the same values in LSItemContentTypes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cdisplayagain.spec` around lines 105 - 118, Update the UTTypeIdentifier
values in UTExportedTypeDeclarations to app-owned identifiers under
io.github.joshclwren.cdisplayagain for both CBZ and CBR, and update the
corresponding LSItemContentTypes entries to use those exact same identifiers
while preserving the existing descriptions, conformances, and filename
extensions.
| - `make sync`: install dependencies from `uv.lock`. | ||
| - `make lint`: run ruff. | ||
| - `make pytest`: run the test suite. | ||
| - `make pytest`: run the test suite (xvfb on Linux, container on macOS). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the macOS make pytest behavior.
make pytest opens real Tk windows on macOS. It does not use a container. Direct users to make pytest-container for the headless Docker path.
Proposed fix
- - `make pytest`: run the test suite (xvfb on Linux, container on macOS).
+ - `make pytest`: run the test suite (xvfb on Linux; real Tk windows on macOS).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `make pytest`: run the test suite (xvfb on Linux, container on macOS). | |
| - `make pytest`: run the test suite (xvfb on Linux; real Tk windows on macOS). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 205, Update the README entry for make pytest to state that
it runs tests with real Tk windows on macOS, and direct users to make
pytest-container for the headless Docker execution path.
| # Copying rewrites nothing, but an unsigned bundle that moved needs its ad-hoc | ||
| # signature refreshed or arm64 refuses to exec it. | ||
| if command -v codesign >/dev/null 2>&1; then | ||
| codesign --force --sign - --timestamp=none "$installed_app" >/dev/null 2>&1 || true | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Suppressed exit statuses make the installer report success after a failure. Both steps end with || true or 2>/dev/null, and the script then prints an unconditional success message. The user sees a completed install even when the bundle cannot launch or the file associations are not set.
scripts/install-macos.sh#L119-L123: check thecodesignexit status and print a warning that names the manualcodesign --force --sign -command.scripts/install-macos.sh#L137-L141: collect the extensions for whichdutifails, and print the success message only for the extensions that succeeded.
📍 Affects 1 file
scripts/install-macos.sh#L119-L123(this comment)scripts/install-macos.sh#L137-L141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-macos.sh` around lines 119 - 123, Update
scripts/install-macos.sh lines 119-123 around the codesign invocation to capture
its exit status instead of suppressing failure, and print a warning naming the
manual codesign --force --sign - command when signing fails. Update lines
137-141 to collect extensions whose duti registration fails, suppress
per-extension errors only while recording them, and report success only for
extensions that succeeded.
| elif command -v duti >/dev/null 2>&1; then | ||
| for extension in "${extensions[@]}"; do | ||
| duti -s "$bundle_id" "$extension" all 2>/dev/null || true | ||
| done | ||
| echo "Set cdisplayagain as the default handler for: ${extensions[*]}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report a duti failure instead of claiming success.
Line 139 discards the exit status of every duti call. Line 141 then always reports that the default handler is set. If duti fails, the message is wrong and the user does not know that Finder still opens another app. Track the failures and adjust the message.
🔧 Proposed fix
elif command -v duti >/dev/null 2>&1; then
+ failed=()
for extension in "${extensions[@]}"; do
- duti -s "$bundle_id" "$extension" all 2>/dev/null || true
+ duti -s "$bundle_id" "$extension" all 2>/dev/null || failed+=("$extension")
done
- echo "Set cdisplayagain as the default handler for: ${extensions[*]}"
+ if [[ ${`#failed`[@]} -eq 0 ]]; then
+ echo "Set cdisplayagain as the default handler for: ${extensions[*]}"
+ else
+ echo "WARNING: duti could not set the handler for: ${failed[*]}" >&2
+ fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif command -v duti >/dev/null 2>&1; then | |
| for extension in "${extensions[@]}"; do | |
| duti -s "$bundle_id" "$extension" all 2>/dev/null || true | |
| done | |
| echo "Set cdisplayagain as the default handler for: ${extensions[*]}" | |
| elif command -v duti >/dev/null 2>&1; then | |
| failed=() | |
| for extension in "${extensions[@]}"; do | |
| duti -s "$bundle_id" "$extension" all 2>/dev/null || failed+=("$extension") | |
| done | |
| if [[ ${`#failed`[@]} -eq 0 ]]; then | |
| echo "Set cdisplayagain as the default handler for: ${extensions[*]}" | |
| else | |
| echo "WARNING: duti could not set the handler for: ${failed[*]}" >&2 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-macos.sh` around lines 137 - 141, Update the duti-handling
branch to track whether any invocation in the extension loop fails instead of
unconditionally suppressing errors. Use that result to report success only when
all handlers were set, and otherwise emit a failure message indicating the
default-handler configuration did not complete.
| # ditto preserves the bundle's ad-hoc signature and extended attributes; zip does not. | ||
| ditto "$app_bundle" "$package_root/cdisplayagain.app" | ||
| install -m 0644 "$root_dir/LICENSE" "$package_root/LICENSE" | ||
| install -m 0755 "$root_dir/scripts/install-macos.sh" "$package_root/install.sh" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The packaged install.sh cannot find the bundle it ships with.
Line 38 places the bundle at <package_root>/cdisplayagain.app. Line 40 places the installer at <package_root>/install.sh.
scripts/install-macos.sh Line 25 computes source_root as the parent of the script directory, because it assumes it lives in scripts/. In the extracted archive, the script directory is <package_root>, so source_root becomes the extraction directory one level above. The candidate paths at Lines 92-97 then resolve to <extract_dir>/dist/cdisplayagain.app and <extract_dir>/cdisplayagain.app. Neither exists, and the installer aborts with "ERROR: no cdisplayagain.app found. Run 'make build' first."
The documented flow in README-install.txt ("Install with: ./install.sh") therefore fails for every downloaded release. Make install-macos.sh search the script directory as well as its parent.
🐛 Proposed fix in scripts/install-macos.sh
-source_root=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/.." && pwd)
+script_dir=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)
+source_root=$(cd -- "$script_dir/.." && pwd)-for candidate in "$source_root/dist/$app_name" "$source_root/$app_name"; do
+for candidate in "$source_root/dist/$app_name" "$script_dir/$app_name" "$source_root/$app_name"; doNote: warn_about_stray_copies compares against $source_root. Keep that comparison, and add $script_dir so the shipped copy is not reported as a stray.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/package-macos.sh` around lines 37 - 40, Update install-macos.sh so
its bundle discovery checks script_dir in addition to source_root, allowing the
packaged install.sh and cdisplayagain.app siblings to be found. Preserve the
existing source_root comparison in warn_about_stray_copies, and also exclude the
script_dir copy from stray-copy warnings.
| with ( | ||
| patch("tkinter.Tk") as mock_tk, | ||
| patch.object(cdisplayagain, "ComicViewer") as mock_viewer, | ||
| patch.object(cdisplayagain, "await_open_document", return_value=str(comic)), | ||
| patch("tkinter.filedialog.askopenfilename") as mock_dialog, | ||
| patch("sys.argv", ["cdisplayagain.py"]), | ||
| ): | ||
| mock_tk.return_value = MagicMock() | ||
| cdisplayagain.main() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace ComicViewer mocks with interface fakes.
MagicMock accepts any member access. These tests can pass when main() calls an invalid viewer member. Use a small fake that defines the constructor, _fullscreen, _set_cursor_hidden(), and _request_focus().
tests/test_macos_open_document.py#L148-L156: use an interface fake and assert its capturedcomic_path.tests/test_macos_open_document.py#L170-L178: use the same interface fake and assert its capturedcomic_path.
As per coding guidelines, “Mirror the real ComicViewer interface in fakes rather than relaxing production code.”
📍 Affects 1 file
tests/test_macos_open_document.py#L148-L156(this comment)tests/test_macos_open_document.py#L170-L178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_macos_open_document.py` around lines 148 - 156, Replace the
permissive ComicViewer mocks in tests/test_macos_open_document.py at lines
148-156 and 170-178 with one shared interface fake defining the constructor,
_fullscreen, _set_cursor_hidden(), and _request_focus(). Use the fake in both
tests and assert each instance captured the expected comic_path; no direct
change is needed elsewhere.
Source: Coding guidelines
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Isolate logging from these main() tests.
main() calls _init_logging() before the mocked GUI path. Both tests can create logs/ in the repository and can fail when that directory is not writable.
tests/test_macos_open_document.py#L148-L156: patchcdisplayagain._init_loggingin the test context.tests/test_macos_open_document.py#L170-L178: patchcdisplayagain._init_loggingin the test context.
📍 Affects 1 file
tests/test_macos_open_document.py#L148-L156(this comment)tests/test_macos_open_document.py#L170-L178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_macos_open_document.py` around lines 148 - 156, Patch
cdisplayagain._init_logging in both main() test contexts to prevent real logging
initialization: tests/test_macos_open_document.py lines 148-156 and lines
170-178. Add the mock alongside the existing tkinter, viewer, document, dialog,
and argv patches; both sites require the same direct change.
Fold in the parts of the earlier file-associations branch that are better than what this branch had. tk_bootstrap.py replaces scripts/tk-env.sh. Setting TCL_LIBRARY and TK_LIBRARY at import time fixes uv's python-build-standalone Tk for every entry point, including plain "python cdisplayagain.py", pytest, and IDE launches. The shell helper only reached commands routed through the Makefile and needed an eval anywhere else. Add packaging/linux/cdisplayagain.xml so install-desktop registers the cbz/cbr MIME types with shared-mime-info instead of assuming the desktop already knows them, and install the icon so the desktop entry can reference it. Pin uv to managed interpreters, matching the runtime tk_bootstrap targets.
Superseding #42Folded in the two pieces #42 did better, and closed it in favour of this branch (which is current; #42 had gone Adopted from #42:
Kept from this branch, where the approaches differed:
Only here: the Both branches independently diagnosed the relative- |
A developer with UV_INDEX_URL or a pip.conf mirror set has every uv command silently rewrite the registry of all 25 entries in uv.lock. Committing that breaks CI and any clone that cannot reach the mirror, and nothing announces the change, so it is easy to stage by accident. The pre-commit hook now fails when a staged uv.lock references any registry other than pypi.org, naming the offending URLs.
Add a macOS job to the release workflow, on macos-14 for arm64 and macos-13 for Intel, so a tag produces downloadable app bundles alongside the Linux archive. The job does not reuse the shared setup action: that action is Linux-only and rebuilds pyvips from source, while macOS needs the pyvips-binary wheel that carries libvips inside the bundle. Two checks guard the regressions this platform actually hit. The bundle is run from "/" to catch anything that assumes a writable working directory, the way the relative log path did, and Info.plist is checked for its cbz and cbr declarations so a spec change cannot silently drop file associations. publish and attest now collect every artifact and regenerate SHA256SUMS across the full published set. scripts/update-cask.sh generates a Homebrew cask from a published release, reading the checksums the workflow published so the cask cannot disagree with the assets people download. Drop --sequesterRsrc from the packaging step; it only added __MACOSX noise, and the ad-hoc signature lives in the bundle rather than in xattrs.
Point macOS users at the tap first, and state plainly why the cask clears the quarantine flag: the bundle is ad-hoc signed rather than notarized, which needs a paid Apple Developer account.
PyInstaller does not emit Contents/PkgInfo, and Finder falls back to a generic icon for bundles without it even though Info.plist already carries CFBundlePackageType. Verified the ad-hoc signature still passes 'codesign --verify --deep --strict' with the file present.
Finder caches an app icon against the bundle path and its modification time, so installing over a previous copy kept showing the stale icon and needed a manual lsregister plus killall Finder afterwards. The installer now bumps the timestamp and relaunches Finder itself. Both are skipped when MACOS_APPDIR points somewhere non-standard, matching how default handlers are already left alone for scratch installs.
1 participant
Packages the viewer as
cdisplayagain.appso Finder opens.cbz/.cbron double-click, matching the Linux desktop-entry workflow. Everything platform-specific is guarded ondarwin; the Linux build, install, and argv paths are unchanged.Why app code had to change
macOS does not put a double-clicked file in
argv. Finder launches the app with an emptyargvand sends the path as anopenDocumentApple Event once the Tk event loop is running. Without a handler, double-clicking a comic launched the viewer and then showed the "Open Comic" file picker.main()now registers a::tk::mac::OpenDocumenthandler and pumps the event loop briefly (750ms) before falling back to the dialog. After the viewer exists the handler is re-registered against it, so opening a second comic from Finder loads it into the open window.Bugs found along the way
_init_loggingkilled every Finder launch. It created a relativelogsdirectory, and Finder sets the working directory to/, so the packaged app died withOSError: [Errno 30] Read-only file system: 'logs'before any window appeared. A frozen build now logs to~/Library/Logs/cdisplayagainon macOS and$XDG_STATE_HOME/cdisplayagain/logson Linux; source runs keep the relativelogs/;CDISPLAYAGAIN_LOG_DIRstill overrides everything, which the CI and compatibility scripts depend on. An unwritable log root now degrades to console logging rather than aborting startup. This was latent on Linux too, for any launcher with a read-only working directory.file://arguments were truncated.raw[7]took a single character whereraw[7:]was meant. Now parsed withurlparse+url2pathname, so percent-encoded paths decode correctly.Packaging
cdisplayagain.specgains aBUNDLEstep behind asys.platformcheck:CFBundleDocumentTypesfor cbz/cbr/cbt/cba atLSHandlerRank: Owner, images asAlternate, plus exported UTIs for cbz/cbr.console=False) on macOS only, so Finder does not open a Terminal beside the viewer. Linux keeps its console for CLI diagnostics.cdisplayagain.icns, generated byscripts/make-icns.sh. PyInstaller's PNG auto-conversion produced a hash-named single-size file thatCFBundleTypeIconFilecould not reference.The bundle carries its own Python, Tcl/Tk, Pillow, libvips, and unrar; it does not use the developer's venv.
Install
make installnow depends onbuild-onedir, so a staledist/can never be installed.scripts/install-macos.shcopies to/Applications, ad-hoc re-signs (arm64 refuses to exec a moved unsigned bundle), registers with Launch Services, writes a~/.local/bin/cdisplayagainwrapper, and sets default handlers viadutiwhen present, printing the manual Get Info steps otherwise.It removes prior installs before copying, sharing one routine with
--uninstall. With noMACOS_APPDIRset it sweeps/Applicationsand~/Applications; with one set it is scoped strictly to that directory and leaves system defaults alone, so pointing it at a scratch directory cannot remove a real install.make package-macosproduces a distributable zip viaditto(preserves the ad-hoc signature), mirroringpackage-linux.sh.Testing notes
tests/test_macos_open_document.pyandtests/test_log_location.py.FakeTkintest_parity_tasks.pygainedcreatecommand.make pyteston macOS opens real Tk windows for ~30s; there is no xvfb for Aqua.test_right_click_shows_context_menufails natively because Aqua'stk_popupneeds a live event loop.make pytest-containerruns headless in the Debian container with a dedicated Docker volume, so the host.venvis never overwritten with Linux binaries. It was observed hanging on macOS, where Docker bind-mounts the repo across the VM boundary, so it is documented as the Linux/CI-parity path rather than the macOS default.Verified
Built, installed, and confirmed working end to end on Apple silicon (macOS 15, arm64): double-clicking a
.cbzin Finder opens the viewer. Lint and pyright are clean. The full suite has not been run on Linux locally; CI covers that.Not included
No signing/notarization (the bundle is ad-hoc signed, so a downloaded copy is quarantined until right-click > Open), no universal2 binary, and no macOS CI job. Happy to add a build job if you want the bundle verified on every PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests