Collapse engine patch/undo/status into one dynamic button#38
Conversation
Three separate widgets (Patch button, Undo button, status label) become a single state-driven button: - unpatched -> "Patch BlueStacks Engine (required for root)" (red), patches - patched -> "Engine patched - click to Undo" (green), restores - partial -> "Engine partially patched - click to finish patching" (orange) - unknown -> disabled, greyed The label, colour, tooltip, and click action all derive from the live engine state, re-read at click time so a stale label can't trigger the wrong action.
![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 BlueStacks engine patching UI by consolidating the separate patch and restore buttons, along with the status label, into a single dynamic button that updates its appearance and action based on the current patch state. The review feedback highlights a potential issue where unrecognized builds (state "unknown") could accidentally be patched. To prevent this, it is recommended to set the engine action to None for unknown states and explicitly verify the action type in the click handler before executing any operations.
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.
| # Remember the action for the click handler; disabled only on unknown | ||
| # builds (busy-state disabling is handled by _set_busy during ops). | ||
| self._engine_action = "restore" if state == "patched" else "patch" | ||
| self.engine_button.setEnabled(enabled) |
There was a problem hiding this comment.
If the engine state is determined to be unknown, the button is disabled, but self._engine_action is still set to "patch". If handle_engine_button is triggered (e.g., due to a race condition or if the state becomes unknown during the click-time refresh), it will proceed to call self.handle_apply_patches(). This could attempt to patch an unrecognized build, potentially corrupting the binaries.
To prevent accidental patching of unrecognized builds, we should set self._engine_action to None when the state is unknown.
| # Remember the action for the click handler; disabled only on unknown | |
| # builds (busy-state disabling is handled by _set_busy during ops). | |
| self._engine_action = "restore" if state == "patched" else "patch" | |
| self.engine_button.setEnabled(enabled) | |
| # Remember the action for the click handler; disabled only on unknown | |
| # builds (busy-state disabling is handled by _set_busy during ops). | |
| self._engine_action = "restore" if state == "patched" else ("patch" if state != "unknown" else None) | |
| self.engine_button.setEnabled(enabled) |
| def handle_engine_button(self) -> None: | ||
| """Patch or restore the engine, per the current (freshly-read) state.""" | ||
| # Re-read state at click time so a stale label can't send us the wrong way. | ||
| self._refresh_patch_ui() | ||
| if getattr(self, "_engine_action", "patch") == "restore": | ||
| self.handle_restore_patches() | ||
| else: | ||
| self.handle_apply_patches() |
There was a problem hiding this comment.
Currently, if self._engine_action is not "restore", the handler defaults to calling self.handle_apply_patches(). If the state is unknown (or if there is no patch build), we should avoid executing any action.
We should explicitly check for "restore" and "patch" actions, and do nothing if the action is None.
| def handle_engine_button(self) -> None: | |
| """Patch or restore the engine, per the current (freshly-read) state.""" | |
| # Re-read state at click time so a stale label can't send us the wrong way. | |
| self._refresh_patch_ui() | |
| if getattr(self, "_engine_action", "patch") == "restore": | |
| self.handle_restore_patches() | |
| else: | |
| self.handle_apply_patches() | |
| def handle_engine_button(self) -> None: | |
| """Patch or restore the engine, per the current (freshly-read) state.""" | |
| # Re-read state at click time so a stale label can't send us the wrong way. | |
| self._refresh_patch_ui() | |
| action = getattr(self, "_engine_action", None) | |
| if action == "restore": | |
| self.handle_restore_patches() | |
| elif action == "patch": | |
| self.handle_apply_patches() |
1 participant
Replaces the three engine widgets — Patch button, Undo button, and the status label — with a single button whose label, colour, tooltip, and action all reflect the live engine state:
State is re-read at click time so a stale label can't send the action the wrong way. Busy-state disabling is handled by the existing
_set_busypath.Verified live: Undo → restore → re-patch cycle works in the app; and headlessly across all four states (correct label/colour/action/enabled) plus busy-disable.
(Supersedes #37, which auto-closed when its stacked base branch was deleted on merge of #36.)