Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
e954924 to
d3b93eb
Compare
|
🔍 OpenCodeReview found 16 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - uses: actions/checkout@v3 | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v3 | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
actions/checkout 最新版本为 v4，v6 不存在。工作流将在运行第一步时立即失败，导致所有 macOS Intel 发布构建被阻塞。请改为 actions/checkout@v4。
Suggestion:
| - uses: actions/checkout@v6 | |
| - uses: actions/checkout@v4 |
| fetch-depth: 0 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v6 |
There was a problem hiding this comment.
actions/setup-python 最新版本为 v5，v6 不存在。工作流将在此步骤失败。请改为 actions/setup-python@v5。
Suggestion:
| uses: actions/setup-python@v6 | |
| uses: actions/setup-python@v5 |
| build-backend = "hatchling.build" | ||
|
|
||
| [tool.hatch.version] | ||
| path = "src/version.py" |
There was a problem hiding this comment.
版本号建议去掉 v 前缀。PEP 440 标准中版本号不应包含 v 前缀（如 v0.3.0-beta40）。虽然某些工具可能容忍这种格式，但在 PyPI 上传及与其他版本比较工具交互时可能导致意外问题。建议改为 "0.3.0-beta40"。
Suggestion:
| path = "src/version.py" | |
| path = "src/version.py" |
| deploy: | ||
|
|
||
| runs-on: macos-13 | ||
| runs-on: macos-15-intel |
There was a problem hiding this comment.
deploy 作业缺少 timeout-minutes 设置。若构建或归档步骤卡住，作业将无限运行（默认 6 小时），浪费 runner 资源并阻塞其他作业。建议添加 timeout-minutes: 30。
Suggestion:
| deploy: | |
| runs-on: macos-13 | |
| runs-on: macos-15-intel | |
| deploy: | |
| runs-on: macos-15-intel | |
| timeout-minutes: 30 |
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| pip install -r requirements_build_addon.txt | ||
| pip install Pillow | ||
| python -m pip install uv | ||
| uv sync --locked --group build |
There was a problem hiding this comment.
uv 依赖安装未配置缓存，每次运行都会重新下载所有 Python 包，增加构建时间和网络故障风险。建议在 setup-python 步骤中添加 cache: 'pip'，或使用 actions/cache 缓存 ~/.cache/uv 目录。
Suggestion:
| - name: Install dependencies | |
| run: | | |
| python -m pip install --upgrade pip | |
| pip install -r requirements.txt | |
| pip install -r requirements_build_addon.txt | |
| pip install Pillow | |
| python -m pip install uv | |
| uv sync --locked --group build | |
| - name: Install dependencies | |
| run: | | |
| python -m pip install uv | |
| uv sync --locked --group build | |
| env: | |
| UV_CACHE_DIR: /tmp/.uv-cache | |
| - name: Cache uv dependencies | |
| uses: actions/cache@v4 | |
| with: | |
| path: /tmp/.uv-cache | |
| key: ${{ runner.os }}-uv-${{ hashFiles('uv.lock') }} | |
| restore-keys: | | |
| ${{ runner.os }}-uv- |
| child.setSelected(True) | ||
| other.setSelected(True) | ||
|
|
||
| controller.context_menu.actions()[0].trigger() |
There was a problem hiding this comment.
通过 actions()[0] 按索引触发删除动作，依赖于上下文菜单中动作的添加顺序。如果将来在 TreeWidgetController.__init__ 中向 context_menu 添加其他动作，索引 0 可能不再是删除操作，导致测试误报。建议直接调用 controller.item_remove_current() 来验证删除逻辑，避免依赖内部实现细节。
Suggestion:
| controller.context_menu.actions()[0].trigger() | |
| controller.item_remove_current() |
| window.update_action.trigger() | ||
|
|
||
| assert window.statusbar.currentMessage() == expected_status | ||
| assert bool(opened_urls) is opened |
There was a problem hiding this comment.
使用 is 进行布尔值比较不符合 Python 惯用法则。虽然 CPython 中 True/False 为单例对象，此处行为正确，但应使用 == 进行值比较，使意图更明确，避免潜在的可移植性风险。
Suggestion:
| assert bool(opened_urls) is opened | |
| assert bool(opened_urls) == opened |
|
|
||
| window.trans = FakeTranslator() | ||
|
|
||
| assert window._load_translation("en") is True |
There was a problem hiding this comment.
使用 is True 进行布尔值断言不符合 Python 惯用法则。_load_translation 已返回布尔值，可直接使用真值检查 assert window._load_translation("en")，更简洁且意图明确。
Suggestion:
| assert window._load_translation("en") is True | |
| assert window._load_translation("en") |
| qtbot.waitUntil(lambda: window._worker is None, timeout=5_000) | ||
|
|
||
| assert calls[0][0] == "source.pdf" | ||
| assert calls[0][2] is True |
There was a problem hiding this comment.
使用 is True 验证函数参数值不符合 Python 惯用法则。测试中只需验证参数为真值，直接使用 assert calls[0][2] 更简洁，无需依赖身份比较。
Suggestion:
| assert calls[0][2] is True | |
| assert calls[0][2] |
| assert is_in("123", "1") is True | ||
| assert is_in("456", "1") is False | ||
| assert is_in("第1章", r"第\d章") is True |
There was a problem hiding this comment.
使用 is True / is False 比较布尔返回值。虽然 is_in 通过 bool() 包装始终返回 True/False 单例，is 在此处能正确工作，但更 Pythonic 的写法是直接使用 assert is_in(...) 和 assert not is_in(...) 进行真值判断，这更符合 PEP 8 建议且对返回类型变更更具鲁棒性。
Suggestion:
| assert is_in("123", "1") is True | |
| assert is_in("456", "1") is False | |
| assert is_in("第1章", r"第\d章") is True | |
| assert is_in("123", "1") | |
| assert not is_in("456", "1") | |
| assert is_in("第1章", r"第\d章") |
| deploy: | ||
|
|
||
| runs-on: macos-13 | ||
| runs-on: macos-15-intel |
There was a problem hiding this comment.
macos-15-intel 运行器标签不存在。GitHub 的 macOS 15 运行器均为 Apple Silicon (arm64)，没有 Intel 版本。鉴于该工作流专门用于 Intel 构建（项目另有 mac-silicon-release.yml 处理 Apple Silicon），应使用最后一个支持 Intel 的 macOS 运行器：macos-13。使用不存在的标签将导致作业永远无法启动。
Suggestion:
| runs-on: macos-15-intel | |
| runs-on: macos-13 |
| deploy: | ||
|
|
||
| runs-on: macos-13 | ||
| runs-on: macos-15-intel |
There was a problem hiding this comment.
作业缺少 timeout-minutes 设置。若构建过程因意外原因挂起（如 PyInstaller 打包卡死、网络请求阻塞），将耗尽运行器资源长达 6 小时（GitHub 默认上限）。建议根据构建经验设置合理的超时时间（如 30 分钟）。
Suggestion:
| runs-on: macos-15-intel | |
| deploy: | |
| timeout-minutes: 30 | |
| runs-on: macos-13 |
| jobs: | ||
| deploy: |
There was a problem hiding this comment.
缺少并发控制 (concurrency)。连续推送多个标签时可能产生并行运行，导致版本号冲突或重复上传。建议添加基于 github.ref 的并发组并启用 cancel-in-progress: true。
Suggestion:
| jobs: | |
| deploy: | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref }} | |
| cancel-in-progress: true | |
| jobs: | |
| deploy: |
| deploy: | ||
|
|
||
| runs-on: ubuntu-22.04 |
There was a problem hiding this comment.
deploy 作业未设置 timeout-minutes，若构建或测试步骤卡死将无限消耗 runner 资源。建议添加 timeout-minutes: 30。
Suggestion:
| deploy: | |
| runs-on: ubuntu-22.04 | |
| deploy: | |
| runs-on: ubuntu-22.04 | |
| timeout-minutes: 30 |
| on: | ||
| push: | ||
| tags: | ||
| - 'v*' | ||
| release: | ||
| types: [ published ] | ||
| - "v*" | ||
|
|
||
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
工作流缺少 concurrency 控制。虽然仅由 tag push 触发，但重复推送 tag 会产生并行构建。建议添加 concurrency 组以自动取消冗余运行。
Suggestion:
| on: | |
| push: | |
| tags: | |
| - 'v*' | |
| release: | |
| types: [ published ] | |
| - "v*" | |
| permissions: | |
| contents: write | |
| on: | |
| push: | |
| tags: | |
| - "v*" | |
| concurrency: | |
| group: linux-release-${{ github.ref }} | |
| cancel-in-progress: true | |
| permissions: | |
| contents: write |
| except Exception as e: # noqa: BLE001 - invalid third-party PDFs are expected | ||
| logger.warning("Read pdf %s failed! %s", path, e) | ||
| return [] |
There was a problem hiding this comment.
get_bookmarks 中 catch except Exception 范围过宽，虽然通过 # noqa: BLE001 抑制了 flake8 规则并注释说明「无效的第三方 PDF 是可预期的」，但仍可能吞掉非 PDF 解析相关的异常（如 MemoryError）。建议将捕获范围缩小到 pypdf 相关异常（如 PyPdfError 及其子类），或至少显式 re-raise 不应被吞掉的异常类型。
Suggestion:
| except Exception as e: # noqa: BLE001 - invalid third-party PDFs are expected | |
| logger.warning("Read pdf %s failed! %s", path, e) | |
| return [] | |
| except Exception as e: # noqa: BLE001 - invalid third-party PDFs are expected | |
| if not isinstance(e, (OSError, ValueError)): | |
| raise | |
| logger.warning("Read pdf %s failed! %s", path, e) | |
| return [] |
| def item_remove_current(self): | ||
| selecteds = self.selectedItems() | ||
| for item in selecteds: | ||
| for item in self.widget.selectedItems(): | ||
| self.remove_item(item) |
There was a problem hiding this comment.
直接遍历 selectedItems() 并逐个删除存在风险：当同时选中父项和子项时，先删除父项会导致 Qt 级联销毁子项（底层 C++ 对象被释放），后续迭代到已销毁的子项时将访问无效的 PySide6 包装对象，抛出 RuntimeError: "Internal C++ object already deleted"。建议先收集待删除项，过滤掉祖先已在集合中的子项，或收集后再批量删除。
Suggestion:
| def item_remove_current(self): | |
| selecteds = self.selectedItems() | |
| for item in selecteds: | |
| for item in self.widget.selectedItems(): | |
| self.remove_item(item) | |
| def item_remove_current(self): | |
| items = self.widget.selectedItems() | |
| # 过滤掉祖先已被选中的子项，避免级联删除导致访问已销毁对象 | |
| to_remove = [ | |
| item for item in items | |
| if not any(item is not ancestor and self._is_descendant(item, ancestor) for ancestor in items) | |
| ] | |
| for item in to_remove: | |
| self.remove_item(item) | |
| @staticmethod | |
| def _is_descendant(item, ancestor): | |
| parent = item.parent() | |
| while parent is not None: | |
| if parent is ancestor: | |
| return True | |
| parent = parent.parent() | |
| return False |
| PROJECT_ROOT / ".github/workflows/linux-release.yml" | ||
| ).read_text(encoding="utf-8") | ||
|
|
||
| assert test_workflow.count(f"bash {INSTALL_SCRIPT}") == 3 |
There was a problem hiding this comment.
使用精确计数 == 3 来断言 bash {INSTALL_SCRIPT} 的出现次数过于脆弱。每当 test.yml 中新增或移除一个需要 Qt 依赖的 Linux job 时，这个测试就会失败，而测试的意图（验证工作流使用了共享安装脚本而非内联设置）并未被违反。建议改为 >= 1 或 > 0，使测试更具韧性。
Suggestion:
| assert test_workflow.count(f"bash {INSTALL_SCRIPT}") == 3 | |
| assert test_workflow.count(f"bash {INSTALL_SCRIPT}") >= 1 |
| def test_release_network_and_json_failures_return_empty_response(failure): | ||
| release = Release.__new__(Release) | ||
| release.base_api_url = "https://example.com" | ||
| context = ( | ||
| patch("src.updater.request.urlopen", side_effect=failure) | ||
| if isinstance(failure, Exception) | ||
| else patch("src.updater.request.urlopen", return_value=failure) | ||
| ) | ||
|
|
||
| with context: | ||
| assert release._get_response("/latest") == {} |
There was a problem hiding this comment.
使用 Release.new(Release) 绕过 init 初始化，直接设置 base_api_url 后调用 _get_response。如果将来 _get_response 依赖了 init 中设置的其他属性（如 self.url），该测试将无法发现因未初始化导致的缺陷。建议改为通过正常构造路径测试：mock urlopen 后创建 Release 实例，并验证 latest_response 和 latest_tag 的返回值。
Suggestion:
| def test_release_network_and_json_failures_return_empty_response(failure): | |
| release = Release.__new__(Release) | |
| release.base_api_url = "https://example.com" | |
| context = ( | |
| patch("src.updater.request.urlopen", side_effect=failure) | |
| if isinstance(failure, Exception) | |
| else patch("src.updater.request.urlopen", return_value=failure) | |
| ) | |
| with context: | |
| assert release._get_response("/latest") == {} | |
| def test_release_network_and_json_failures_return_empty_response(failure): | |
| context = ( | |
| patch("src.updater.request.urlopen", side_effect=failure) | |
| if isinstance(failure, Exception) | |
| else patch("src.updater.request.urlopen", return_value=failure) | |
| ) | |
| with context: | |
| release = Release("https://github.com/example/project") | |
| assert release.latest_response == {} | |
| assert release.latest_tag is None |
| return True | ||
|
|
||
| window._worker = RunningWorker() | ||
| event = QtGui.QCloseEvent() |
There was a problem hiding this comment.
在 PySide6/Qt6 中，QCloseEvent 没有公开的无参构造函数（Qt6 C++ 中已移除）。虽然某些 PySide6 绑定版本可能暂时允许 QCloseEvent()，但这不是稳定的 API，可能在未来的版本或不同平台上导致 TypeError。更可靠的做法是传入 QEvent.Type.Close：QtGui.QCloseEvent(QtCore.QEvent.Type.Close)。
Suggestion:
| event = QtGui.QCloseEvent() | |
| event = QtGui.QCloseEvent(QtCore.QEvent.Type.Close) |
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: "3.10" |
There was a problem hiding this comment.
未配置 uv 缓存，每次运行都会重新下载所有依赖。可在 Set up Python 步骤中添加 cache: 'pip' 或使用专用缓存步骤缓存 ~/.cache/uv，减少重复构建时间。
Suggestion:
| - name: Set up Python | |
| uses: actions/setup-python@v6 | |
| with: | |
| python-version: "3.10" | |
| - name: Set up Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.10" | |
| cache: 'pip' |
| pip install -r requirements_build_addon.txt | ||
| pip install Pillow | ||
| python -m pip install uv | ||
| uv sync --locked --group build |
There was a problem hiding this comment.
uv sync --locked 要求仓库中存在 uv.lock 锁文件，但当前项目根目录下不存在该文件。这会导致依赖安装步骤直接失败。建议：要么将 uv.lock 纳入版本控制（推荐），要么移除 --locked 参数以允许自动解析依赖。
Suggestion:
| uv sync --locked --group build | |
| uv sync --group build |
| on: | ||
| push: | ||
| tags: | ||
| - 'v*' | ||
| release: | ||
| types: [ published ] | ||
| - "v*" |
There was a problem hiding this comment.
工作流缺少 concurrency 控制。虽然该工作流由标签推送触发，并发风险较低，但若运维过程中快速连续推送相同标签（如 force push），仍可能产生冗余运行。建议添加 concurrency 组以取消进行中的旧运行。
Suggestion:
| on: | |
| push: | |
| tags: | |
| - 'v*' | |
| release: | |
| types: [ published ] | |
| - "v*" | |
| on: | |
| push: | |
| tags: | |
| - "v*" | |
| concurrency: | |
| group: mac-release-${{ github.ref }} | |
| cancel-in-progress: true |
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: "3.10" |
There was a problem hiding this comment.
依赖安装步骤未利用缓存（如 actions/cache 或 actions/setup-python 内置的 cache: uv）。每次运行都会重新解析和下载所有依赖包，延长构建时间并增加网络失败风险。建议启用 setup-python 的内置 uv 缓存。
Suggestion:
| - name: Set up Python | |
| uses: actions/setup-python@v6 | |
| with: | |
| python-version: "3.10" | |
| - name: Set up Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.10" | |
| cache: pip |
| - name: Upload packages | ||
| uses: softprops/action-gh-release@v3 |
There was a problem hiding this comment.
上传步骤移除了原有的 if: startsWith(github.ref, 'refs/tags/') 条件保护。虽然当前触发器仅针对标签推送，但显式条件是一种防御性编程实践——如果将来增加 workflow_dispatch 等其他触发器，缺少该条件可能导致非标签触发的构建产物被错误上传到 Release 页面。建议恢复该条件。
Suggestion:
| - name: Upload packages | |
| uses: softprops/action-gh-release@v3 | |
| - name: Upload packages | |
| uses: softprops/action-gh-release@v2 | |
| if: startsWith(github.ref, 'refs/tags/') |
| from src.config import CONFIG | ||
| from src.gui.main import Main |
There was a problem hiding this comment.
该文件顶部文档字符串声明"测试已安装的 wheel 而不导入源码检出"，但第 36-37 行直接从 src.* 导入模块。如果脚本确实运行在已安装 wheel 的环境中（而非源码检出目录），src 不在 sys.path 上，这两行将抛出 ImportError，导致冒烟测试失败。请确认预期运行环境：若始终从项目根目录运行则无实际影响，否则应改为从已安装的包名（如 pdfdirectory）导入。
Suggestion:
| from src.config import CONFIG | |
| from src.gui.main import Main | |
| from pdfdirectory.config import CONFIG | |
| from pdfdirectory.gui.main import Main |
| def fix_column(self): | ||
| header = self.header() | ||
| # Only resize first column | ||
| header.setSectionResizeMode(0, QHeaderView.Stretch) | ||
|
|
||
| def init_connect(self, parents=None): | ||
| super(TreeWidget, self).__init__(parents) | ||
| self.itemPressed.connect(self.close_editor) | ||
| self.itemDoubleClicked.connect(self.item_double_clicked) | ||
| self.add_action("删除", self.item_remove_current) | ||
| self.last_item = None | ||
| self.last_column = None | ||
|
|
||
| # TODO: Fix page num when drop item | ||
| def dropEvent(self, event): | ||
| """""" | ||
| # self.current_item.setText('') | ||
| super(TreeWidget, self).dropEvent(event) | ||
| self.widget.header().setSectionResizeMode(0, QHeaderView.ResizeMode.Stretch) |
There was a problem hiding this comment.
fix_column 方法名有误导性：它实际上只将第一列的 resize 模式设为 Stretch，并不会“修复”所有列。建议重命名为 stretch_first_column 或添加方法文档说明其真实行为，降低维护者误读的可能。
Suggestion:
| def fix_column(self): | |
| header = self.header() | |
| # Only resize first column | |
| header.setSectionResizeMode(0, QHeaderView.Stretch) | |
| def init_connect(self, parents=None): | |
| super(TreeWidget, self).__init__(parents) | |
| self.itemPressed.connect(self.close_editor) | |
| self.itemDoubleClicked.connect(self.item_double_clicked) | |
| self.add_action("删除", self.item_remove_current) | |
| self.last_item = None | |
| self.last_column = None | |
| # TODO: Fix page num when drop item | |
| def dropEvent(self, event): | |
| """""" | |
| # self.current_item.setText('') | |
| super(TreeWidget, self).dropEvent(event) | |
| self.widget.header().setSectionResizeMode(0, QHeaderView.ResizeMode.Stretch) | |
| def stretch_first_column(self): | |
| self.widget.header().setSectionResizeMode(0, QHeaderView.ResizeMode.Stretch) |
| except Exception: | ||
| if temporary_path is not None: | ||
| temporary_path.unlink(missing_ok=True) | ||
| raise |
There was a problem hiding this comment.
except 块中的 unlink 调用若自身抛异常（如权限错误），将遮蔽原始异常，使根因难以定位。建议用 try/except 或 contextlib.suppress(OSError) 包裹，确保原始异常不被覆盖。
Suggestion:
| except Exception: | |
| if temporary_path is not None: | |
| temporary_path.unlink(missing_ok=True) | |
| raise | |
| except Exception: | |
| if temporary_path is not None: | |
| try: | |
| temporary_path.unlink(missing_ok=True) | |
| except OSError: | |
| pass | |
| raise |
| chapter = reader.outline[0] | ||
| section = reader.outline[1][0] |
There was a problem hiding this comment.
pypdf 中嵌套书签的 outline 结构为 [[ParentDest, [ChildDest]]]，而非扁平列表。当前索引 reader.outline[0] 获取到的是 list 而非 Destination（会引发 AttributeError），reader.outline[1] 因只有 1 个顶层条目而越界（IndexError）。应改为 reader.outline[0][0] 获取父书签、reader.outline[0][1][0] 获取子书签。
Suggestion:
| chapter = reader.outline[0] | |
| section = reader.outline[1][0] | |
| chapter = reader.outline[0][0] | |
| section = reader.outline[0][1][0] |
| controller._show_context_menu(widget.pos()) | ||
| assert calls == [] | ||
|
|
||
| item = _tree_item("Current", 1) | ||
| widget.addTopLevelItem(item) | ||
| widget.setCurrentItem(item) | ||
| controller._show_context_menu(widget.pos()) | ||
| assert len(calls) == 1 |
There was a problem hiding this comment.
Directly calling private method _show_context_menu couples the test to internal implementation details. If the method is renamed or refactored, this test breaks unnecessarily. The same behavior (context menu only appears when there is a current item) can be tested by simulating a right-click event through Qt's event system.
Suggestion:
| controller._show_context_menu(widget.pos()) | |
| assert calls == [] | |
| item = _tree_item("Current", 1) | |
| widget.addTopLevelItem(item) | |
| widget.setCurrentItem(item) | |
| controller._show_context_menu(widget.pos()) | |
| assert len(calls) == 1 | |
| # Simulate right-click via the public signal | |
| widget.customContextMenuRequested.emit(widget.pos()) | |
| assert calls == [] | |
| item = _tree_item("Current", 1) | |
| widget.addTopLevelItem(item) | |
| widget.setCurrentItem(item) | |
| widget.customContextMenuRequested.emit(widget.pos()) | |
| assert len(calls) == 1 |
Add automatic page offset inference
…pector, and button hierarchy
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| include: | ||
| - os: ubuntu-latest | ||
| python: "3.9" | ||
| - os: macos-latest | ||
| python: "3.12" | ||
| - os: windows-latest | ||
| python: "3.12" | ||
| runs-on: ${{ matrix.os }} | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v6 | ||
|
|
||
| - uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: ${{ matrix.python }} | ||
|
|
||
| - uses: astral-sh/setup-uv@v8 | ||
|
|
||
| - name: Install Linux Qt runtime | ||
| if: runner.os == 'Linux' | ||
| run: .github/scripts/install-linux-qt-deps.sh | ||
|
|
||
| - name: Install dependencies | ||
| run: uv sync --locked --group build | ||
|
|
||
| - name: Verify generated UI | ||
| if: runner.os == 'Linux' | ||
| shell: bash | ||
| run: | | ||
| uv run python src/gui/ui_to_py.py src/gui/main_ui.ui "${RUNNER_TEMP}/main_ui.py" | ||
| diff -u src/gui/main_ui.py "${RUNNER_TEMP}/main_ui.py" | ||
|
|
||
| - name: Run tests | ||
| shell: bash | ||
| env: | ||
| QT_QPA_PLATFORM: offscreen | ||
| run: uv run pytest -q -m "not e2e" --strict-markers | ||
|
|
||
| - name: Run desktop E2E | ||
| shell: bash | ||
| env: | ||
| QT_QPA_PLATFORM: offscreen | ||
| run: uv run pytest -q -m e2e --strict-markers | ||
|
|
||
| - name: Measure coverage | ||
| if: runner.os == 'Linux' | ||
| shell: bash | ||
| env: | ||
| QT_QPA_PLATFORM: offscreen | ||
| COVERAGE_FILE: ${{ runner.temp }}/.coverage | ||
| run: | | ||
| uv run coverage run -m pytest -q -m "not e2e" --strict-markers | ||
| uv run coverage report | ||
|
|
||
| - name: Build and test installed wheel | ||
| if: runner.os == 'Linux' | ||
| shell: bash | ||
| run: | | ||
| uv build | ||
| uvx twine check dist/* | ||
| uv venv "${RUNNER_TEMP}/package-venv" | ||
| uv pip install --python "${RUNNER_TEMP}/package-venv/bin/python" dist/*.whl | ||
| "${RUNNER_TEMP}/package-venv/bin/python" tests/package_smoke.py "${RUNNER_TEMP}/package-smoke" "${RUNNER_TEMP}/package-venv/bin/pdfdir" | ||
|
|
||
| - name: Build frozen desktop app | ||
| if: runner.os == 'Linux' | ||
| shell: bash | ||
| run: | | ||
| uv run pyinstaller --noconfirm --clean --onefile --windowed \ | ||
| --name PDFdir \ | ||
| --icon "${GITHUB_WORKSPACE}/pdf.ico" \ | ||
| --add-data "${GITHUB_WORKSPACE}/pdf.ico:." \ | ||
| "${GITHUB_WORKSPACE}/run_gui.py" | ||
|
|
||
| - name: Smoke-test frozen desktop app | ||
| if: runner.os == 'Linux' | ||
| shell: bash | ||
| env: | ||
| QT_QPA_PLATFORM: offscreen | ||
| run: dist/PDFdir --smoke-test | ||
|
|
||
| minimum-dependencies: |
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: "3.9" | ||
| - uses: astral-sh/setup-uv@v8 | ||
| - run: bash .github/scripts/install-linux-qt-deps.sh | ||
| - name: Run minimum dependency tests | ||
| env: | ||
| QT_QPA_PLATFORM: offscreen | ||
| run: uv run --python 3.9 --with "pypdf[crypto]==3.17.4" --with "PySide6==6.5.3" pytest -q |
| output_fingerprint = self.output_fingerprint(output_path) | ||
| if output_fingerprint is not None: | ||
| raise OutputTargetChangedError( | ||
| "Output target already exists; refusing to replace it" | ||
| ) |
There was a problem hiding this comment.
save_pdf 的行为从旧的“删除已有目标后重写”变成“目标一旦存在即报错拒绝”，且整个调用链（add_bookmark/save_pdf）都没有提供任何显式覆盖开关。GUI 侧已通过 _next_available_output_path 每次生成新文件名规避了冲突，但 src/cli.py 的 add_directory -> add_bookmark 仍使用固定输出 xxx_new.pdf，对同一 PDF 重复运行命令行（例如修改目录文本后重新生成书签）会直接抛 OutputTargetChangedError 而失败。另外注意这里在判定“已存在即拒绝”之前先对目标文件做了全量 SHA-256 读取，对已达到百 MB 的 PDF 会在报错前引入无谓延迟。建议：a) 先做一次 os.path.exists 快速失败（并发窗口仍由后续 os.link 的 FileExistsError 兜住，TOCTOU 保护不受影响）；b) 为需要覆盖的历史调用方提供显式的允许覆盖/新输出名策略。
| # Hard-linking is an atomic create-if-absent operation on the | ||
| # same filesystem. Unlike os.replace(), it cannot overwrite a | ||
| # file created in the gap after our final check. | ||
| os.link(temporary_path, output_path) |
There was a problem hiding this comment.
提交阶段只使用 os.link 硬链接且没有其它回退。两点兼容性隐患：1) 临时文件由 tempfile.mkstemp 创建，权限固定为 0600，硬链接共享同一 inode，因此最终输出文件在 POSIX 系统上不再是旧实现 open(path, "wb") 按 umask 生成的 0644 权限——在共享/多用户目录中生成的 PDF 会变成仅属主可读（Windows NTFS 下 mode 位无意义可忽略）；2) 在不支持硬链接的文件系统（FAT/exFAT、部分 Samba/网盘同步目录）上 os.link 会直接抛 OSError，整个保存流程无回退地失败。建议在 os.link 前对临时文件按 umask 修正权限（os.chmod），并在 os.link 不支持时提供基于 os.replace 的显式覆盖路径作为兜底。
| $env:QT_QPA_PLATFORM = "offscreen" | ||
| .\pdfdir.exe --smoke-test | ||
| .\pdfdir_folder\pdfdir_folder.exe --smoke-test |
There was a problem hiding this comment.
Windows smoke test 步骤可能无法让失败的 smoke test 使该步骤变红。PowerShell 脚本默认不会因原生程序返回非零退出码而失败(非零码只写入 $LASTEXITCODE,需显式检查),而且这里顺序执行两个 exe,前一个失败会被后一个成功覆盖,--noconsole 的 GUI-subsystem 程序在部分调用环境下也不保证调用方同步等待。该发布流水线以 fail-closed 为核心设计,若此处静默吞掉失败,损坏的 Windows 产物会进入 publish 环节。建议在每条命令后显式检查 $LASTEXITCODE 并退出。
Suggestion:
| $env:QT_QPA_PLATFORM = "offscreen" | |
| .\pdfdir.exe --smoke-test | |
| .\pdfdir_folder\pdfdir_folder.exe --smoke-test | |
| $env:QT_QPA_PLATFORM = "offscreen" | |
| .\pdfdir.exe --smoke-test | |
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | |
| .\pdfdir_folder\pdfdir_folder.exe --smoke-test | |
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } |
| bash .github/scripts/install-linux-qt-deps.sh | ||
| uv pip install --system -r requirements.txt -r requirements_dev.txt |
There was a problem hiding this comment.
仓库已提交 uv.lock,且 test.yml 用 uv sync --locked 保证依赖与锁文件一致,但 release.yml 的三个 build 作业全部用 uv pip install --system -r requirements.txt -r requirements_dev.txt 按范围裸解析安装(requirements 只是 pip 兼容文件,含上限但未锁定精确版本)。这会让发布产物基于与 CI 锁定验证不一致的依赖集构建,削弱可复现性。建议改为 uv sync --locked --group build(后续命令相应使用 uv run),或从 uv.lock 导出精确版本。
| uses: softprops/action-gh-release@v3 | ||
| with: |
There was a problem hiding this comment.
第三方 action(astral-sh/setup-uv@v8、softprops/action-gh-release@v3)以可变 tag 引用,未固定到完整 commit SHA;tag 可被上游重指向,存在供应链篡改风险。另外 tests/test_release_workflows.py 把 astral-sh/setup-uv@v8、softprops/action-gh-release@v3 作为契约断言,后续若改为 SHA 固定需同步更新这些测试(建议测试改用正则校验前缀,如 astral-sh/setup-uv@)以免加固受阻。
| self.setEditTriggers( | ||
| self.editTriggers() | ||
| | QAbstractItemView.DoubleClicked | ||
| | QAbstractItemView.EditKeyPressed | ||
| ) |
There was a problem hiding this comment.
这里同时开启了 Qt 原生编辑触发器（DoubleClicked/EditKeyPressed），但下方仍保留旧的持久编辑器交互链路（itemPressed→close_editor、itemDoubleClicked→openPersistentEditor/closePersistentEditor、last_item/last_column 记忆），两套编辑机制叠加：QAbstractItemView 处理鼠标双击时会先发出 itemDoubleClicked 信号（触发 openPersistentEditor），随后又因 DoubleClicked 触发器调用 edit() 再打开一个原生内联编辑器，同一单元格可能出现两个编辑器重叠；F2 分支手动 editItem 与 EditKeyPressed 触发器以及持久编辑器的 last_item/last_column 状态也会互相干扰，导致双击开关编辑、F2/双击编辑的行为不一致（last_item/last_column 复位逻辑失真）。建议只保留一种机制：若继续以持久编辑器交互为主，就不要追加 DoubleClicked/EditKeyPressed 触发器；若改用 Qt 原生编辑，则应移除 openPersistentEditor/close_editor/last_item 相关自定义逻辑。
| name: test | ||
|
|
||
| on: | ||
| push: | ||
| pull_request: |
There was a problem hiding this comment.
test.yml 未在顶层声明 permissions,GITHUB_TOKEN 将沿用仓库默认权限(push 事件下可能为 write 级)。该流水线各作业只读代码、装依赖、跑测试,并不需要任何写权限,建议按最小权限原则显式声明 permissions: contents: read,缩小凭证暴露面。
Suggestion:
| name: test | |
| on: | |
| push: | |
| pull_request: | |
| name: test | |
| on: | |
| push: | |
| pull_request: | |
| permissions: | |
| contents: read |
| jobs: | ||
| desktop: | ||
| strategy: | ||
| fail-fast: false |
There was a problem hiding this comment.
各作业均未设置 timeout-minutes。本流水线包含跨平台矩阵、桌面 GUI/E2E 测试与 PyInstaller 打包,一旦 GUI 用例挂起或依赖安装卡住,作业会一直占用 runner 直至 GitHub 默认上限(数小时)。建议为每个 job 设置合理的超时(如 desktop 45-60 分钟、minimum-dependencies 15-20 分钟)以便尽早失败释放资源。
| jobs: | ||
| build-linux: | ||
| name: Build Linux | ||
| runs-on: ubuntu-22.04 |
There was a problem hiding this comment.
所有 job(含 4 个 build 变体与 publish)均未设置 timeout-minutes。发布流水线包含依赖安装、pytest、多次 PyInstaller 打包,以及 macOS 的签名/notarytool --wait/stapler 等可能长时间等待的步骤,一旦某步卡住会一直占用 runner 直到 GitHub 默认上限。建议为各 job 设置显式超时(如 build 60 分钟、publish 20 分钟),与整体 fail-closed 设计保持一致地尽早失败。
| self.setDragDropMode(QAbstractItemView.InternalMove) | ||
| self.setDefaultDropAction(Qt.MoveAction) | ||
| self.setDragEnabled(True) | ||
| self.setAcceptDrops(True) |
There was a problem hiding this comment.
迁移到 PySide6/新 UI 后丢失了旧 main_ui.ui 中对该树的 setSelectionMode(QAbstractItemView.ExtendedSelection) 设置，而这里也只在 init_connect 中配置了拖拽/删除行为、未设置选择模式。QTreeWidget 默认 selectionMode 为 SingleSelection，导致用户无法通过 Ctrl/Shift 多选节点，下方 item_remove_current 中专门针对多选实现的“去除父子重复、只删除顶层选中祖先”逻辑（以及批量删除/批量拖拽）实际永远不会生效，属于功能回归。若仍需要多选删除/拖拽，建议在 init_connect 中补回 self.setSelectionMode(QAbstractItemView.ExtendedSelection)。
Suggestion:
| self.setDragDropMode(QAbstractItemView.InternalMove) | |
| self.setDefaultDropAction(Qt.MoveAction) | |
| self.setDragEnabled(True) | |
| self.setAcceptDrops(True) | |
| self.setDragDropMode(QAbstractItemView.InternalMove) | |
| self.setSelectionMode(QAbstractItemView.ExtendedSelection) | |
| self.setDefaultDropAction(Qt.MoveAction) | |
| self.setDragEnabled(True) | |
| self.setAcceptDrops(True) |
| if os.path.isdir(_documents_folder) | ||
| else os.path.expanduser("~") | ||
| ) | ||
| SELECTED_LEVEL = 0 |
There was a problem hiding this comment.
移除 config.ini 读取逻辑后，SELECTED_LEVEL 变成了一个恒为 0 且全仓库无任何读取点的常量（src/、tests/ 中都不存在 CONFIG.SELECTED_LEVEL 的引用，main.py 也只用到 VERSION/DEFAULT_FOLDER/WINDOW_ICON 等）。这属于新引入的死代码：后续读者容易误以为「层级选择」仍在被持久化，而实际上既无写入也无读取路径。建议直接删除该属性，或改由 QSettings 等真实持久化机制读写，避免留下误导性的空壳常量。
| jobs: | ||
| desktop: |
There was a problem hiding this comment.
该工作流未声明 permissions，GITHUB_TOKEN 将沿用仓库默认权限（可能是读写）。它会在 push/pull_request 上检出并执行代码、构建产物，缺少最小权限声明会放大被注入代码或受污染第三方 action 的影响面；仓库内其它工作流（release.yml、open-code-review.yml）都显式声明了 permissions: contents: read，建议保持一致。另外该工作流也没有 concurrency，同一分支连续 push 会并发跑完整的三平台桌面矩阵（含 E2E、覆盖率、PyInstaller 构建），建议补充 concurrency 组以减少重复的 runner 消耗。
Suggestion:
| jobs: | |
| desktop: | |
| +permissions: | |
| + contents: read | |
| + | |
| +jobs: | |
| + desktop: |
| gh release create "$RELEASE_VERSION" --draft --verify-tag --generate-notes --title "PDFdir ${RELEASE_VERSION}" --repo "$GITHUB_REPOSITORY" | ||
|
|
||
| - name: Upload verified assets to the draft | ||
| uses: softprops/action-gh-release@v3 |
There was a problem hiding this comment.
第三方 action 只固定到可变 tag（astral-sh/setup-uv@v8 在三个构建 job 各出现一次，softprops/action-gh-release@v3 出现在持有 contents: write 的 publish job），tag 可被上游移动或劫持，一旦上游被污染会直接影响发布产物；仓库既有的 .github/workflows/open-code-review.yml 已经把第三方 action 固定到完整 commit SHA（并注明上游提交日期）。建议这里也改为固定 SHA（含 test.yml 中的 astral-sh/setup-uv@v8），并同步更新约束这些字符串的测试。
| output_dir = os.path.dirname(output_path) | ||
| output_name = os.path.basename(output_path) | ||
| output_fingerprint = self.output_fingerprint(output_path) | ||
| if output_fingerprint is not None: |
There was a problem hiding this comment.
新增的“目标已存在就拒绝写入”策略与默认输出路径冲突时会破坏重复转换。_new_path（pdf.py:64-68）在未传 output_path 时是确定性的 <name>_new.pdf，旧实现是 os.remove 后覆盖，现在则直接抛 OutputTargetChangedError。GUI 的 PdfWriteWorker 会先通过 _next_available_output_path 选一个不存在的路径，所以不受影响；但 pdfdirectory.add_directory()（CLI 入口 src/cli.py 使用）调用 add_bookmark 时没有传 output_path，因此对同一源文件第二次运行 CLI 会直接报错而不是覆盖更新。建议让 add_directory/CLI 也选择可用输出路径（或在 add_bookmark 内做同样处理），否则这是一处用户可见的功能回归。
| output_path = os.path.abspath(self._new_path) | ||
| output_dir = os.path.dirname(output_path) | ||
| output_name = os.path.basename(output_path) | ||
| output_fingerprint = self.output_fingerprint(output_path) |
There was a problem hiding this comment.
output_fingerprint() 会打开目标文件并对其做完整 SHA-256（大文件等于整文件读盘），但这里的调用只判断 is not None，摘要值与 stat 变化检测结果都没有被使用；enforce_output_fingerprint 分支在“先判存在”之后实际不可达，expected_output_fingerprint 也仅被当作布尔量使用，_output_fingerprint 别名同样没有调用方。若只需要“已存在就拒绝”，用 os.path.lexists() 即可，可省掉整文件哈希；否则应真正用指纹做比较，避免这段校验逻辑形同虚设。
| if self._close_requested: | ||
| return | ||
| self.show_status(self._t("generation_failed"), 5000) | ||
| if "Output target changed" in message: |
There was a problem hiding this comment.
此处通过匹配后端异常文案（"Output target changed"）来区分错误类型，_friendly_recognition_error 中同样用 "tesseract"/"pymupdf" 等子串匹配 OCR 后端报错。这类写法把 UI 逻辑与后端具体英文措辞强耦合：一旦后端错误消息被改写或本地化，就会静默退回到通用错误分支，用户看不到准确提示。建议由后端返回结构化的错误码/异常类型（如专用 BookmarkPageError 那样的 reason），再由 UI 做映射。
| - name: Install system and project dependencies | ||
| run: | | ||
| bash .github/scripts/install-linux-qt-deps.sh | ||
| uv pip install --system -r requirements.txt -r requirements_dev.txt |
There was a problem hiding this comment.
三个构建作业（第 38、95、167 行）都用 requirements*.txt 安装依赖，但 requirements.txt（自称 “pyproject.toml is the canonical source” 的兼容副本）缺少 pyproject.toml / uv.lock 中声明的 requests>=2.28,<3：src/updater.py 在模块顶层 import requests，而 src/gui/main.py 又顶层 from src.updater import check_for_update，所以该环境里没有 requests 时，python -m pytest -q（tests/test_updater.py 直接 import requests）以及 PyInstaller 冻结包的 --smoke-test 都会以 ModuleNotFoundError 失败（除非 runner 恰好自带 requests）——即发布流程会在“Test/冒烟”阶段就中断，或产出缺少 requests 的冻结包。建议让 release 构建使用规范依赖源（uv sync --locked --group build 后统一 uv run pytest / uv run pyinstaller，与 readme.md、export_exe.bat、test.yml 保持一致），或至少把 requests>=2.28,<3 补进 requirements.txt，并加一条测试保证 requirements*.txt 与 pyproject.toml 的运行时依赖同步。
| idnum = o.page if isinstance(o.page, int) else o.page.idnum | ||
| title = " " * current_level + o.title.strip() | ||
| page_num = self.pages_num[idnum] + 1 | ||
| page_num = self.reader.get_destination_page_number(o) + 1 |
There was a problem hiding this comment.
exist_bookmarks 改为使用 reader.get_destination_page_number() 后，self.pages_num 在整个仓库中已无任何读取方：它只在 __init__ 里由 _get_pages_num()（遍历全部页、构建 idnum→page_number 字典）赋值，成为只写不读的死状态。这不仅是死代码，也让每次构造 Pdf（例如 check_bookmarks 的校验路径）都白白遍历一遍所有页。若它已不再作为对外 API 使用，建议连同 self.pages_num 赋值一并清理（此时 _get_pages_num 也只剩测试引用）；若需保留，请补充注释说明其用途，避免后续误以为仍有消费方。
|
|
||
| @staticmethod | ||
| def dict_to_pdf(pdf_path, index_dict, keep_exist_dir=False): | ||
| return add_bookmark(pdf_path, index_dict, keep_exist_dir) | ||
| def dict_to_pdf( |
There was a problem hiding this comment.
dict_to_pdf 的唯一调用点（旧 write_tree_to_pdf 中的 self.dict_to_pdf(...)）已被 PdfWriteWorker 取代。全仓库检索确认该方法现在没有任何引用（含 src/、tests/、.github/），而本次改动仍为它新增了 cancel_check 形参，属于改造后遗留的死代码。建议删除该方法；若它是有意保留的对外 API，建议补上测试或在注释中说明保留原因，避免形参与真实调用路径长期脱节。
| "page_below_minimum": "书签页码 {page} 小于 1，请在预览中修正", | ||
| "page_above_maximum": "书签页码 {page} 超出 PDF 总页数 {total}，请在预览中修正", | ||
| "output_changed": "输出位置已被其他程序占用，PDFdir 未覆盖该文件。请检查后重试，应用会改用新的编号文件名。", | ||
| "generation_cancelled": "已取消生成 PDF", |
There was a problem hiding this comment.
该文案表条目（中英文各一份）没有任何 _t() 引用，属于死条目：
advanced_mode：层级模式标签已改为在_apply_language的static_text中硬编码，此键未使用；keep_source_bookmarks：keep_exist_dir_box的文案同样来自static_text（“Keep source PDF bookmarks”），与字典中的同义条目重复，存在漂移风险；generation_cancelled：取消统一走_task_cancelled()→task_cancelled，此键未使用；generated：生成完成后使用的是generated_ready，此键未使用。
建议清理这 4 组条目（或改为真正引用它们），否则翻译维护者会误以为需要同步维护这些字符串。
3 participants
No description provided.