Fix `PlotItem` cleanup in `GapOverlayMngr`
`PlotItem.addItem()` records overlays outside the Qt scene. Direct scene removal leaves stale plot refs and range bookkeeping behind. Track each annotation's owning plot and remove it through the same API used for insertion. Exclude pixel-sized arrows from data bounds. Deats, - centralize replacement, hiding and owner teardown; - retain scene removal as a defensive fallback; - cover real Qt registries and duplicate-FQME chart identity. (this patch was generated in some part by `opencode` using `gpt-5.6-sol` (`openai`))chart_local_gapper
parent
f65df6a138
commit
78982d8280
|
|
@ -31,8 +31,10 @@ from piker.types import Struct
|
||||||
from ._annotate import GapAnnotations
|
from ._annotate import GapAnnotations
|
||||||
|
|
||||||
if TYPE_CHECKING:
|
if TYPE_CHECKING:
|
||||||
from piker.ui.qt import QGraphicsItem
|
|
||||||
from PyQt6.QtWidgets import QGraphicsScene
|
from PyQt6.QtWidgets import QGraphicsScene
|
||||||
|
from pyqtgraph import PlotItem
|
||||||
|
|
||||||
|
from piker.ui.qt import QGraphicsItem
|
||||||
from ._chart import ChartPlotWidget
|
from ._chart import ChartPlotWidget
|
||||||
from ._dataviz import Viz
|
from ._dataviz import Viz
|
||||||
from ._display import DisplayState
|
from ._display import DisplayState
|
||||||
|
|
@ -226,6 +228,37 @@ class GapOverlayMngr:
|
||||||
],
|
],
|
||||||
bool, # `SetGapOverlay.visible`
|
bool, # `SetGapOverlay.visible`
|
||||||
] = {}
|
] = {}
|
||||||
|
self._plots: dict[
|
||||||
|
int, # `id(GapAnnotations)`
|
||||||
|
PlotItem, # owning `Viz.plot`
|
||||||
|
] = {}
|
||||||
|
|
||||||
|
def _remove_annot(
|
||||||
|
self,
|
||||||
|
aid: int,
|
||||||
|
|
||||||
|
) -> None:
|
||||||
|
'''
|
||||||
|
Remove one gap item from all manager and plot registries.
|
||||||
|
|
||||||
|
'''
|
||||||
|
annot: QGraphicsItem|None = self.annots.pop(
|
||||||
|
aid,
|
||||||
|
None,
|
||||||
|
)
|
||||||
|
plot: PlotItem|None = self._plots.pop(aid, None)
|
||||||
|
if annot is None:
|
||||||
|
return
|
||||||
|
|
||||||
|
if plot is not None:
|
||||||
|
plot.removeItem(annot)
|
||||||
|
return
|
||||||
|
|
||||||
|
# Defensive fallback for manager state created before the
|
||||||
|
# owning `Viz.plot` registry was introduced.
|
||||||
|
scene: QGraphicsScene|None = annot.scene()
|
||||||
|
if scene:
|
||||||
|
scene.removeItem(annot)
|
||||||
|
|
||||||
def apply(
|
def apply(
|
||||||
self,
|
self,
|
||||||
|
|
@ -277,16 +310,9 @@ class GapOverlayMngr:
|
||||||
)
|
)
|
||||||
aid: int|None = self._layers.pop(layer_key, None)
|
aid: int|None = self._layers.pop(layer_key, None)
|
||||||
if aid is not None:
|
if aid is not None:
|
||||||
annot: QGraphicsItem|None = self.annots.pop(
|
|
||||||
aid,
|
|
||||||
None,
|
|
||||||
)
|
|
||||||
if aids is not None:
|
if aids is not None:
|
||||||
aids.discard(aid)
|
aids.discard(aid)
|
||||||
if annot is not None:
|
self._remove_annot(aid)
|
||||||
scene: QGraphicsScene|None = annot.scene()
|
|
||||||
if scene:
|
|
||||||
scene.removeItem(annot)
|
|
||||||
|
|
||||||
if owner == 'chart-local':
|
if owner == 'chart-local':
|
||||||
self._visible[
|
self._visible[
|
||||||
|
|
@ -318,11 +344,15 @@ class GapOverlayMngr:
|
||||||
fqme=req.fqme,
|
fqme=req.fqme,
|
||||||
timeframe=req.timeframe,
|
timeframe=req.timeframe,
|
||||||
)
|
)
|
||||||
viz.plot.addItem(gaps_item)
|
viz.plot.addItem(
|
||||||
|
gaps_item,
|
||||||
|
ignoreBounds=True,
|
||||||
|
)
|
||||||
gaps_item._chart = chart
|
gaps_item._chart = chart
|
||||||
|
|
||||||
new_aid: int = id(gaps_item)
|
new_aid: int = id(gaps_item)
|
||||||
self.annots[new_aid] = gaps_item
|
self.annots[new_aid] = gaps_item
|
||||||
|
self._plots[new_aid] = viz.plot
|
||||||
self._layers[layer_key] = new_aid
|
self._layers[layer_key] = new_aid
|
||||||
if aids is not None:
|
if aids is not None:
|
||||||
aids.add(new_aid)
|
aids.add(new_aid)
|
||||||
|
|
@ -403,11 +433,4 @@ class GapOverlayMngr:
|
||||||
layer_key: tuple[str, int, str, float]
|
layer_key: tuple[str, int, str, float]
|
||||||
for layer_key in owner_keys:
|
for layer_key in owner_keys:
|
||||||
aid: int = self._layers.pop(layer_key)
|
aid: int = self._layers.pop(layer_key)
|
||||||
annot: QGraphicsItem|None = self.annots.pop(
|
self._remove_annot(aid)
|
||||||
aid,
|
|
||||||
None,
|
|
||||||
)
|
|
||||||
if annot is not None:
|
|
||||||
scene: QGraphicsScene|None = annot.scene()
|
|
||||||
if scene:
|
|
||||||
scene.removeItem(annot)
|
|
||||||
|
|
|
||||||
|
|
@ -2,10 +2,18 @@
|
||||||
Typed chart-local gap-overlay regressions.
|
Typed chart-local gap-overlay regressions.
|
||||||
|
|
||||||
'''
|
'''
|
||||||
|
from collections.abc import Iterator
|
||||||
|
import os
|
||||||
|
from types import SimpleNamespace
|
||||||
|
|
||||||
|
os.environ['QT_QPA_PLATFORM'] = 'offscreen'
|
||||||
|
|
||||||
import msgspec
|
import msgspec
|
||||||
import numpy as np
|
import numpy as np
|
||||||
|
import pyqtgraph as pg
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
|
from piker.ui._annotate import GapAnnotations
|
||||||
from piker.ui._gaps import (
|
from piker.ui._gaps import (
|
||||||
GapOverlay,
|
GapOverlay,
|
||||||
GapOverlayMngr,
|
GapOverlayMngr,
|
||||||
|
|
@ -14,6 +22,118 @@ from piker.ui._gaps import (
|
||||||
SetGapOverlay,
|
SetGapOverlay,
|
||||||
gap_specs_from_ohlcv,
|
gap_specs_from_ohlcv,
|
||||||
)
|
)
|
||||||
|
from piker.ui.qt import QApplication
|
||||||
|
|
||||||
|
|
||||||
|
FQME: str = 'gap.test'
|
||||||
|
|
||||||
|
|
||||||
|
class _ChartStub:
|
||||||
|
'''
|
||||||
|
Real Qt plot with the small chart API required by `gapman`.
|
||||||
|
|
||||||
|
'''
|
||||||
|
def __init__(
|
||||||
|
self,
|
||||||
|
fqme: str,
|
||||||
|
array: np.ndarray,
|
||||||
|
|
||||||
|
) -> None:
|
||||||
|
self.fqme: str = fqme
|
||||||
|
self.widget: pg.PlotWidget = pg.PlotWidget()
|
||||||
|
self.viz: SimpleNamespace = SimpleNamespace(
|
||||||
|
plot=self.widget.plotItem,
|
||||||
|
shm=SimpleNamespace(array=array),
|
||||||
|
)
|
||||||
|
|
||||||
|
def get_viz(
|
||||||
|
self,
|
||||||
|
fqme: str,
|
||||||
|
|
||||||
|
) -> SimpleNamespace:
|
||||||
|
'''
|
||||||
|
Return this chart's only viz.
|
||||||
|
|
||||||
|
'''
|
||||||
|
assert fqme == self.fqme
|
||||||
|
return self.viz
|
||||||
|
|
||||||
|
def close(self) -> None:
|
||||||
|
'''
|
||||||
|
Release this test-owned plot widget.
|
||||||
|
|
||||||
|
'''
|
||||||
|
self.widget.close()
|
||||||
|
self.widget.deleteLater()
|
||||||
|
|
||||||
|
|
||||||
|
def _ohlcv_array(
|
||||||
|
times: tuple[float, ...],
|
||||||
|
|
||||||
|
) -> np.ndarray:
|
||||||
|
'''
|
||||||
|
Build the minimal structured OHLCV used by gap rendering.
|
||||||
|
|
||||||
|
'''
|
||||||
|
dtype: np.dtype = np.dtype([
|
||||||
|
('index', 'i8'),
|
||||||
|
('time', 'f8'),
|
||||||
|
('open', 'f8'),
|
||||||
|
('close', 'f8'),
|
||||||
|
])
|
||||||
|
rows: list[tuple[int, float, float, float]] = []
|
||||||
|
i: int
|
||||||
|
time_s: float
|
||||||
|
for i, time_s in enumerate(times):
|
||||||
|
price: float = 100 + i
|
||||||
|
rows.append((i, time_s, price, price + 0.5))
|
||||||
|
|
||||||
|
return np.array(rows, dtype=dtype)
|
||||||
|
|
||||||
|
|
||||||
|
def _display_state(
|
||||||
|
fqme: str,
|
||||||
|
|
||||||
|
) -> tuple[
|
||||||
|
SimpleNamespace,
|
||||||
|
tuple[_ChartStub, _ChartStub],
|
||||||
|
]:
|
||||||
|
'''
|
||||||
|
Build independent realtime and history chart stubs.
|
||||||
|
|
||||||
|
'''
|
||||||
|
rt_chart: _ChartStub = _ChartStub(
|
||||||
|
fqme,
|
||||||
|
_ohlcv_array((1, 2, 4, 5)),
|
||||||
|
)
|
||||||
|
hist_chart: _ChartStub = _ChartStub(
|
||||||
|
fqme,
|
||||||
|
_ohlcv_array((60, 120, 300, 360)),
|
||||||
|
)
|
||||||
|
ds: SimpleNamespace = SimpleNamespace(
|
||||||
|
fqme=fqme,
|
||||||
|
chart=rt_chart,
|
||||||
|
viz=rt_chart.viz,
|
||||||
|
hist_chart=hist_chart,
|
||||||
|
hist_viz=hist_chart.viz,
|
||||||
|
)
|
||||||
|
return ds, (rt_chart, hist_chart)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture(scope='session')
|
||||||
|
def qapp() -> Iterator[QApplication]:
|
||||||
|
'''
|
||||||
|
Keep one offscreen Qt application alive for graphics tests.
|
||||||
|
|
||||||
|
'''
|
||||||
|
app: QApplication|None = QApplication.instance()
|
||||||
|
if app is None:
|
||||||
|
app = QApplication(['piker-gap-tests'])
|
||||||
|
app.setQuitOnLastWindowClosed(False)
|
||||||
|
|
||||||
|
yield app
|
||||||
|
|
||||||
|
app.processEvents()
|
||||||
|
|
||||||
|
|
||||||
def test_gap_specs_and_wire_roundtrip() -> None:
|
def test_gap_specs_and_wire_roundtrip() -> None:
|
||||||
|
|
@ -146,3 +266,180 @@ def test_gap_overlay_unknown_fqme_returns_typed_error() -> None:
|
||||||
|
|
||||||
assert resp.request_id == req.request_id
|
assert resp.request_id == req.request_id
|
||||||
assert resp.error == 'No display state for fqme=gone.test'
|
assert resp.error == 'No display state for fqme=gone.test'
|
||||||
|
|
||||||
|
|
||||||
|
def test_gap_manager_real_qt_lifecycle(
|
||||||
|
qapp: QApplication,
|
||||||
|
|
||||||
|
) -> None:
|
||||||
|
'''
|
||||||
|
Remove overlays from every PyQtGraph registry.
|
||||||
|
|
||||||
|
Gap layers are inserted through `PlotItem.addItem()`. Removing
|
||||||
|
the item directly from its scene leaves stale `PlotItem.items`
|
||||||
|
and `ViewBox.addedItems` refs which can leak memory and affect
|
||||||
|
range calculations. This test uses real offscreen Qt plots,
|
||||||
|
replaces one remotely owned layer, hides it, and proves each
|
||||||
|
registry releases the old graphics object.
|
||||||
|
|
||||||
|
'''
|
||||||
|
ds: SimpleNamespace
|
||||||
|
charts: tuple[_ChartStub, _ChartStub]
|
||||||
|
ds, charts = _display_state(FQME)
|
||||||
|
annots: dict[int, GapAnnotations] = {}
|
||||||
|
gapman: GapOverlayMngr = GapOverlayMngr(
|
||||||
|
dss={FQME: ds},
|
||||||
|
annots=annots,
|
||||||
|
)
|
||||||
|
specs: list[GapSpec] = gap_specs_from_ohlcv(
|
||||||
|
ds.hist_viz.shm.array,
|
||||||
|
period_s=60,
|
||||||
|
)
|
||||||
|
aids: set[int] = set()
|
||||||
|
req: SetGapOverlay = SetGapOverlay(
|
||||||
|
fqme=FQME,
|
||||||
|
timeframe=60,
|
||||||
|
specs=specs,
|
||||||
|
request_id='first',
|
||||||
|
)
|
||||||
|
|
||||||
|
try:
|
||||||
|
first: GapOverlay = gapman.apply(
|
||||||
|
req=req,
|
||||||
|
owner='remote-test',
|
||||||
|
aids=aids,
|
||||||
|
)
|
||||||
|
assert first.aid is not None
|
||||||
|
first_item: GapAnnotations = annots[first.aid]
|
||||||
|
assert first_item.scene() is charts[1].widget.scene()
|
||||||
|
assert first_item in ds.hist_viz.plot.items
|
||||||
|
assert first_item not in ds.hist_viz.plot.vb.addedItems
|
||||||
|
assert aids == {first.aid}
|
||||||
|
|
||||||
|
second: GapOverlay = gapman.apply(
|
||||||
|
req=SetGapOverlay(
|
||||||
|
fqme=FQME,
|
||||||
|
timeframe=60,
|
||||||
|
specs=specs,
|
||||||
|
request_id='second',
|
||||||
|
),
|
||||||
|
owner='remote-test',
|
||||||
|
aids=aids,
|
||||||
|
)
|
||||||
|
assert second.aid is not None
|
||||||
|
assert second.aid != first.aid
|
||||||
|
assert first.aid not in annots
|
||||||
|
assert first_item.scene() is None
|
||||||
|
assert first_item not in ds.hist_viz.plot.items
|
||||||
|
assert first_item not in ds.hist_viz.plot.vb.addedItems
|
||||||
|
assert aids == {second.aid}
|
||||||
|
|
||||||
|
hidden: GapOverlay = gapman.apply(
|
||||||
|
req=SetGapOverlay(
|
||||||
|
fqme=FQME,
|
||||||
|
timeframe=60,
|
||||||
|
specs=specs,
|
||||||
|
visible=False,
|
||||||
|
request_id='hidden',
|
||||||
|
),
|
||||||
|
owner='remote-test',
|
||||||
|
aids=aids,
|
||||||
|
)
|
||||||
|
assert hidden.visible is False
|
||||||
|
assert hidden.aid is None
|
||||||
|
assert aids == set()
|
||||||
|
assert annots == {}
|
||||||
|
assert gapman._layers == {}
|
||||||
|
assert gapman._plots == {}
|
||||||
|
item: object
|
||||||
|
for item in ds.hist_viz.plot.items:
|
||||||
|
assert not isinstance(item, GapAnnotations)
|
||||||
|
|
||||||
|
local: GapOverlay = gapman.apply(
|
||||||
|
req=SetGapOverlay(
|
||||||
|
fqme=FQME,
|
||||||
|
timeframe=60,
|
||||||
|
specs=specs,
|
||||||
|
request_id='local',
|
||||||
|
),
|
||||||
|
)
|
||||||
|
remote: GapOverlay = gapman.apply(
|
||||||
|
req=SetGapOverlay(
|
||||||
|
fqme=FQME,
|
||||||
|
timeframe=60,
|
||||||
|
specs=specs,
|
||||||
|
request_id='remote',
|
||||||
|
),
|
||||||
|
owner='remote-test',
|
||||||
|
)
|
||||||
|
assert local.aid is not None
|
||||||
|
assert remote.aid is not None
|
||||||
|
gapman.remove_owner('remote-test')
|
||||||
|
assert local.aid in annots
|
||||||
|
assert remote.aid not in annots
|
||||||
|
assert annots[local.aid].scene() is not None
|
||||||
|
gapman.remove_owner('chart-local')
|
||||||
|
assert annots == {}
|
||||||
|
finally:
|
||||||
|
chart: _ChartStub
|
||||||
|
for chart in charts:
|
||||||
|
chart.close()
|
||||||
|
qapp.processEvents()
|
||||||
|
|
||||||
|
|
||||||
|
def test_duplicate_fqme_layers_use_local_chart_identity(
|
||||||
|
qapp: QApplication,
|
||||||
|
|
||||||
|
) -> None:
|
||||||
|
'''
|
||||||
|
Keep cached displays for one FQME in separate local layers.
|
||||||
|
|
||||||
|
The actor-global display lookup is keyed by FQME and can
|
||||||
|
represent only its currently registered display. Local cached
|
||||||
|
charts still pass their exact `DisplayState` into
|
||||||
|
`GapOverlayMngr.refresh()`. Build two independent chart pairs
|
||||||
|
with the same FQME, refresh both, and prove their layers survive
|
||||||
|
independently.
|
||||||
|
|
||||||
|
'''
|
||||||
|
first_ds: SimpleNamespace
|
||||||
|
first_charts: tuple[_ChartStub, _ChartStub]
|
||||||
|
first_ds, first_charts = _display_state(FQME)
|
||||||
|
second_ds: SimpleNamespace
|
||||||
|
second_charts: tuple[_ChartStub, _ChartStub]
|
||||||
|
second_ds, second_charts = _display_state(FQME)
|
||||||
|
annots: dict[int, GapAnnotations] = {}
|
||||||
|
gapman: GapOverlayMngr = GapOverlayMngr(
|
||||||
|
dss={FQME: second_ds},
|
||||||
|
annots=annots,
|
||||||
|
)
|
||||||
|
|
||||||
|
try:
|
||||||
|
first: GapOverlay = gapman.refresh(
|
||||||
|
ds=first_ds,
|
||||||
|
timeframe=60,
|
||||||
|
)
|
||||||
|
second: GapOverlay = gapman.refresh(
|
||||||
|
ds=second_ds,
|
||||||
|
timeframe=60,
|
||||||
|
)
|
||||||
|
assert first.aid is not None
|
||||||
|
assert second.aid is not None
|
||||||
|
assert first.aid != second.aid
|
||||||
|
assert len(gapman._layers) == 2
|
||||||
|
chart_ids: set[int] = set()
|
||||||
|
key: tuple[str, int, str, float]
|
||||||
|
for key in gapman._layers:
|
||||||
|
chart_ids.add(key[1])
|
||||||
|
assert chart_ids == {
|
||||||
|
id(first_ds.hist_chart),
|
||||||
|
id(second_ds.hist_chart),
|
||||||
|
}
|
||||||
|
assert annots[first.aid].scene() is not None
|
||||||
|
assert annots[second.aid].scene() is not None
|
||||||
|
finally:
|
||||||
|
gapman.remove_owner('chart-local')
|
||||||
|
chart: _ChartStub
|
||||||
|
for chart in (*first_charts, *second_charts):
|
||||||
|
chart.close()
|
||||||
|
qapp.processEvents()
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue