Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REF: create a window singleton for access throughout superscore #115

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions docs/source/upcoming_release_notes/115-ref_window_singleton.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
115 ref_window_singleton
########################

API Breaks
----------
- N/A

Features
--------
- N/A

Bugfixes
--------
- N/A

Maintenance
-----------
- Makes Window a QtSingleton, and adds an access function for use throughout superscore ui widgets
Uses this singleton to access methods that were passed around manually in the past as optional arguments or set attributes (open_page_slot, client)

Contributors
------------
- tangkong
8 changes: 8 additions & 0 deletions superscore/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
Root, Setpoint, Severity, Snapshot, Status)
from superscore.tests.ioc import IOCFactory
from superscore.widgets.views import EntryItem
from superscore.widgets.window import Window


def linac_data() -> Root:
Expand Down Expand Up @@ -1003,3 +1004,10 @@ def nest_depth(entry: Union[Nestable, EntryItem]) -> int:
q.append((child, depth+1))

return max(depths)


@pytest.fixture(autouse=True)
def teardown_window_singleton():
# clean up window autouse fixture inside this module only
yield
Window._instance = None
11 changes: 6 additions & 5 deletions superscore/tests/test_page.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""Largely smoke tests for various pages"""

from unittest.mock import MagicMock, patch
from unittest.mock import MagicMock, PropertyMock, patch

import pytest
from pytestqt.qtbot import QtBot
Expand Down Expand Up @@ -219,10 +219,11 @@ def test_open_page_slot(
page_fixture: str,
request: pytest.FixtureRequest,
):
page: BaseParameterPage = request.getfixturevalue(page_fixture)
page.open_page_slot = MagicMock()
page.open_rbv_button.clicked.emit()
assert page.open_page_slot.called
with patch("superscore.widgets.page.entry.BaseParameterPage.open_page_slot",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test design question: when would you choose unittest.mock.patch over the pytest monkeypatch fixture and vice versa?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's not much of a difference most of the time. In this case I wanted to mock a property, and the first example I found was through unittest.mock. I've been using MagicMock and finding it very convenient, but it is some deep mystical magic

I should probably be better about sticking to one, but we have access to both.

new_callable=PropertyMock):
page: BaseParameterPage = request.getfixturevalue(page_fixture)
page.open_rbv_button.clicked.emit()
assert page.open_page_slot.called


@pytest.mark.parametrize(
Expand Down
15 changes: 15 additions & 0 deletions superscore/widgets/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,18 @@ def _get_icon_map():


ICON_MAP = _get_icon_map()


def get_window():
"""
Return the window singleton if it already exists, to allow other widgets to
access its members.
Must not be called in the code path that results from Window.__init__.
A good (safe) rule of thumb is to make sure this function cannot be reached
from any widget's __init__ method.
Hides import in __init__ to avoid circular imports.
"""
from .window import Window
if Window._instance is None:
return
return Window()
46 changes: 44 additions & 2 deletions superscore/widgets/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@
from __future__ import annotations

from pathlib import Path
from typing import ClassVar, List
from typing import ClassVar, List, Optional
from weakref import WeakValueDictionary

from pcdsutils.qt.designer_display import DesignerDisplay
from qtpy import QtCore, QtWidgets

from superscore.client import Client
from superscore.qt_helpers import QDataclassBridge, QDataclassList
from superscore.type_hints import AnyDataclass
from superscore.type_hints import AnyDataclass, OpenPageSlot
from superscore.utils import SUPERSCORE_SOURCE_PATH
from superscore.widgets import get_window
from superscore.widgets.manip_helpers import (FrameOnEditFilter,
match_line_edit_text_width)

Expand Down Expand Up @@ -423,3 +425,43 @@ def __call__(cls, *args, **kwargs):
if cls._instance is None:
cls._instance = super().__call__(*args, **kwargs)
return cls._instance


class WindowLinker:
"""
Mixin class that provides access methods for resources held by the main Window.
These include:
- client: first attempts to grab the client set at init, if none exists use
the Window's client
- open_page_slot: grabs the slot from the Window
"""

def __init__(self, *args, client: Optional[Client] = None, **kwargs) -> None:
self._client = client
super().__init__(*args, **kwargs)

@property
def client(self) -> Optional[Client]:
# Return the provided client if it exists, grab the Window's otherwise
if self._client is not None:
return self._client
else:
window = get_window()
if window is not None:
return window.client

@client.setter
def client(self, client: Client):
if not isinstance(client, Client):
raise TypeError(f"Cannot set a {type(client)} as a client")

if client is self._client:
return

self._client = client
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that this allows different widgets to use different clients. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the context of the superscore app, this isn't something we expect to happen. We should only have one client for one superscore instance and all its child pages. I'm not particularly worried about a new client getting slipped into the superscore app, but this does provide that possibility

Personally, specifying this per widget lets me test widgets individually, free of needing to create a Window.


@property
def open_page_slot(self) -> Optional[OpenPageSlot]:
window = get_window()
if window is not None:
return window.open_page
12 changes: 3 additions & 9 deletions superscore/widgets/page/collection_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@
from qtpy import QtCore, QtWidgets
from qtpy.QtGui import QCloseEvent

from superscore.client import Client
from superscore.model import Collection, Entry, Parameter
from superscore.type_hints import OpenPageSlot
from superscore.widgets.core import DataWidget, Display, NameDescTagsWidget
from superscore.widgets.core import (DataWidget, Display, NameDescTagsWidget,
WindowLinker)
from superscore.widgets.enhanced import FilterComboBox
from superscore.widgets.manip_helpers import insert_widget
from superscore.widgets.views import (BaseTableEntryModel, LivePVHeader,
Expand All @@ -17,7 +16,7 @@
logger = logging.getLogger(__name__)


class CollectionBuilderPage(Display, DataWidget):
class CollectionBuilderPage(Display, DataWidget, WindowLinker):
filename = 'collection_builder_page.ui'
data: Collection

Expand Down Expand Up @@ -46,16 +45,12 @@ class CollectionBuilderPage(Display, DataWidget):
def __init__(
self,
*args,
client: Client,
data: Optional[Collection] = None,
open_page_slot: Optional[OpenPageSlot] = None,
**kwargs
):
if data is None:
data = Collection()
super().__init__(*args, data=data, **kwargs)
self.client = client
self.open_page_slot = open_page_slot
self.tree_model = None
self._coll_options: list[Collection] = []
self._title = self.data.title
Expand Down Expand Up @@ -90,7 +85,6 @@ def setup_ui(self):

self.tree_view.client = self.client
self.tree_view.set_data(self.data)
self.tree_view.open_page_slot = self.open_page_slot
self.tree_model: RootTree = self.tree_view.model()

self.sub_coll_table_view.data_updated.connect(self.tree_model.refresh_tree)
Expand Down
12 changes: 2 additions & 10 deletions superscore/widgets/page/diff.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@
from PyQt5.QtGui import QCloseEvent
from qtpy import QtCore, QtGui, QtWidgets

from superscore.client import Client
from superscore.compare import DiffType, EntryDiff
from superscore.model import Entry
from superscore.type_hints import OpenPageSlot
from superscore.widgets.core import Display
from superscore.widgets.core import Display, WindowLinker
from superscore.widgets.views import (EntryItem, LivePVHeader,
LivePVTableModel, LivePVTableView,
NestableHeader, NestableTableModel,
Expand Down Expand Up @@ -246,7 +244,7 @@ class Side(Flag):
RIGHT = False


class DiffPage(Display, QtWidgets.QWidget):
class DiffPage(Display, QtWidgets.QWidget, WindowLinker):
"""
Diff View Page. Compares two ``Entry`` objects, attempting to highlight
differences where appropriate
Expand Down Expand Up @@ -285,17 +283,13 @@ class DiffPage(Display, QtWidgets.QWidget):
def __init__(
self,
*args,
client: Client,
open_page_slot: Optional[OpenPageSlot] = None,
l_entry: Optional[Entry] = None,
r_entry: Optional[Entry] = None,
**kwargs
):
super().__init__(*args, **kwargs)
self.client = client
self.l_entry = l_entry
self.r_entry = r_entry
self.open_page_slot = open_page_slot
self._linked_uuids: Dict[UUID, UUID] = BiDict()

