Conversation
|
| Filename | Overview |
|---|---|
| ui/dockwidget/action_row.py | Refactors _button_stylesheet from a bool primary flag to a string role; adds style_destructive_action_button and danger-tone hover chrome; secondary role intentionally returns empty stylesheet for native Qt appearance. |
| ui/dockwidget/sync_page.py | Swaps clear_button from style_secondary_action_button to style_destructive_action_button; imports and layout unchanged. |
| tests/test_wizard_action_row.py | Updates secondary-button test to assert empty styleSheet(); adds new test_destructive_action_button_gets_danger_role_and_chrome covering #c01c28 presence and #589632 absence. |
| tests/test_sync_page.py | Replaces secondaryAction/"secondary" assertions with destructiveAction/"destructive" for clear_button; adds stylesheet inequality and green-colour exclusion guards. |
| docs/design-system.md | Documents the three shared wizard-button helpers and the intentional native-Qt treatment for secondary buttons. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[style_primary_action_button] -->|role=primary| C[_apply_button_chrome]
B[style_secondary_action_button] -->|role=secondary| C
D[style_destructive_action_button] -->|role=destructive| C
C --> E[setToolButtonStyle ToolButtonTextBesideIcon]
C --> F[_allow_button_shrink]
C --> G[setCursor PointingHandCursor]
C --> H[setStyleSheet _button_stylesheet role]
H -->|role == primary| I[Green accent font-weight 700]
H -->|role == destructive| J[Transparent bg Red text + danger hover font-weight 700]
H -->|role == secondary| K[Empty string native Qt styling]
Reviews (1): Last reviewed commit: "Normalize wizard button roles" | Re-trigger Greptile
| @@ -181,24 +195,27 @@ def _button_stylesheet(*, primary: bool) -> str: | |||
| f"color: {COLOR_MUTED}; " | |||
| "}" | |||
| ) | |||
| return ( | |||
| "QToolButton { " | |||
| "background: transparent; " | |||
| f"color: {COLOR_TEXT}; " | |||
| f"border: 1px solid {COLOR_SEPARATOR}; " | |||
| "border-radius: 6px; " | |||
| "padding: 5px 10px; " | |||
| "font-weight: 500; " | |||
| "} " | |||
| f"QToolButton:hover:enabled {{ background: {COLOR_HOVER}; }} " | |||
| f"QToolButton:disabled {{ color: {COLOR_MUTED}; }}" | |||
| ) | |||
| if role == "destructive": | |||
| return ( | |||
| "QToolButton { " | |||
| "background: transparent; " | |||
| f"color: {COLOR_DANGER}; " | |||
| "border: 1px solid transparent; " | |||
| "border-radius: 6px; " | |||
| "padding: 5px 10px; " | |||
| "font-weight: 700; " | |||
| "} " | |||
| f"QToolButton:hover:enabled {{ background: {COLOR_DANGER_BG}; }} " | |||
| f"QToolButton:disabled {{ color: {COLOR_MUTED}; }}" | |||
| ) | |||
| return "" | |||
There was a problem hiding this comment.
Silent fallthrough for unknown roles
_button_stylesheet returns "" for any string that is neither "primary" nor "destructive" — including the "secondary" role by design, but also any future typo or misuse. Since this is a private function only reachable through the three public style_* helpers the risk is low today, but an explicit elif role == "secondary": return "" (or a final else: raise ValueError(...)) would make the contract self-documenting and catch mistakes at the call site instead of silently producing an unstyled button.
| def _apply_button_chrome(button: QToolButton, *, role: str = "primary") -> None: | ||
| if hasattr(button, "setToolButtonStyle"): | ||
| button.setToolButtonStyle(Qt.ToolButtonTextBesideIcon) | ||
| _allow_button_shrink(button) | ||
| if hasattr(button, "setCursor"): | ||
| button.setCursor(Qt.PointingHandCursor) | ||
| if hasattr(button, "setStyleSheet"): | ||
| button.setStyleSheet(_button_stylesheet(primary=primary)) | ||
| button.setStyleSheet(_button_stylesheet(role=role)) |
There was a problem hiding this comment.
setToolButtonStyle and cursor applied to secondary buttons
_apply_button_chrome unconditionally calls setToolButtonStyle(Qt.ToolButtonTextBesideIcon) and setCursor(Qt.PointingHandCursor) for all roles, including "secondary". The PR description says secondary buttons should keep native Qt styling, but these two widget-level properties are not stylesheet-based and won't be reset by the empty setStyleSheet(""). Secondary buttons will still end up with ToolButtonTextBesideIcon and a pointer cursor even though they aren't receiving qfit chrome. If fully native secondary buttons are the goal, consider gating those two calls on role != "secondary".
8834213 to
39e92c4
Compare
|
Addressed the first Greptile P2 in For the second P2: I kept |
|



Summary
Clear local database…as a separated destructive action, never as primary/green.Closes #793.
Validation
python3 -m pytest tests/test_wizard_action_row.py tests/test_sync_page.py tests/test_map_page.py tests/test_analysis_page.py tests/test_atlas_page.py tests/test_connection_page.py -qpython3 -m pytest tests/ -x -q --tb=shortgit diff --checkScreenshot validation
Before:
After: