Skip to content

Commit b0e6923

Browse files
okohlbacherclaude
okohlbacher
and
claude
committed
merge(999.73): scope keyboard shortcuts (#622) + project files don't clobber user settings (#621)
- #622 (BUG-622): Delete/Cut/Copy/Paste scoped to WidgetWithChildrenShortcut on their owning controls — no longer fire window-globally / steal keys from text fields. - #621 (BUG-621): projects no longer embed/restore the whole-app prefs blob (was clobbering font/language/PDB-URL/paths). Scene-visual state (background/fog/lighting/ clipping) DOES travel with the project via scene-only writePreferences/fetchPreferences (user UAT choice). Global prefs stay protected; scene look reopens as designed. UAT approved 2026-05-31 (#622; #621 incl. scene-visuals refinement). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 parents afc8c89 + aa04433 commit b0e6923

6 files changed

Lines changed: 317 additions & 15 deletions

File tree

‎.planning/phases/999.73-platform-ux-bugs/999.73-03-SUMMARY.md‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
---
2+
phase: 999.73-platform-ux-bugs
3+
plan: 03
4+
subsystem: ui
5+
tags: [qt, shortcuts, keyboard, focus, QAction, ShortcutContext, BALLView]
6+
7+
# Dependency graph
8+
requires:
9+
- phase: 999.73-platform-ux-bugs
10+
provides: prior platform UX bug fixes in the same phase
11+
provides:
12+
- High-impact destructive shortcuts (Del / Ctrl+C/V/X) scoped to their owning widget focus subtree
13+
- insertDeleteEntry(QWidget* owner) focus-owner registration API on MainControl
14+
affects: [999.73-04, platform-ux, keyboard-shortcuts]
15+
16+
# Tech tracking
17+
tech-stack:
18+
added: []
19+
patterns:
20+
- "Qt::WidgetWithChildrenShortcut scoping + QWidget::addAction to bind a menu QAction's shortcut to a focus subtree"
21+
22+
key-files:
23+
created: []
24+
modified:
25+
- include/BALL/VIEW/KERNEL/mainControl.h
26+
- source/VIEW/KERNEL/mainControl.C
27+
- source/VIEW/WIDGETS/genericControl.C
28+
- source/VIEW/WIDGETS/molecularControl.C
29+
30+
key-decisions:
31+
- "Scope only the high-impact actions (Delete, Cut/Copy/Paste) rather than blanket-changing insertMenuEntry, so safe app-wide accelerators (Quit, Preferences) keep Qt::WindowShortcut."
32+
- "insertDeleteEntry gains an optional QWidget* owner; each GenericControl registers itself as a Delete focus owner via addAction, since the single shared delete_action_ is driven from multiple controls."
33+
- "No Space QAction exists anywhere in the codebase (grep for Key_Space is empty), so Space needed no scoping change — removing the window-global destructive actions already frees Space for text inputs."
34+
35+
patterns-established:
36+
- "Pattern: a menu QAction created via insertMenuEntry is scoped to a widget by setShortcutContext(Qt::WidgetWithChildrenShortcut) + that widget's addAction()."
37+
38+
requirements-completed: [BUG-622]
39+
40+
# Metrics
41+
duration: ~20min
42+
completed: 2026-06-01
43+
---
44+
45+
# Phase 999.73 Plan 03: Scope keyboard shortcuts (BUG-622) Summary
46+
47+
**Del / Ctrl+C / Ctrl+V / Ctrl+X are now scoped to Qt::WidgetWithChildrenShortcut on their owning widgets (GenericControls / MolecularControl) instead of firing window-globally, so they no longer steal keystrokes from text fields and the embedded web view while still working in their intended context.**
48+
49+
## Performance
50+
51+
- **Duration:** ~20 min
52+
- **Completed:** 2026-06-01
53+
- **Tasks:** 2 of 2 automated tasks (UAT checkpoint pending human verification)
54+
- **Files modified:** 4
55+
56+
## Accomplishments
57+
58+
- **Task 1 — Delete scoping** (`4112d663a3`): `delete_action_` now sets `Qt::WidgetWithChildrenShortcut`. `insertDeleteEntry` gained an optional `QWidget* owner` parameter; each `GenericControl` registers itself via `owner->addAction(delete_action_)` in `GenericControl::initializeWidget`, so Del only deletes selected structures when a structure-owning control (or its child) holds focus.
59+
- **Task 2 — Cut/Copy/Paste scoping** (`64668ec36a`): `cut_id_`, `copy_id_`, `paste_id_` set `Qt::WidgetWithChildrenShortcut` and are added to the `MolecularControl` widget, so Ctrl+C/V/X resolve only when the molecular control subtree has focus. `MolecularControl` also passes `this` as the Delete focus owner.
60+
61+
## Root cause confirmed
62+
63+
`MainControl::insertMenuEntry` (mainControl.C:938) creates every menu QAction via `popup->addAction(...)` and never sets a shortcut context, so all actions defaulted to `Qt::WindowShortcut` — they fired whenever the main window was active, regardless of which child widget had focus. Del / Ctrl+C/V/X therefore stole keys from text fields and the embedded web view and could delete structures from the wrong context.
64+
65+
## Build result
66+
67+
`cmake --build build/ci-macos --target VIEW` — clean compile, `libVIEW.1.7.dylib` links. `BALLView` app target also links cleanly (`bin/BALLView.app/Contents/MacOS/BALLView`).
68+
69+
## Automated verification
70+
71+
- `grep -n setShortcutContext source/VIEW/KERNEL/mainControl.C` → 1 hit (delete_action_).
72+
- `grep -c setShortcutContext source/VIEW/WIDGETS/molecularControl.C` → 3 (cut/copy/paste).
73+
74+
## Deviations from Plan
75+
76+
**None functionally.** Two clarifications:
77+
78+
1. **[Rule 3 — blocking] `insertDeleteEntry` signature change.** The plan said to "addAction it to the appropriate VIEW widget," but `delete_action_` is a single MainControl-owned action created from multiple call sites (`GenericControl::initializeWidget` and `MolecularControl`). To register the correct focus owner per call site, `insertDeleteEntry()` gained an optional `QWidget* owner` parameter (default `0`, backward compatible). The two known call sites pass `this`. The header doc was updated to match.
79+
2. **Space:** the plan flagged Space as a candidate high-impact action "(e.g. Space)". A repo-wide grep for `Key_Space` / `"Space"` QAction bindings returned nothing — Space is not registered as a BALLView shortcut, so no Space-specific change was required. Removing the window-global destructive actions is what frees Space for text inputs.
80+
81+
## UAT steps (autonomous:false — NOT self-approved)
82+
83+
The orchestrator builds a fresh app and runs the human UAT:
84+
85+
1. Launch BALLView (`BALL_DATA_PATH` → `data/`).
86+
2. Load a structure and SELECT some atoms in the molecular control.
87+
3. Click into a text input (a dialog field, the Python shell/log input, or the embedded help/web view) so it has focus.
88+
4. Press Space and Delete, and Ctrl+C / Ctrl+V / Ctrl+X. **EXPECTED:** the text field receives the keystrokes normally; the selected structure is NOT deleted/copied/cut/pasted; Space is NOT swallowed.
89+
5. Click into the 3D Scene / molecular control so it has focus and press Del. **EXPECTED:** Del deletes the selected structure (action still works in its intended context).
90+
91+
## Self-Check: PASSED
92+
93+
- Commit `4112d663a3` present (Task 1 Delete scoping).
94+
- Commit `64668ec36a` present (Task 2 Cut/Copy/Paste scoping).
95+
- `setShortcutContext` present in both mainControl.C (1) and molecularControl.C (3).
96+
- VIEW and BALLView build clean.

‎.planning/phases/999.73-platform-ux-bugs/999.73-04-SUMMARY.md‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
---
2+
phase: 999.73-platform-ux-bugs
3+
plan: 04
4+
subsystem: ui
5+
tags: [persistence, project-file, preferences, INIFile, BALLView, bvp]
6+
7+
# Dependency graph
8+
requires:
9+
- phase: 999.73-platform-ux-bugs
10+
provides: plan 03 shortcut scoping (shares source/VIEW/KERNEL/mainControl.C; executed first)
11+
provides:
12+
- Project (.bvp) load/save that restores project state only, never the user's preferences
13+
affects: [platform-ux, persistence, PresentaBALL]
14+
15+
# Tech tracking
16+
tech-stack:
17+
added: []
18+
patterns:
19+
- "Separation of project STATE (representations/camera/systems) from user SETTINGS (the user INI) across the .bvp persistence boundary"
20+
21+
key-files:
22+
created: []
23+
modified:
24+
- source/VIEW/KERNEL/mainControl.C
25+
26+
key-decisions:
27+
- "Remove both fetchPreferences(in) AND applyPreferences() from loadBALLViewProjectFile: applyPreferences() there only pushed the just-fetched project prefs; project state re-renders via restoreRepresentations independently, and the user's live preferences must remain untouched."
28+
- "Remove writePreferences(out) from saveBALLViewProjectFile so new .bvp files carry project state only; old files with a stale [preferences] block are now harmless because load no longer reads them."
29+
- "Leave the user-INI persistence path untouched (fetchPreferences at startup l.626, writePreferences at exit l.691) — user settings live solely in the user's own INI."
30+
31+
patterns-established:
32+
- "Pattern: an embedded settings copy inside a document file is never applied to global app state on load; the document restores only the state it legitimately owns."
33+
34+
requirements-completed: [BUG-621]
35+
36+
# Metrics
37+
duration: ~15min
38+
completed: 2026-06-01
39+
---
40+
41+
# Phase 999.73 Plan 04: Project files no longer override user settings (BUG-621) Summary
42+
43+
**Loading a (supplied) BALLView project file now restores only its own project state — representations, scene turn point/camera, and serialized systems — and never overwrites the user's live preferences (font, working dir, PDB download URL, every ModularWidget's prefs), fixing the PresentaBALL "every project load reloads settings" symptom.**
44+
45+
## Performance
46+
47+
- **Duration:** ~15 min
48+
- **Completed:** 2026-06-01
49+
- **Tasks:** 2 of 2 automated tasks (UAT checkpoint pending human verification)
50+
- **Files modified:** 1
51+
52+
## Accomplishments
53+
54+
Both changes in `source/VIEW/KERNEL/mainControl.C`, committed together as one BUG-621 fix (`8bf7c913f0`) since they are tightly coupled in the same file and must ship together:
55+
56+
- **Task 1 — load side:** `loadBALLViewProjectFile` no longer calls `fetchPreferences(in)` + `applyPreferences()`. Those restored working dir, font, the preferences dialog, and every ModularWidget's prefs from the project's embedded (often stale) INI copy, clobbering the user's live settings. Project STATE — representations (`restoreRepresentations`), scene camera/turn point, and the serialized System composites — still restores below, independently.
57+
- **Task 2 — save side:** `saveBALLViewProjectFile` no longer calls `writePreferences(out)`, so newly-saved `.bvp` files contain only the `BALLVIEW_PROJECT` section (representations, turn point) plus the serialized systems — no user-preference copy.
58+
59+
## Root cause confirmed
60+
61+
`saveBALLViewProjectFile` embedded the full user INI into the project via `writePreferences(out)`; `loadBALLViewProjectFile` restored it via `fetchPreferences(in)` + `applyPreferences()`, applying the project's stale preference copy over the user's live settings on every open. `fetchPreferences` (mainControl.C:1120) restores working dir, font, the preferences dialog and every ModularWidget's prefs — exactly the clobber described in #621. Project state restore is entirely separate (lines ~2149–2243), so dropping the preference fetch/apply removes the clobber without losing any project content.
62+
63+
## Exact change
64+
65+
- Deleted `fetchPreferences(in);` and `applyPreferences();` from the load path (replaced with a BUG-621 explanatory comment).
66+
- Deleted `writePreferences(out);` from the save path (replaced with a BUG-621 explanatory comment).
67+
- The user-INI path is untouched: startup `fetchPreferences(preferences_file_)` (l.626) and exit `writePreferences(preferences_file_)` (l.691) remain, so user settings persist exactly as before.
68+
69+
## Build result
70+
71+
`cmake --build build/ci-macos --target VIEW` — `mainControl.C` compiles clean, `libVIEW.1.7.dylib` links. `BALLView` app target links cleanly.
72+
73+
## Automated verification
74+
75+
- `grep -n BUG-621 source/VIEW/KERNEL/mainControl.C` → 2 hits (load + save comments).
76+
- No `fetchPreferences(in)` / `writePreferences(out)` remain in the project-file paths; only the user-INI calls (l.626, l.691) and the function definitions remain.
77+
78+
## Deviations from Plan
79+
80+
**None.** The plan offered a conditional on `applyPreferences()` ("if it is only there to push the just-fetched project prefs, remove it too"). Analysis confirmed `applyPreferences()` at the load site existed solely to apply the just-fetched project prefs — project-state re-render is handled separately by `restoreRepresentations` and the camera/turn-point messages — so it was removed along with `fetchPreferences(in)`, leaving user preferences entirely untouched.
81+
82+
The plan split this into two tasks but both edits live in the same file and are interdependent (load ignoring prefs only makes sense alongside save not writing them), so they were committed together as a single commit-per-bug commit, satisfying the success criterion "Commit isolates BUG-621."
83+
84+
## UAT steps (autonomous:false — NOT self-approved)
85+
86+
The orchestrator builds a fresh app and runs the human UAT:
87+
88+
1. Launch BALLView (`BALL_DATA_PATH` → `data/`).
89+
2. Change a distinctive user setting (e.g. a Preferences value, the background color, or the PDB download URL); confirm applied. Quit and relaunch to confirm it persisted to the user INI.
90+
3. Open a SUPPLIED/example project file (a PresentaBALL project or any bundled `.bvp`) via File > Open Project. **EXPECTED:** the project's molecules/representations/view load, BUT your distinctive setting from step 2 is UNCHANGED. The PDB download URL in particular must remain your value.
91+
4. Quit and relaunch: the user setting is still your value.
92+
5. Save a new project, reopen it: project state restores correctly and settings are still untouched.
93+
94+
## Self-Check: PASSED
95+
96+
- Commit `8bf7c913f0` present (BUG-621 load + save fix).
97+
- `BUG-621` comments present in mainControl.C (2 hits).
98+
- No project-path `fetchPreferences(in)` / `writePreferences(out)` remain; user-INI calls preserved.
99+
- VIEW and BALLView build clean.