self.widget_map = {
Expand Down Expand Up @@ -332,13 +326,11 @@ def setup_ui(self):

pv_view: LivePVTableView = self.widget_map[side]['pv']
pv_view._model_cls = DiffPVTableModel
pv_view.open_page_slot = self.open_page_slot
pv_view.client = self.client

nest_view: NestableTableView = self.widget_map[side]['nest']
nest_view._model_cls = DiffNestableTableModel
nest_view.client = self.client
nest_view.open_page_slot = self.open_page_slot

self.set_entry(self.l_entry, Side.LEFT)
self.set_entry(self.r_entry, Side.RIGHT)
Expand Down
19 changes: 5 additions & 14 deletions superscore/widgets/page/entry.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@
from qtpy import QtWidgets
from qtpy.QtGui import QCloseEvent

from superscore.client import Client
from superscore.control_layers._base_shim import EpicsData
from superscore.model import (Collection, Nestable, Parameter, Readback,
Setpoint, Severity, Snapshot, Status)
from superscore.type_hints import AnyEpicsType, OpenPageSlot
from superscore.widgets.core import DataWidget, Display, NameDescTagsWidget
from superscore.type_hints import AnyEpicsType
from superscore.widgets.core import (DataWidget, Display, NameDescTagsWidget,
WindowLinker)
from superscore.widgets.manip_helpers import (insert_widget,
match_line_edit_text_width)
from superscore.widgets.thread_helpers import BusyCursorThread
Expand All @@ -25,7 +25,7 @@
logger = logging.getLogger(__name__)


class NestablePage(Display, DataWidget):
class NestablePage(Display, DataWidget, WindowLinker):
filename = 'nestable_page.ui'

meta_placeholder: QtWidgets.QWidget
Expand All @@ -42,15 +42,11 @@ def __init__(
self,
*args,
data: Nestable,
client: Client,
editable: bool = False,
open_page_slot: Optional[OpenPageSlot] = None,
**kwargs
):
super().__init__(*args, data=data, **kwargs)
self.client = client
self.editable = editable
self.open_page_slot = open_page_slot
self._last_data = deepcopy(self.data)
self.setup_ui()

Expand All @@ -61,7 +57,6 @@ def setup_ui(self):
# show tree view
self.tree_view.client = self.client
self.tree_view.set_data(self.data)
self.tree_view.open_page_slot = self.open_page_slot

self.sub_pv_table_view.client = self.client
self.sub_pv_table_view.set_data(self.data)
Expand Down Expand Up @@ -115,7 +110,7 @@ class SnapshotPage(NestablePage):
data: Snapshot


class BaseParameterPage(Display, DataWidget):
class BaseParameterPage(Display, DataWidget, WindowLinker):
filename = 'parameter_page.ui'

meta_placeholder: QtWidgets.QWidget
Expand Down Expand Up @@ -159,15 +154,11 @@ class BaseParameterPage(Display, DataWidget):
def __init__(
self,
*args,
client: Client,
editable: bool = False,
open_page_slot: Optional[OpenPageSlot] = None,
**kwargs
):
super().__init__(*args, **kwargs)
self.client = client
self.editable = editable
self.open_page_slot = open_page_slot
self.value_stored_widget = None
self.edata = None
self._edata_thread: Optional[BusyCursorThread] = None
Expand Down
21 changes: 10 additions & 11 deletions superscore/widgets/page/search.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,17 @@
from qtpy import QtCore, QtWidgets

from superscore.backends.core import SearchTerm
from superscore.client import Client
from superscore.model import Collection, Entry, Readback, Setpoint, Snapshot
from superscore.type_hints import OpenPageSlot
from superscore.widgets import ICON_MAP
from superscore.widgets.core import Display
from superscore.widgets import ICON_MAP, get_window
from superscore.widgets.core import Display, WindowLinker
from superscore.widgets.views import (BaseTableEntryModel, ButtonDelegate,
HeaderEnum)

logger = logging.getLogger(__name__)


class SearchPage(Display, QtWidgets.QWidget):
class SearchPage(Display, QtWidgets.QWidget, WindowLinker):
"""
Widget for searching and displaying Entry's. Contains a variety of filter
option input widgets, a sortable table view, and a filter management table.
Expand Down Expand Up @@ -56,13 +55,9 @@ class SearchPage(Display, QtWidgets.QWidget):
def __init__(
self,
*args,
client: Client,
open_page_slot: Optional[OpenPageSlot] = None,
**kwargs
) -> None:
super().__init__(*args, **kwargs)
self.client = client
self.open_page_slot = open_page_slot
self.model: Optional[ResultModel] = None

self.type_checkboxes: List[QtWidgets.QCheckBox] = [
Expand All @@ -86,7 +81,7 @@ def setup_ui(self) -> None:

# set up filter table view
self.model = ResultModel(entries=[])
self.proxy_model = ResultFilterProxyModel(open_page_slot=self.open_page_slot)
self.proxy_model = ResultFilterProxyModel()
self.proxy_model.setSourceModel(self.model)
self.results_table_view.setModel(self.proxy_model)
self.results_table_view.setSortingEnabled(True)
Expand Down Expand Up @@ -235,13 +230,17 @@ class ResultFilterProxyModel(QtCore.QSortFilterProxyModel):
def __init__(
self,
*args,
open_page_slot: Optional[OpenPageSlot] = None,
**kwargs
) -> None:
super().__init__(*args, **kwargs)
self.open_page_slot = open_page_slot
self.name_regexp = QtCore.QRegularExpression()

@property
def open_page_slot(self) -> Optional[OpenPageSlot]:
window = get_window()
if window is not None:
return window.open_page

def filterAcceptsRow(
self,
source_row: int,
Expand Down
Loading