Redesign UI: nav-rail layout with patch-gating, update-revert alert, real progress#45
Redesign UI: nav-rail layout with patch-gating, update-revert alert, real progress#45RobThePCGuy wants to merge 2 commits into
Conversation
…real progress Replaces the single-window layout with a Dashboard / Instances / Modules nav-rail app and adds the safety features the old UI lacked. main.py is slimmed to a 47-line bootstrap; the operations logic lives in views/main_window.py. Layout - Dashboard: install paths, engine-patch state, rooted-instance count, and the update-revert alert with a one-click re-patch. - Instances: per-instance Root and R/W toggles with a patch-gating banner. - Modules: sideload Magisk/Kitsune modules into a running instance. Safety features (previously absent) - Patch-gating: blocks turning root ON for patch-mode instances while the engine is unpatched/partial (turning root OFF is never blocked, since it doesn't need the patch). A banner links straight to the Dashboard patch button. - Update-reverted alert: detects when a BlueStacks auto-update silently undoes the engine patch while a rooted instance exists, and prompts to re-patch. UX - Real determinate/indeterminate progress bar with percentages. - Light/dark theme toggle, persisted across launches. - The Modules running-instance ADB probe runs on a worker thread, so switching to the tab no longer freezes the UI for several seconds. - Rich-text confirmation dialogs and plain-language status copy. Testing - pytest + pytest-qt suite (73 tests). An autouse conftest stub neutralizes the live registry probe fired by MainWindow's deferred init, so the suite passes regardless of whether the host has BlueStacks installed.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the application by modularizing the GUI into a new views package, simplifying main.py to a bootstrap entry point, and introducing a comprehensive test suite using pytest and pytest-qt. It also adds ADB-based running instance detection in adb_handler.py. The review feedback highlights several critical areas for improvement, including preventing application closure during active background operations to avoid corruption, wrapping directory listings and ADB connection checks in exception handlers for better resilience, and resolving memory leaks associated with worker threads (_scan_worker and _op_worker) by ensuring deleteLater() is processed while their respective event loops are still active.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def closeEvent(self, event): | ||
| self.status_refresh_timer.stop() | ||
| # Don't let a running ADB probe outlive the window (QThread would warn | ||
| # "destroyed while still running"). It's bounded by adb's own timeout. | ||
| self._scan_pending = False | ||
| if self._scan_thread is not None: | ||
| self._scan_thread.quit() | ||
| self._scan_thread.wait(2000) | ||
| event.accept() |
There was a problem hiding this comment.
If the user closes the window while a background operation (such as patching or restoring the engine) is running, _op_thread is not stopped or waited for. This can cause a crash on exit (due to the thread attempting to emit signals to a destroyed MainWindow or progress bar) or, worse, corrupt the BlueStacks installation files if the write operation is abruptly interrupted. We should block the close event and warn the user if a background operation is currently in progress.
| def closeEvent(self, event): | |
| self.status_refresh_timer.stop() | |
| # Don't let a running ADB probe outlive the window (QThread would warn | |
| # "destroyed while still running"). It's bounded by adb's own timeout. | |
| self._scan_pending = False | |
| if self._scan_thread is not None: | |
| self._scan_thread.quit() | |
| self._scan_thread.wait(2000) | |
| event.accept() | |
| def closeEvent(self, event): | |
| if getattr(self, "_op_thread", None) is not None: | |
| QMessageBox.warning( | |
| self, "Operation in progress", | |
| "A background operation is currently running. Please wait for it to " | |
| "complete before closing the application." | |
| ) | |
| event.ignore() | |
| return | |
| self.status_refresh_timer.stop() | |
| # Don't let a running ADB probe outlive the window (QThread would warn | |
| # "destroyed while still running"). It's bounded by adb's own timeout. | |
| self._scan_pending = False | |
| if self._scan_thread is not None: | |
| self._scan_thread.quit() | |
| self._scan_thread.wait(2000) | |
| event.accept() |
| cp = runner([adb_exe, "connect", "127.0.0.1:%d" % port]) | ||
| out = (cp.stdout or "") + (cp.stderr or "") | ||
| if "connected" in out.lower(): | ||
| running[unique_id] = port |
There was a problem hiding this comment.
If runner raises an exception (such as subprocess.TimeoutExpired due to a hanging connection, or OSError if the ADB executable is invalid), the entire loop will terminate immediately. This prevents checking any remaining instances. Wrapping the connection attempt in a try...except block ensures that a failure or timeout on one instance does not block or crash the detection of other running instances.
| cp = runner([adb_exe, "connect", "127.0.0.1:%d" % port]) | |
| out = (cp.stdout or "") + (cp.stderr or "") | |
| if "connected" in out.lower(): | |
| running[unique_id] = port | |
| try: | |
| cp = runner([adb_exe, "connect", "127.0.0.1:%d" % port]) | |
| out = (cp.stdout or "") + (cp.stderr or "") | |
| if "connected" in out.lower(): | |
| running[unique_id] = port | |
| except Exception as e: | |
| logger.warning("Failed to check ADB status for %s: %s", unique_id, e) |
| root_info = config_handler.get_complete_root_statuses(config_path) | ||
| instance_root_statuses = root_info['instance_statuses'] | ||
|
|
||
| disk_instances = {entry for entry in (os.listdir(data_path) if os.path.isdir(data_path) else []) if os.path.isdir(os.path.join(data_path, entry))} |
There was a problem hiding this comment.
If data_path is not accessible (e.g., due to restricted permissions or locking issues), os.listdir can raise an OSError (such as PermissionError). Since this runs inside the status refresh timer, any unhandled exception here will crash the timer and potentially the entire UI. Wrapping os.listdir in a try...except OSError block ensures the application remains robust and handles restricted directories gracefully.
| disk_instances = {entry for entry in (os.listdir(data_path) if os.path.isdir(data_path) else []) if os.path.isdir(os.path.join(data_path, entry))} | |
| disk_instances = set() | |
| if os.path.isdir(data_path): | |
| try: | |
| disk_instances = { | |
| entry for entry in os.listdir(data_path) | |
| if os.path.isdir(os.path.join(data_path, entry)) | |
| } | |
| except OSError as e: | |
| logger.warning("Failed to list directory %s: %s", data_path, e) |
| self._scan_worker.finished.connect(self._on_scan_finished) | ||
| self._scan_worker.finished.connect(self._scan_thread.quit) | ||
| self._scan_thread.finished.connect(self._cleanup_scan) |
There was a problem hiding this comment.
Calling deleteLater() on _scan_worker inside _cleanup_scan (which runs after the thread has finished) posts a DeferredDelete event to the thread's event loop. Since the thread's event loop has already stopped, the event is never processed, resulting in a memory leak of the worker object. Connecting the worker's finished signal directly to its own deleteLater slot ensures it is safely deleted while the thread's event loop is still active.
| self._scan_worker.finished.connect(self._on_scan_finished) | |
| self._scan_worker.finished.connect(self._scan_thread.quit) | |
| self._scan_thread.finished.connect(self._cleanup_scan) | |
| self._scan_worker.finished.connect(self._on_scan_finished) | |
| self._scan_worker.finished.connect(self._scan_worker.deleteLater) | |
| self._scan_worker.finished.connect(self._scan_thread.quit) | |
| self._scan_thread.finished.connect(self._cleanup_scan) |
| if self._scan_worker is not None: | ||
| self._scan_worker.deleteLater() | ||
| if self._scan_thread is not None: | ||
| self._scan_thread.deleteLater() | ||
| self._scan_worker = None | ||
| self._scan_thread = None |
There was a problem hiding this comment.
Since _scan_worker is now safely deleted via the thread's event loop using deleteLater connected to finished, we can remove the manual deleteLater() call from _cleanup_scan to avoid redundant or unsafe deletion attempts on a stopped thread.
| if self._scan_worker is not None: | |
| self._scan_worker.deleteLater() | |
| if self._scan_thread is not None: | |
| self._scan_thread.deleteLater() | |
| self._scan_worker = None | |
| self._scan_thread = None | |
| if self._scan_thread is not None: | |
| self._scan_thread.deleteLater() | |
| self._scan_worker = None | |
| self._scan_thread = None |
| self._op_worker.done.connect(self._on_async_done) | ||
| self._op_worker.done.connect(self._op_thread.quit) | ||
| self._op_thread.finished.connect(self._cleanup_async) |
There was a problem hiding this comment.
Similar to the scan worker, calling deleteLater() on _op_worker inside _cleanup_async after the thread has finished results in a memory leak because the thread's event loop is no longer running. Connecting the worker's done signal directly to its own deleteLater slot ensures it is safely deleted while the thread's event loop is still active.
| self._op_worker.done.connect(self._on_async_done) | |
| self._op_worker.done.connect(self._op_thread.quit) | |
| self._op_thread.finished.connect(self._cleanup_async) | |
| self._op_worker.done.connect(self._on_async_done) | |
| self._op_worker.done.connect(self._op_worker.deleteLater) | |
| self._op_worker.done.connect(self._op_thread.quit) | |
| self._op_thread.finished.connect(self._cleanup_async) |
| if getattr(self, "_op_worker", None) is not None: | ||
| self._op_worker.deleteLater() | ||
| if getattr(self, "_op_thread", None) is not None: | ||
| self._op_thread.deleteLater() | ||
| self._op_worker = None | ||
| self._op_thread = None |
There was a problem hiding this comment.
Since _op_worker is now safely deleted via the thread's event loop using deleteLater connected to done, we can remove the manual deleteLater() call from _cleanup_async to avoid redundant or unsafe deletion attempts on a stopped thread.
| if getattr(self, "_op_worker", None) is not None: | |
| self._op_worker.deleteLater() | |
| if getattr(self, "_op_thread", None) is not None: | |
| self._op_thread.deleteLater() | |
| self._op_worker = None | |
| self._op_thread = None | |
| if getattr(self, "_op_thread", None) is not None: | |
| self._op_thread.deleteLater() | |
| self._op_worker = None | |
| self._op_thread = None |
…ater Resolves the gemini-code-assist review on #45. - closeEvent refuses to close while a background operation is running (high): it was possible to close the window mid-patch/mid-toggle, killing a thread writing HD-Player.exe or the guest VHDX and risking a corrupt install. Now it warns and ignores the close until the op finishes. +tests. - adb_handler.list_running_instances: wrap the per-instance `adb connect` in try/except so a timeout or OSError on one instance no longer aborts the whole sweep (the others still get probed). - update_instance_data: guard os.listdir with try/except OSError so a PermissionError on a data dir can't crash the status-refresh timer. - scan/op workers: delete them via finished/done -> deleteLater, i.e. from inside the worker's own still-running event loop. The previous deleteLater in the post-thread cleanup was posted to an already-stopped loop and never ran, leaking the worker object.
1 participant
What & why
Replaces the single-window layout with a Dashboard / Instances / Modules nav-rail app and adds the safety features the old UI lacked.
main.pyis slimmed to a 47-line bootstrap; the operations logic moves intoviews/main_window.py.Layout
Safety features (previously absent)
UX
Testing
pytest+pytest-qtsuite — 73 passing.confteststub neutralizes the live registry probe fired byMainWindow's deferred init, so the suite passes regardless of whether the host has BlueStacks installed.Squashed from the working branch; the process history, planning docs, and agent worktrees are intentionally left out.