Add GUI foundation and pop_adjustevents port

[suraj-ranganath]

Summary

This PR adds the first GUI foundation for EEGPREP's EEGLAB-compatible desktop workflows.

• Adds a new eegprep.gui package with renderer-independent dialog specs modeled after EEGLAB inputgui/uilist/geometry.
• Adds an optional PySide6/Qt renderer behind the eegprep[gui] extra, with lazy imports so core import eegprep does not require Qt or a display.
• Ports EEGLAB's pop_adjustevents workflow for adjusting event latencies by milliseconds or samples, including GUI result decoding and EEGLAB-style com history generation.
• Adds docs/gui_porting_principles.md as agent-facing GUI porting guidance, while keeping it out of Sphinx.
• Adds Sphinx user/API documentation for GUI support and pop_adjustevents.

Validation

• EEGPREP_SKIP_MATLAB=1 /data/projects/suraj/.miniforge3/envs/eegprep-dev/bin/python -m pytest tests/test_parity_harness.py tests/test_pop_adjustevents.py -q
• LC_ALL=C LANG=C /data/projects/suraj/.miniforge3/envs/eegprep-dev/bin/python -m sphinx -b html docs/source /tmp/eegprep-docs-gui-foundation -q
• git diff --check

Notes

This is stacked on feat/parity-harness-foundation because the GUI foundation builds on the parity harness and agent guidance work from that branch.

The Sphinx build completes with existing autodoc/docstring warnings from unrelated files; no GUI-porting-principles agent doc is included in the Sphinx toctree.

[claude]

Code Review: PR #46 — GUI Foundation and pop_adjustevents Port

This is a well-structured, thoughtful PR with good test coverage and clean separation between the spec layer and the Qt renderer. The architecture (renderer-independent specs, lazy Qt import, fake-renderer testing pattern) follows the porting principles document well. Below are issues found at varying severity levels.

---

Bugs

1. _event_list mutates the original EEG dict when event is a plain dict (pop_adjustevents.py:684)

if isinstance(events, dict):
    EEG["event"] = [events]   # <-- mutates the caller's dict
    return EEG["event"]

pop_adjustevents calls deepcopy before calling _event_list on EEG_out, so the main path is safe. But _run_gui calls _event_list(EEG) on the original uncopied dict to build the event-type picker list (_unique_event_types). If EEG["event"] happens to be a bare dict, _run_gui silently rewrites the caller's EEG["event"] from a dict to a single-element list. Minimal fix: make _event_list non-mutating, e.g. return [events] without reassigning EEG["event"], or guard the mutation with a else path.

2. BOM and missing trailing newline in pop_adjustevents.rst

The generated RST file starts with a UTF-8 BOM (\xef\xbb\xbf) and has no trailing newline. Both can silently break Sphinx/tooling. Strip the BOM and add a final newline.

---

Design and API Issues

3. CallbackSpec is frozen=True but contains a mutable dict — false immutability (spec.py:422)

@dataclass(frozen=True)
class CallbackSpec:
    name: str
    params: dict[str, Any] = field(default_factory=dict)

frozen=True prevents re-binding params, but callers can still do spec.callback.params["srate"] = .... Since specs are intended as stable, shareable constants, either use types.MappingProxyType to wrap params after construction, or document the limitation. ControlSpec.value: Any has the same issue if a mutable value is passed.

4. When both edit_time and edit_samples are non-empty from the GUI, addms silently wins (pop_adjustevents.py:617–620)

options_for_com.append(("addms", addms))
elif addsamples is not None:
    options_for_com.append(("addsamples", addsamples))

The sync callbacks are designed to keep both fields in sync, so in practice both will be set. The implicit precedence (addms wins) is technically correct EEGLAB behavior, but it is not documented anywhere. A comment noting this, or a check that addms-derived and addsamples-derived shifts are consistent, would make the intent explicit.

5. pop_adjustevents is placed under "Epoching and Selection" in api/index.rst

eegprep.pop_epoch
eegprep.pop_select
eegprep.pop_adjustevents   # ← placed here
eegprep.eeg_eegrej

The preprocessing.rst correctly files it under "Event Operations". The index placement under epoching is inconsistent and could confuse readers — consider moving it to an "Event Operations" section or creating one in the index.

6. gui extra is not included in any umbrella extra (pyproject.toml)

The PR adds [gui] as an opt-in extra, but if the project has a [dev] or [all] extra (common pattern), it should be included there so developers don't have to discover it separately. Worth checking and updating if those extras exist.

---

Test Coverage Gaps

7. No test for GUI cancel flow

_run_gui returns None on cancel, and pop_adjustevents short-circuits with return EEG, "". The existing test_gui_renderer_decodes_options tests the accepted path, but there is no test for a renderer that returns None (cancel). Add a case with FakeRenderer.run returning None and assert the original EEG is returned unchanged and com == "".

8. No test for ndarray event format

_event_list has an explicit isinstance(events, np.ndarray) branch, but no test exercises it. An EEG dict with "event": np.array([...], dtype=object) should be tested end-to-end.

9. No test for the dict-event mutation side-effect

The bug in point #1 above has no regression test. Once fixed, a test that calls pop_adjustevents(..., gui=True, ...) with a renderer on an EEG where event is a plain dict (not a list) should verify the original EEG["event"] is not mutated.

---

Minor / Nit

10. _build_widget looks up initial_values.get(None, ...) when tag is None

value = initial_values.get(control.tag, control.value)

None is a valid Python dict key, so this doesn't error, but it's semantically odd to look up None. Guard with if control.tag before the lookup.

11. _row_width geometry clamping may mask layout bugs

row_geom = geometry[min(row, len(geometry) - 1)]

Silently clamping to the last geometry row when row >= len(geometry) could produce incorrect column widths without any indication. At least add an assertion or warnings.warn for the out-of-bounds case during debug/test runs.

12. _select_event_types only selects one type per click

The MATLAB original uses pop_chansel(unique({tmpevent.type})) which opens a multi-select dialog. The Python implementation opens a single-item picker and appends each selected item with a space. This is a known and documented divergence (it's not in known_differences), so it should be added there.

---

Summary

Severity	Count
Bug	2
Design/API	4
Test gap	3
Minor/Nit	3

The GUI foundation architecture is solid and the test strategy (fake renderer, spec-level tests, MATLAB parity) is exactly right. The main items to address before merging are the _event_list mutation bug (#1), the false immutability of CallbackSpec (#3), and the test gaps for cancel and ndarray paths (#7, #8).

[suraj-ranganath]

Closing because this PR is no longer relevant.