‎include/BALL/VIEW/KERNEL/mainControl.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -582,8 +582,15 @@ namespace BALL
582582

583583
/** Insert the delete entry for GenericControls.
584584
Called by all GenericControls.
585+
586+
BUG-622 (#622): the optional \p owner widget is registered as a
587+
focus owner for the Delete shortcut. The Delete QAction uses
588+
Qt::WidgetWithChildrenShortcut, so Del only deletes selected
589+
structures when one of the registered GenericControls (or the
590+
scene) has keyboard focus — it no longer fires window-globally and
591+
steals the Delete key from text fields or the embedded web view.
585592
*/
586-
void insertDeleteEntry();
593+
void insertDeleteEntry(QWidget* owner = 0);
587594

588595
/// Get the ID of the last highlighted menu entry
589596
QAction* getLastHighLightedMenuEntry() { return last_highlighted_menu_entry_;}

‎source/VIEW/KERNEL/mainControl.C‎

Lines changed: 82 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1744,13 +1744,34 @@ namespace BALL
17441744
}
17451745
}
17461746

1747-
void MainControl::insertDeleteEntry()
1747+
void MainControl::insertDeleteEntry(QWidget* owner)
17481748
{
1749-
if (delete_action_ == 0)
1749+
if (delete_action_ == 0)
17501750
{
1751-
delete_action_ = insertMenuEntry(MainControl::EDIT, (String)tr("Delete"), this,
1751+
delete_action_ = insertMenuEntry(MainControl::EDIT, (String)tr("Delete"), this,
17521752
SLOT(deleteClicked()), "Shortcut|Edit|Delete", QKeySequence::Delete,
1753-
UIOperationMode::MODE_ADVANCED);
1753+
UIOperationMode::MODE_ADVANCED);
1754+
1755+
// BUG-622 (#622): scope the destructive Delete shortcut so it only
1756+
// fires when a structure-owning widget (a GenericControl such as the
1757+
// molecular control, or the scene) has keyboard focus. Without this
1758+
// the action defaults to Qt::WindowShortcut and Del fires
1759+
// window-globally — deleting selected structures even while a text
1760+
// field or the embedded web view is focused, and swallowing the
1761+
// Delete key from those inputs.
1762+
if (delete_action_ != 0)
1763+
{
1764+
delete_action_->setShortcutContext(Qt::WidgetWithChildrenShortcut);
1765+
}
1766+
}
1767+
1768+
// BUG-622 (#622): register the calling widget (and its child subtree) as a
1769+
// focus owner for the Delete shortcut. Each GenericControl that can delete
1770+
// items adds the (shared) action to itself, so Del resolves only when that
1771+
// control — or a child of it — holds focus.
1772+
if (delete_action_ != 0 && owner != 0)
1773+
{
1774+
owner->addAction(delete_action_);
17541775
}
17551776
}
17561777

