feat: complete standalone GUI for mtkclient - #1
Open
sudotsu wants to merge 3 commits into
Open
Conversation
12-tab modular PySide6 GUI with plugin system: - Device, Read, Write, Erase, Keys, Bootloader, Memory, RPMB, IMEI, Exploit, eFuse, Server - All buttons wired to mtkclient backend via MtkWrapper facade - GuiSignalProxy bridges mtkclient logging to Qt signals - USB polling and partition loading run off the main thread - Button-disable-during-operations prevents concurrent conflicts - Destructive operations gated through ConfirmationDialog - Dark/light theme support with QSS stylesheets - Plugin system with hot-loadable MtkPlugin subclasses - PyInstaller spec for Windows executable build Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change adds the first MTK-GUI desktop application. It includes startup, packaging, an ChangesMTK-GUI application
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant DeviceManager
participant Worker
participant MtkWrapper
User->>MainWindow: Start a device operation
MainWindow->>DeviceManager: create_worker(operation)
DeviceManager->>Worker: start(operation)
Worker->>MtkWrapper: execute backend method
MtkWrapper-->>Worker: progress and operation result
Worker-->>MainWindow: finished(result) or error
MainWindow-->>User: update panels and operation controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
PR Summary by QodoStandalone PySide6 GUI for mtkclient (tabs, plugins, themes, packaging)
AI Description
Diagram
High-Level Assessment
Files changed (41)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@mtk_gui/ui/tabs/exploit_tab.py`:
- Around line 155-159: Update the stage_addr method to parse every non-empty
entered address with base 16, including values without a 0x prefix; preserve the
existing default address for empty input.
In `@mtk_gui/ui/tabs/imei_tab.py`:
- Around line 51-60: The IMEI write handlers and both seed/AES-key operation
handlers currently start workers with unchecked input. Add pre-operation
validation in _on_imei_write and the corresponding seed/AES-key handlers:
require IMEI values to contain exactly 15 digits, and require hexadecimal inputs
to be valid, even-length strings before calling bytes.fromhex() or starting a
worker. Display the existing UI validation error mechanism and return
immediately on invalid input, preserving normal worker startup for valid values.
In `@mtk_gui/ui/tabs/memory_tab.py`:
- Around line 148-153: Update get_peek_params to reject empty or invalid
hexadecimal address and length fields instead of defaulting them to zero, while
preserving the registers flag. In mtk_gui/main_window.py._on_peek, catch and
handle validation errors before starting the worker or progress UI, presenting
the existing appropriate user-facing error feedback.
In `@mtk_gui/ui/tabs/read_tab.py`:
- Around line 177-193: Update get_read_params and the read-operation caller to
validate parameters before invoking the backend: catch invalid hexadecimal
offset/length values, reject empty output filenames for file-based reads, and
reject zero or otherwise invalid lengths. Show a user-facing validation error
and stop the operation when validation fails, while preserving valid
mode-specific parameter construction.
In `@mtk_gui/ui/tabs/rpmb_tab.py`:
- Around line 51-75: Update RpmbTab’s write flow to expose a validated getter
for the write sector, sector count, and input file, then have the write handler
pass those values to wrapper.write_rpmb. Ensure validation rejects invalid or
missing parameters and preserve compatibility with the backend method’s expected
argument format.
🪄 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: 84c79f03-f869-4b92-95b9-be0d795126b2
📒 Files selected for processing (41)
mtk-gui.specmtk_gui/__init__.pymtk_gui/app.pymtk_gui/backend/__init__.pymtk_gui/backend/device_manager.pymtk_gui/backend/log_interceptor.pymtk_gui/backend/mtk_wrapper.pymtk_gui/backend/worker.pymtk_gui/constants.pymtk_gui/main_window.pymtk_gui/plugins/__init__.pymtk_gui/plugins/base_plugin.pymtk_gui/plugins/loader.pymtk_gui/resources/__init__.pymtk_gui/theme/__init__.pymtk_gui/theme/colors.pymtk_gui/theme/dark.qssmtk_gui/theme/light.qssmtk_gui/ui/__init__.pymtk_gui/ui/tabs/__init__.pymtk_gui/ui/tabs/bootloader_tab.pymtk_gui/ui/tabs/device_tab.pymtk_gui/ui/tabs/efuse_tab.pymtk_gui/ui/tabs/erase_tab.pymtk_gui/ui/tabs/exploit_tab.pymtk_gui/ui/tabs/imei_tab.pymtk_gui/ui/tabs/keys_tab.pymtk_gui/ui/tabs/memory_tab.pymtk_gui/ui/tabs/read_tab.pymtk_gui/ui/tabs/rpmb_tab.pymtk_gui/ui/tabs/server_tab.pymtk_gui/ui/tabs/write_tab.pymtk_gui/ui/widgets/__init__.pymtk_gui/ui/widgets/confirmation_dialog.pymtk_gui/ui/widgets/hex_viewer.pymtk_gui/ui/widgets/log_panel.pymtk_gui/ui/widgets/partition_list.pymtk_gui/ui/widgets/progress_panel.pymtk_gui/ui/widgets/serial_port_dialog.pyplugins/example_plugin.pyrun.py
👮 Files not reviewed due to content moderation or server errors (15)
- mtk_gui/ui/widgets/confirmation_dialog.py
- mtk_gui/ui/widgets/log_panel.py
- mtk_gui/ui/widgets/progress_panel.py
- mtk_gui/ui/tabs/device_tab.py
- mtk_gui/ui/tabs/bootloader_tab.py
- mtk_gui/ui/tabs/write_tab.py
- mtk_gui/ui/tabs/erase_tab.py
- mtk_gui/main_window.py
- mtk_gui/init.py
- mtk_gui/theme/colors.py
- mtk_gui/backend/init.py
- mtk_gui/backend/log_interceptor.py
- mtk_gui/backend/worker.py
- mtk_gui/backend/mtk_wrapper.py
- mtk_gui/backend/device_manager.py
Comment on lines
+51
to
+60
| w1.addWidget(QLabel("IMEI 1:")) | ||
| self._imei1_input = QLineEdit() | ||
| self._imei1_input.setPlaceholderText("15-digit IMEI") | ||
| self._imei1_input.setMaxLength(15) | ||
| w1.addWidget(self._imei1_input, 1) | ||
| w1.addWidget(QLabel("IMEI 2:")) | ||
| self._imei2_input = QLineEdit() | ||
| self._imei2_input.setPlaceholderText("15-digit IMEI (optional)") | ||
| self._imei2_input.setMaxLength(15) | ||
| w1.addWidget(self._imei2_input, 1) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate IMEI and hexadecimal input before an operation starts.
setMaxLength(15) accepts short and non-numeric IMEIs. _on_imei_write forwards every non-empty value to wrapper.write_imei. Both IMEI handlers also pass these raw seed and AES-key strings to bytes.fromhex(), which raises ValueError for malformed or odd-length input.
Require a 15-digit IMEI and valid even-length hexadecimal values. Show a validation error before starting the worker.
Also applies to: 147-154
🤖 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 `@mtk_gui/ui/tabs/imei_tab.py` around lines 51 - 60, The IMEI write handlers
and both seed/AES-key operation handlers currently start workers with unchecked
input. Add pre-operation validation in _on_imei_write and the corresponding
seed/AES-key handlers: require IMEI values to contain exactly 15 digits, and
require hexadecimal inputs to be valid, even-length strings before calling
bytes.fromhex() or starting a worker. Display the existing UI validation error
mechanism and return immediately on invalid input, preserving normal worker
startup for valid values.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @sudotsu. The following files were modified: * `mtk_gui/app.py` * `mtk_gui/backend/device_manager.py` * `mtk_gui/backend/log_interceptor.py` * `mtk_gui/backend/mtk_wrapper.py` * `mtk_gui/backend/worker.py` * `mtk_gui/main_window.py` * `mtk_gui/plugins/base_plugin.py` * `mtk_gui/plugins/loader.py` * `mtk_gui/theme/__init__.py` * `mtk_gui/ui/tabs/bootloader_tab.py` * `mtk_gui/ui/tabs/device_tab.py` * `mtk_gui/ui/tabs/efuse_tab.py` * `mtk_gui/ui/tabs/erase_tab.py` * `mtk_gui/ui/tabs/exploit_tab.py` * `mtk_gui/ui/tabs/imei_tab.py` * `mtk_gui/ui/tabs/keys_tab.py` * `mtk_gui/ui/tabs/memory_tab.py` * `mtk_gui/ui/tabs/read_tab.py` * `mtk_gui/ui/tabs/rpmb_tab.py` * `mtk_gui/ui/tabs/server_tab.py` * `mtk_gui/ui/tabs/write_tab.py` * `mtk_gui/ui/widgets/confirmation_dialog.py` * `mtk_gui/ui/widgets/hex_viewer.py` * `mtk_gui/ui/widgets/log_panel.py` * `mtk_gui/ui/widgets/partition_list.py` * `mtk_gui/ui/widgets/progress_panel.py` * `mtk_gui/ui/widgets/serial_port_dialog.py` * `plugins/example_plugin.py` These file types are not supported: * `mtk-gui.spec` * `mtk_gui/theme/dark.qss` * `mtk_gui/theme/light.qss`
Backend hardening: - Replace sys.exit monkey-patching with SystemExit catch in all wrapper methods - Single worker for multi-partition reads (no concurrent DA operations) - Worker emits exactly one terminal signal (finished/error/cancelled) - USB poll thread joined on shutdown, shutdown flag prevents post-close signals - Fix b"" * 32 → b"\x00" * 32 in nvitem_crypt/read_imei/write_imei defaults - Plugin loader rejects relative dirs when APPDATA is missing Input validation: - IMEI: 15-digit check, hex seed/aeskey even-length validation - Memory peek/poke: reject empty/invalid hex addresses - Read tab: validate offset/length/filename before backend call - RPMB write: require input file, validate sector params - Exploit stage_addr: always parse as hex UX polish: - Dismissable warning banners (erase/write tabs) with QSettings persistence - 40 hover tooltips across all 12 tabs - Expanded Help menu with mtkclient docs, wiki, XDA links - Expanded About dialog with version and feature summary - README.md with project rationale, install, build, and plugin docs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mtk_gui/ui/tabs/rpmb_tab.py (1)
93-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConnect the RPMB authentication control to an operation.
The key field has no button, accessor, or main-window handler.
MtkWrapper.auth_rpmbis therefore unreachable from the GUI. Add an Authenticate action that validates exactly 32 bytes of hexadecimal input and runsauth_rpmbin a tracked worker.🤖 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 `@mtk_gui/ui/tabs/rpmb_tab.py` around lines 93 - 103, Extend the RPMB authentication UI around _auth_key with an Authenticate action, validate that the entered value is exactly 32 bytes (64 hexadecimal characters), and expose the action through the tab’s existing worker-tracking flow. Add the corresponding main-window handler to read the key and invoke MtkWrapper.auth_rpmb via a tracked worker, reporting validation or operation errors through the established GUI feedback path.mtk_gui/ui/tabs/erase_tab.py (1)
116-130: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate sector erase ranges before dispatch.
EraseTab.get_erase_params()turns empty sector fields into zero and accepts negative values or zero-length ranges.MtkWrapper.erase_sectors()forwards those values toda_handler.da_ess(). Validate start as nonnegative and sector count as positive before callingerase_sectors.🤖 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 `@mtk_gui/ui/tabs/erase_tab.py` around lines 116 - 130, Update EraseTab.get_erase_params() to validate sector-mode inputs before returning them: require a nonnegative starting sector and a strictly positive sector count, rejecting empty, negative, or zero-length ranges. Ensure invalid values cannot reach MtkWrapper.erase_sectors() or da_handler.da_ess(), while preserving partition-mode behavior.mtk_gui/plugins/base_plugin.py (1)
33-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate returned metadata on connected state.
DeviceInfohas no__bool__, soif dm.device_infois always true even for the empty pre-connection instance. Do not return populated fields when the backend state is notDeviceState.CONNECTED; this preserves the documented contract to return{}when device information is unavailable.🤖 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 `@mtk_gui/plugins/base_plugin.py` around lines 33 - 52, Update get_device_info to require the backend state to be DeviceState.CONNECTED before reading and returning fields from dm.device_info; otherwise return {}. Preserve the existing metadata mapping for connected devices and use the existing DeviceState symbol.
🤖 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 `@mtk_gui/backend/mtk_wrapper.py`:
- Around line 183-187: Contain partition output paths in the loop that builds
filenames before calling da_read: resolve both the selected directory and the
candidate path, then reject any candidate not contained within the resolved
output directory. Preserve valid partition filenames and only invoke da_read for
validated paths.
In `@mtk_gui/backend/worker.py`:
- Around line 47-54: Make cancellation stop work before reporting it: in
mtk_gui/backend/worker.py lines 47-54, use a thread-safe cancellation primitive
and check it before invoking the worker function, while preserving the existing
cancellation signal behavior; in mtk_gui/backend/mtk_wrapper.py lines 177-190,
update read_selected_partitions to return immediately when worker.is_cancelled
is set, checking both before GPT dumping and before each partition read.
In `@mtk_gui/plugins/base_plugin.py`:
- Around line 20-27: The add_menu_item documentation in add_menu_item uses an
incorrect root-prefixed example, causing callers to target a nested Plugins
menu. Update the documented example to be relative to
MainWindow.add_plugin_menu_item’s existing Plugins root, and add a test covering
the documented menu path.
- Around line 16-18: Update PluginContext.add_tab and MainWindow.add_plugin_tab
to accept and forward the optional icon; when an icon is provided, pass it to
the underlying tab widget’s addTab call, while preserving the existing behavior
when no icon is supplied.
In `@mtk_gui/ui/tabs/read_tab.py`:
- Around line 204-224: Update the mode-specific input validation around the
offset and sector-start parsing to reject values below zero before assigning
them to params. In the mode handling logic, validate the parsed offset and start
sector alongside the existing length and sector-count checks, raising the
established ValueError pattern for negative values while preserving valid zero
and positive inputs.
In `@mtk_gui/ui/tabs/rpmb_tab.py`:
- Around line 155-165: Update get_write_params to validate the parsed sector and
sectors values before returning them: reject negative sector values and sector
counts that are zero or negative by raising ValueError, while preserving the
existing defaults for omitted fields.
In `@README.md`:
- Around line 55-70: Update the fenced directory-tree block in the README around
the mtk_gui structure to specify the text language identifier, changing the
opening fence to use text while leaving the tree content unchanged.
---
Outside diff comments:
In `@mtk_gui/plugins/base_plugin.py`:
- Around line 33-52: Update get_device_info to require the backend state to be
DeviceState.CONNECTED before reading and returning fields from dm.device_info;
otherwise return {}. Preserve the existing metadata mapping for connected
devices and use the existing DeviceState symbol.
In `@mtk_gui/ui/tabs/erase_tab.py`:
- Around line 116-130: Update EraseTab.get_erase_params() to validate
sector-mode inputs before returning them: require a nonnegative starting sector
and a strictly positive sector count, rejecting empty, negative, or zero-length
ranges. Ensure invalid values cannot reach MtkWrapper.erase_sectors() or
da_handler.da_ess(), while preserving partition-mode behavior.
In `@mtk_gui/ui/tabs/rpmb_tab.py`:
- Around line 93-103: Extend the RPMB authentication UI around _auth_key with an
Authenticate action, validate that the entered value is exactly 32 bytes (64
hexadecimal characters), and expose the action through the tab’s existing
worker-tracking flow. Add the corresponding main-window handler to read the key
and invoke MtkWrapper.auth_rpmb via a tracked worker, reporting validation or
operation errors through the established GUI feedback path.
🪄 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: 118ef602-016d-42b7-baf0-3af6c6b131ed
📒 Files selected for processing (30)
README.mdmtk_gui/app.pymtk_gui/backend/device_manager.pymtk_gui/backend/log_interceptor.pymtk_gui/backend/mtk_wrapper.pymtk_gui/backend/worker.pymtk_gui/main_window.pymtk_gui/plugins/base_plugin.pymtk_gui/plugins/loader.pymtk_gui/theme/__init__.pymtk_gui/ui/tabs/bootloader_tab.pymtk_gui/ui/tabs/device_tab.pymtk_gui/ui/tabs/efuse_tab.pymtk_gui/ui/tabs/erase_tab.pymtk_gui/ui/tabs/exploit_tab.pymtk_gui/ui/tabs/imei_tab.pymtk_gui/ui/tabs/keys_tab.pymtk_gui/ui/tabs/memory_tab.pymtk_gui/ui/tabs/read_tab.pymtk_gui/ui/tabs/rpmb_tab.pymtk_gui/ui/tabs/server_tab.pymtk_gui/ui/tabs/write_tab.pymtk_gui/ui/widgets/confirmation_dialog.pymtk_gui/ui/widgets/dismissable_banner.pymtk_gui/ui/widgets/hex_viewer.pymtk_gui/ui/widgets/log_panel.pymtk_gui/ui/widgets/partition_list.pymtk_gui/ui/widgets/progress_panel.pymtk_gui/ui/widgets/serial_port_dialog.pyplugins/example_plugin.py
🚧 Files skipped from review as they are similar to previous changes (22)
- mtk_gui/theme/init.py
- plugins/example_plugin.py
- mtk_gui/plugins/loader.py
- mtk_gui/backend/log_interceptor.py
- mtk_gui/ui/widgets/hex_viewer.py
- mtk_gui/app.py
- mtk_gui/ui/tabs/server_tab.py
- mtk_gui/ui/widgets/serial_port_dialog.py
- mtk_gui/ui/widgets/confirmation_dialog.py
- mtk_gui/ui/widgets/progress_panel.py
- mtk_gui/ui/widgets/partition_list.py
- mtk_gui/ui/tabs/memory_tab.py
- mtk_gui/ui/tabs/bootloader_tab.py
- mtk_gui/ui/tabs/efuse_tab.py
- mtk_gui/ui/tabs/write_tab.py
- mtk_gui/backend/device_manager.py
- mtk_gui/ui/tabs/device_tab.py
- mtk_gui/ui/tabs/imei_tab.py
- mtk_gui/ui/tabs/keys_tab.py
- mtk_gui/ui/tabs/exploit_tab.py
- mtk_gui/ui/widgets/log_panel.py
- mtk_gui/main_window.py
Comment on lines
+183
to
+187
| for name in partitions: | ||
| filename = os.path.join(directory, f"{name}.bin") | ||
| try: | ||
| results[name] = bool(self.da_handler.da_read( | ||
| name, parttype, filename, display=True)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Contain partition output paths.
At Line 184, name originates from device GPT metadata. A partition name such as ../../target or /target escapes directory through os.path.join. A malicious connected device can make da_read write outside the user-selected directory.
Resolve the output path and reject paths outside the resolved output directory.
Proposed fix
+from pathlib import Path
+
+ output_dir = Path(directory).resolve()
for name in partitions:
- filename = os.path.join(directory, f"{name}.bin")
+ filename = (output_dir / f"{name}.bin").resolve()
+ try:
+ filename.relative_to(output_dir)
+ except ValueError:
+ results[name] = False
+ continue
try:
results[name] = bool(self.da_handler.da_read(
- name, parttype, filename, display=True))
+ name, parttype, str(filename), display=True))📝 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.
Suggested change
| for name in partitions: | |
| filename = os.path.join(directory, f"{name}.bin") | |
| try: | |
| results[name] = bool(self.da_handler.da_read( | |
| name, parttype, filename, display=True)) | |
| from pathlib import Path | |
| output_dir = Path(directory).resolve() | |
| for name in partitions: | |
| filename = (output_dir / f"{name}.bin").resolve() | |
| try: | |
| filename.relative_to(output_dir) | |
| except ValueError: | |
| results[name] = False | |
| continue | |
| try: | |
| results[name] = bool(self.da_handler.da_read( | |
| name, parttype, str(filename), display=True)) |
🤖 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 `@mtk_gui/backend/mtk_wrapper.py` around lines 183 - 187, Contain partition
output paths in the loop that builds filenames before calling da_read: resolve
both the selected directory and the candidate path, then reject any candidate
not contained within the resolved output directory. Preserve valid partition
filenames and only invoke da_read for validated paths.
Comment on lines
+47
to
+54
| def run(self): | ||
| """Execute the worker function and emit its result or an error message.""" | ||
| try: | ||
| result = self._func(*self._args, worker=self, **self._kwargs) | ||
| if self._cancelled: | ||
| self.cancelled.emit() | ||
| else: | ||
| self.finished.emit(result) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make cancellation stop work before reporting cancellation.
Worker.cancel() only changes the terminal signal after _func returns. read_selected_partitions then continues to start each remaining partition read. A cancelled operation can therefore continue device I/O while the UI reports cancellation.
mtk_gui/backend/worker.py#L47-L54: Check cancellation before invoking_func. Use a thread-safe cancellation primitive.mtk_gui/backend/mtk_wrapper.py#L177-L190: Checkworker.is_cancelledbefore GPT dumping and before each partition read. Return immediately when cancellation is requested.
📍 Affects 2 files
mtk_gui/backend/worker.py#L47-L54(this comment)mtk_gui/backend/mtk_wrapper.py#L177-L190
🤖 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 `@mtk_gui/backend/worker.py` around lines 47 - 54, Make cancellation stop work
before reporting it: in mtk_gui/backend/worker.py lines 47-54, use a thread-safe
cancellation primitive and check it before invoking the worker function, while
preserving the existing cancellation signal behavior; in
mtk_gui/backend/mtk_wrapper.py lines 177-190, update read_selected_partitions to
return immediately when worker.is_cancelled is set, checking both before GPT
dumping and before each partition read.
Comment on lines
+16
to
+18
| def add_tab(self, widget, title: str, icon=None): | ||
| """Add a tab to the main window's tab widget.""" | ||
| self._app.main_window.add_plugin_tab(widget, title) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'def add_plugin_tab|add_plugin_tab\(|addTab\(' \
mtk_gui/main_window.py mtk_gui/plugins/base_plugin.pyRepository: sudotsu/mtk_gui
Length of output: 3078
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant BasePlugin and MainWindow snippets:\n'
sed -n '1,40p' mtk_gui/plugins/base_plugin.py
printf '\n'
sed -n '850,862p' mtk_gui/main_window.py
printf '\nAll add_tab / add_plugin_tab / add_menu_item usages:\n'
rg -n 'add_tab\(|add_plugin_tab\(|add_menu_item\(' mtk_gui || true
printf '\nQt documentation availability in installed PySide6 (if present):\n'
python3 - <<'PY'
try:
from PySide6 import QtWidgets
import inspect
method = QtWidgets.QTabWidget.addTab
for name, method in inspect.getmembers(QtWidgets.QTabWidget):
if name == "addTab":
print(inspect.signature(method))
print(inspect.getdoc(method))
except Exception as exc:
print(f"PySide6 unavailable or introspection failed: {type(exc).__name__}: {exc}")
PYRepository: sudotsu/mtk_gui
Length of output: 2544
🌐 Web query:
PySide6 QTabWidget addTab default overload append addTab widget icon label
💡 Result:
In PySide6, the QTabWidget.addTab method is overloaded to provide two primary ways to add a tab [1][2]. Both variants automatically append the new tab to the end of the tab list [3]. The available method signatures are: 1. addTab(widget, label): Adds a tab with the specified page widget and text label [1][2]. 2. addTab(widget, icon, label): Adds a tab with the specified page widget, icon, and text label [1][2]. In both cases, the widget passed to the method becomes owned by the QTabWidget [1][2]. If you need to insert a tab at a specific position rather than appending it to the end, you should use the insertTab method, which similarly supports both (widget, label) and (widget, icon, label) signatures [1][2].
Citations:
- 1: https://doc.qt.io/qtforpython-6.10/PySide6/QtWidgets/QTabWidget.html
- 2: https://doc.qt.io/QT-6/qtabwidget.html
- 3: https://github.com/qt/qtbase/blob/85949d70/src/widgets/widgets/qtabwidget.cpp
Forward the optional tab icon.
PluginContext.add_tab accepts an icon, but MainWindow.add_plugin_tab calls self._tabs.addTab(widget, title) and discards it. Add an icon parameter to MainWindow.add_plugin_tab, and pass it through when present to preserve plugin tab icons.
🤖 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 `@mtk_gui/plugins/base_plugin.py` around lines 16 - 18, Update
PluginContext.add_tab and MainWindow.add_plugin_tab to accept and forward the
optional icon; when an icon is provided, pass it to the underlying tab widget’s
addTab call, while preserving the existing behavior when no icon is supplied.
Comment on lines
+20
to
+27
| def add_menu_item(self, menu_path: str, action): | ||
| """Adds an action to the specified plugin menu path. | ||
|
|
||
| Parameters: | ||
| menu_path (str): Menu path for the action, such as ``"Plugins/My Action"``. | ||
| action: Action to add to the menu. | ||
| """ | ||
| self._app.main_window.add_plugin_menu_item(menu_path, action) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the documented menu path with the existing menu root.
At Line 24, the example uses "Plugins/My Action". MainWindow.add_plugin_menu_item already starts at the Plugins menu. The documented value therefore creates Plugins > Plugins > My Action. Document paths relative to the existing menu, or strip the leading Plugins component in the helper. Add a test for the documented example.
🤖 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 `@mtk_gui/plugins/base_plugin.py` around lines 20 - 27, The add_menu_item
documentation in add_menu_item uses an incorrect root-prefixed example, causing
callers to target a nested Plugins menu. Update the documented example to be
relative to MainWindow.add_plugin_menu_item’s existing Plugins root, and add a
test covering the documented menu path.
Comment on lines
+204
to
+224
| length = int(len_text, 16) | ||
| if length <= 0: | ||
| raise ValueError("Length must be greater than zero.") | ||
| params["offset"] = int(offset_text, 16) | ||
| params["length"] = length | ||
| params["filename"] = filename | ||
| elif mode == 3: | ||
| start_text = self._sector_start_input.text().strip() | ||
| count_text = self._sector_count_input.text().strip() | ||
| filename = self._sector_file_input.text().strip() | ||
| if not start_text: | ||
| raise ValueError("Start sector is required.") | ||
| if not count_text: | ||
| raise ValueError("Sector count is required.") | ||
| if not filename: | ||
| raise ValueError("Output filename is required.") | ||
| sectors = int(count_text) | ||
| if sectors <= 0: | ||
| raise ValueError("Sector count must be greater than zero.") | ||
| params["start"] = int(start_text) | ||
| params["sectors"] = sectors |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject negative read positions.
Lines 204 and 223 accept negative offsets and start sectors. These values reach the backend read operations. Reject values below zero before constructing params.
Proposed fix
length = int(len_text, 16)
if length <= 0:
raise ValueError("Length must be greater than zero.")
- params["offset"] = int(offset_text, 16)
+ offset = int(offset_text, 16)
+ if offset < 0:
+ raise ValueError("Offset cannot be negative.")
+ params["offset"] = offset
params["length"] = length
params["filename"] = filename
@@
- params["start"] = int(start_text)
+ start = int(start_text)
+ if start < 0:
+ raise ValueError("Start sector cannot be negative.")
+ params["start"] = start📝 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.
Suggested change
| length = int(len_text, 16) | |
| if length <= 0: | |
| raise ValueError("Length must be greater than zero.") | |
| params["offset"] = int(offset_text, 16) | |
| params["length"] = length | |
| params["filename"] = filename | |
| elif mode == 3: | |
| start_text = self._sector_start_input.text().strip() | |
| count_text = self._sector_count_input.text().strip() | |
| filename = self._sector_file_input.text().strip() | |
| if not start_text: | |
| raise ValueError("Start sector is required.") | |
| if not count_text: | |
| raise ValueError("Sector count is required.") | |
| if not filename: | |
| raise ValueError("Output filename is required.") | |
| sectors = int(count_text) | |
| if sectors <= 0: | |
| raise ValueError("Sector count must be greater than zero.") | |
| params["start"] = int(start_text) | |
| params["sectors"] = sectors | |
| length = int(len_text, 16) | |
| if length <= 0: | |
| raise ValueError("Length must be greater than zero.") | |
| offset = int(offset_text, 16) | |
| if offset < 0: | |
| raise ValueError("Offset cannot be negative.") | |
| params["offset"] = offset | |
| params["length"] = length | |
| params["filename"] = filename | |
| elif mode == 3: | |
| start_text = self._sector_start_input.text().strip() | |
| count_text = self._sector_count_input.text().strip() | |
| filename = self._sector_file_input.text().strip() | |
| if not start_text: | |
| raise ValueError("Start sector is required.") | |
| if not count_text: | |
| raise ValueError("Sector count is required.") | |
| if not filename: | |
| raise ValueError("Output filename is required.") | |
| sectors = int(count_text) | |
| if sectors <= 0: | |
| raise ValueError("Sector count must be greater than zero.") | |
| start = int(start_text) | |
| if start < 0: | |
| raise ValueError("Start sector cannot be negative.") | |
| params["start"] = start | |
| params["sectors"] = sectors |
🤖 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 `@mtk_gui/ui/tabs/read_tab.py` around lines 204 - 224, Update the mode-specific
input validation around the offset and sector-start parsing to reject values
below zero before assigning them to params. In the mode handling logic, validate
the parsed offset and start sector alongside the existing length and
sector-count checks, raising the established ValueError pattern for negative
values while preserving valid zero and positive inputs.
Comment on lines
+155
to
+165
| def get_write_params(self) -> dict: | ||
| """Returns write params. Raises ValueError on invalid input.""" | ||
| filename = self._write_file.text().strip() | ||
| if not filename: | ||
| raise ValueError("Input file is required for RPMB write.") | ||
| sector_text = self._write_sector.text().strip() | ||
| sectors_text = self._write_sectors.text().strip() | ||
| return { | ||
| "filename": filename, | ||
| "sector": int(sector_text) if sector_text else 0, | ||
| "sectors": int(sectors_text) if sectors_text else None, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid RPMB write ranges.
Negative sector values and nonpositive sector counts pass to write_rpmb. Reject them before the destructive operation starts.
Proposed fix
sector_text = self._write_sector.text().strip()
sectors_text = self._write_sectors.text().strip()
+ sector = int(sector_text) if sector_text else 0
+ sectors = int(sectors_text) if sectors_text else None
+ if sector < 0:
+ raise ValueError("Sector cannot be negative.")
+ if sectors is not None and sectors <= 0:
+ raise ValueError("Sector count must be greater than zero.")
return {
"filename": filename,
- "sector": int(sector_text) if sector_text else 0,
- "sectors": int(sectors_text) if sectors_text else None,
+ "sector": sector,
+ "sectors": sectors,
}📝 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.
Suggested change
| def get_write_params(self) -> dict: | |
| """Returns write params. Raises ValueError on invalid input.""" | |
| filename = self._write_file.text().strip() | |
| if not filename: | |
| raise ValueError("Input file is required for RPMB write.") | |
| sector_text = self._write_sector.text().strip() | |
| sectors_text = self._write_sectors.text().strip() | |
| return { | |
| "filename": filename, | |
| "sector": int(sector_text) if sector_text else 0, | |
| "sectors": int(sectors_text) if sectors_text else None, | |
| def get_write_params(self) -> dict: | |
| """Returns write params. Raises ValueError on invalid input.""" | |
| filename = self._write_file.text().strip() | |
| if not filename: | |
| raise ValueError("Input file is required for RPMB write.") | |
| sector_text = self._write_sector.text().strip() | |
| sectors_text = self._write_sectors.text().strip() | |
| sector = int(sector_text) if sector_text else 0 | |
| sectors = int(sectors_text) if sectors_text else None | |
| if sector < 0: | |
| raise ValueError("Sector cannot be negative.") | |
| if sectors is not None and sectors <= 0: | |
| raise ValueError("Sector count must be greater than zero.") | |
| return { | |
| "filename": filename, | |
| "sector": sector, | |
| "sectors": sectors, | |
| } |
🤖 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 `@mtk_gui/ui/tabs/rpmb_tab.py` around lines 155 - 165, Update get_write_params
to validate the parsed sector and sectors values before returning them: reject
negative sector values and sector counts that are zero or negative by raising
ValueError, while preserving the existing defaults for omitted fields.
Comment on lines
+55
to
+70
| ``` | ||
| mtk_gui/ | ||
| ├── app.py # QApplication entry point | ||
| ├── main_window.py # Signal wiring, operation handlers | ||
| ├── constants.py # App metadata, USB IDs, part types | ||
| ├── backend/ | ||
| │ ├── device_manager.py # Connection state machine, threaded USB poll | ||
| │ ├── log_interceptor.py # GuiSignalProxy for mtkclient logging bridge | ||
| │ ├── mtk_wrapper.py # Clean facade over mtkclient DA operations | ||
| │ └── worker.py # QThread worker with cancel support | ||
| ├── ui/ | ||
| │ ├── tabs/ # 12 tab widgets (one file each) | ||
| │ └── widgets/ # Reusable widgets (log panel, hex viewer, etc.) | ||
| ├── plugins/ # Plugin base class and loader | ||
| └── theme/ # QSS dark/light stylesheets | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for the fenced block.
Add a language identifier to satisfy Markdown rule MD040. Use text because this block contains a directory tree and comments.
Proposed fix
-```
+```text
mtk_gui/
...
-```
+```📝 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.
Suggested change
| ``` | |
| mtk_gui/ | |
| ├── app.py # QApplication entry point | |
| ├── main_window.py # Signal wiring, operation handlers | |
| ├── constants.py # App metadata, USB IDs, part types | |
| ├── backend/ | |
| │ ├── device_manager.py # Connection state machine, threaded USB poll | |
| │ ├── log_interceptor.py # GuiSignalProxy for mtkclient logging bridge | |
| │ ├── mtk_wrapper.py # Clean facade over mtkclient DA operations | |
| │ └── worker.py # QThread worker with cancel support | |
| ├── ui/ | |
| │ ├── tabs/ # 12 tab widgets (one file each) | |
| │ └── widgets/ # Reusable widgets (log panel, hex viewer, etc.) | |
| ├── plugins/ # Plugin base class and loader | |
| └── theme/ # QSS dark/light stylesheets | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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` around lines 55 - 70, Update the fenced directory-tree block in
the README around the mtk_gui structure to specify the text language identifier,
changing the opening fence to use text while leaving the tree content unchanged.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MtkWrapperfacade withsys.exit()interceptionGuiSignalProxybridges mtkclient'slogsetup()logging to Qt signalsConfirmationDialog.pyfiles inplugins/to extend the GUIArchitecture
Test plan
python -c "from mtk_gui.main_window import MainWindow"— no import errorspython run.py— all 12 tabs visible, dark theme, log panel worksSummary by CodeRabbit