@@ -2010,8 +2031,27 @@ namespace BALL
20102031
String s = vector3ToString(scene->getTurnPoint());
20112032
out.insertValue("BALLVIEW_PROJECT", "TurnPoint", s);
20122033
}
2013-
2014-
writePreferences(out);
2034+
2035+
// BUG-621 (#621): do NOT embed the user's GLOBAL preference set into project
2036+
// files. Previously writePreferences(out) baked the entire user INI (font,
2037+
// working dir, network/PDB-download settings, every ModularWidget's prefs)
2038+
// into the project, which loadBALLViewProjectFile then applied over the
2039+
// user's live settings on every open — the PresentaBALL "every open reloads
2040+
// settings" symptom. Global preferences now live solely in the user's own INI.
2041+
//
2042+
// BUT the SCENE's visual state (background color, fog, lighting, clipping,
2043+
// projection — everything Scene::writePreferences emits) is genuine PROJECT
2044+
// state: a project should reopen looking as it was designed, the same way its
2045+
// camera/turn point and representations do (user choice, 999.73-04 UAT). So we
2046+
// write ONLY the Scene widget's preferences into the project — not the whole-
2047+
// app prefs blob — and the load side restores only those. Global prefs stay
2048+
// protected; scene visuals travel with the project. Older project files that
2049+
// still carry a full [preferences] block are handled by the load side reading
2050+
// back only the Scene section.
2051+
if (scene != 0)
2052+
{
2053+
scene->writePreferences(out);
2054+
}
20152055

20162056
INIFile::LineIterator lit = out.getLine(0);
20172057
File result;
@@ -2110,8 +2150,42 @@ namespace BALL
21102150
}
21112151
}
21122152

2113-
fetchPreferences(in);
2114-
applyPreferences();
2153+
// BUG-621 (#621): a project file embeds a full copy of the user's BALLView
2154+
// INI (see saveBALLViewProjectFile, where writePreferences used to be
2155+
// called). Previously this restore ran fetchPreferences(in) + applyPreferences()
2156+
// here, overwriting the user's live settings (working dir, font, the
2157+
// preferences dialog, the PDB download URL, and every ModularWidget's prefs)
2158+
// with the project's — often stale — copy. That is exactly the PresentaBALL
2159+
// "every project load reloads settings" symptom.
2160+
//
2161+
// A project file legitimately owns only PROJECT STATE — its representations,
2162+
// scene turn point/camera, and serialized systems — all of which are restored
2163+
// below from the BALLVIEW_PROJECT section and the system stream, independently
2164+
// of the user preferences. So we no longer touch user preferences on load:
2165+
// the user's settings live solely in their own INI (preferences_file_, fetched
2166+
// at startup and written at exit) and are never clobbered by opening a project.
2167+
// applyPreferences() here only existed to push the just-fetched project prefs,
2168+
// so the whole-app restore is removed; restoreRepresentations below re-renders
2169+
// the restored project state on its own.
2170+
//
2171+
// EXCEPTION (999.73-04 UAT, user choice): restore ONLY the Scene widget's
2172+
// preferences — the scene-visual state (background color, fog, lighting,
2173+
// clipping, projection) that saveBALLViewProjectFile writes via
2174+
// scene->writePreferences(out). This is project look, not global settings, so
2175+
// it should travel with the project like the camera does. We fetch + apply
2176+
// just the Scene (never MainControl's global prefs or other preference pages),
2177+
// so a project reopens looking as designed while font/language/PDB-URL/paths
2178+
// stay the user's. Old projects that embedded a full prefs blob still carry a
2179+
// Scene section, so their background travels too; their other (global) sections
2180+
// are simply ignored here.
2181+
{
2182+
Scene* scene_prefs = Scene::getInstance(0);
2183+
if (scene_prefs != 0)
2184+
{
2185+
scene_prefs->fetchPreferences(in);
2186+
scene_prefs->applyPreferences();
2187+
}
2188+
}
21152189

21162190
// Phase 999.67 Plan 03: the suppress-during-load guard is re-homed off
21172191
// DisplayProperties onto the non-dialog RepresentationCreator. A project

‎source/VIEW/WIDGETS/genericControl.C‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,12 @@ namespace BALL
8787

8888
void GenericControl::initializeWidget(MainControl& main_control)
8989
{
90-
main_control.insertDeleteEntry();
90+
// BUG-622 (#622): pass this control as a focus owner so the Delete
91+
// shortcut (Qt::WidgetWithChildrenShortcut) resolves only when this
92+
// control — which owns the selectable/deletable items — has focus.
93+
main_control.insertDeleteEntry(this);
9194
DockWidget::initializeWidget(main_control);
92-
}
95+
}
9396

9497
QTreeWidgetItem* GenericControl::addRow(const QStringList& entries)
9598
{

0 commit comments

Comments
 (0)