-
Notifications
You must be signed in to change notification settings - Fork 26
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
ENH: layout changes for compact complex screens + row positioner widget #563
Changes from 36 commits
9533338
7549719
47ea46a
76c8c74
3ca4ad5
ac43191
4f102df
a00bba4
a75ac10
06c3966
5a42a84
ccb8f40
c0709d4
b82f263
b0ac7cf
df39abf
751c08a
2c7dc7f
9b9aaf4
e919aae
284375b
d5dcd3d
27d5c0c
9138889
6e1afa5
5d6f402
923e021
b4524d9
1ec1846
d207920
e62c574
402b747
f0cd1ab
3190483
c7322a2
45c6e5c
4b616c5
aaf937d
8da1096
f7ba0db
60763fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
563 row-positioner | ||
################## | ||
|
||
API Changes | ||
----------- | ||
- ``TyphosNoteEdit`` now supports ``.add_device()`` like other typhos widgets. | ||
This is alongside its original ``setup_data`` API. | ||
- ``TyphosDeviceDisplay`` composite heuristics have been removed in favor of | ||
simpler methods, described in the features section. | ||
|
||
Features | ||
-------- | ||
- ``TyphosNoteEdit`` is now a ``TyphosBase`` object and is accessible in the Qt | ||
designer. | ||
- Added new designable widget ``TyphosPositionerRowWidget``. This compact | ||
positioner widget makes dense motor-heavy screens much more space efficient. | ||
- The layout method for ``TyphosDeviceDisplay`` has changed. For large device trees, | ||
it now favors showing the compact "embedded" screens over detailed screens. The order | ||
of priority is now as follows: | ||
- For top-level devices (e.g., ``at2l0``), the template load priority is as follows: | ||
|
||
* Happi-defined values (``"detailed_screen"``, ``embedded_screen"``, ``"engineering_screen"``) | ||
* Device-specific screens, if available (named as ``ClassNameHere.detailed.ui``) | ||
* The detailed tree, if the device has sub-devices | ||
* The default templates | ||
|
||
- For nested displays in a device tree, sub-device (e.g., ``at2l0.blade_01``) | ||
template load priority is as follows: | ||
|
||
* Device-specific screens, if available (named as ``ClassNameHere.embedded.ui``) | ||
* The detailed tree, if the device has sub-devices | ||
* The default templates (``embedded_screen.ui``) | ||
|
||
Bugfixes | ||
-------- | ||
- For devices which do not require keyword arguments to instantiate, the typhos | ||
CLI will no longer require an empty dictionary. That is, ``$ typhos | ||
ophyd.sim.SynAxis[]`` is equivalent to ``$ typhos ophyd.sim.SynAxis[{}]``. | ||
As before, ophyd's required "name" keyword argument is filled in by typhos by | ||
default. | ||
|
||
|
||
Maintenance | ||
----------- | ||
- N/A | ||
|
||
Contributors | ||
------------ | ||
- klauer | ||
- ZLLentz |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -361,6 +361,12 @@ def generate_context_menu(self): | |
if not display: | ||
return base_menu | ||
|
||
for cls in type(display.device).mro(): | ||
if cls.__name__ in ("BlueskyInterface", "Device"): | ||
break | ||
|
||
base_menu.addSection(cls.__name__) | ||
|
||
klauer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
base_menu.addSection('Templates') | ||
display._generate_template_menu(base_menu) | ||
|
||
|
@@ -962,11 +968,6 @@ class TyphosDeviceDisplay(utils.TyphosBase, widgets.TyphosDesignerMixin, | |
layout. | ||
If omitted, scroll_option is used instead. | ||
|
||
composite_heuristics : bool, optional | ||
Enable composite heuristics, which may change the suggested detailed | ||
screen based on the contents of the added device. See also | ||
:meth:`.suggest_composite_screen`. | ||
|
||
embedded_templates : list, optional | ||
List of embedded templates to use in addition to those found on disk. | ||
|
||
|
@@ -990,25 +991,20 @@ class TyphosDeviceDisplay(utils.TyphosBase, widgets.TyphosDesignerMixin, | |
# Template types and defaults | ||
Q_ENUMS(_DisplayTypes) | ||
TemplateEnum = DisplayTypes # For convenience | ||
|
||
device_count_threshold = 0 | ||
signal_count_threshold = 30 | ||
template_changed = QtCore.Signal(object) | ||
|
||
def __init__( | ||
self, | ||
parent: Optional[QtWidgets.QWidget] = None, | ||
*, | ||
scrollable: Optional[bool] = None, | ||
composite_heuristics: bool = True, | ||
embedded_templates: Optional[list[str]] = None, | ||
detailed_templates: Optional[list[str]] = None, | ||
engineering_templates: Optional[list[str]] = None, | ||
display_type: Union[DisplayTypes, str, int] = 'detailed_screen', | ||
scroll_option: Union[ScrollOptions, str, int] = 'auto', | ||
nested: bool = False, | ||
): | ||
self._composite_heuristics = composite_heuristics | ||
self._current_template = None | ||
self._forced_template = '' | ||
self._macros = {} | ||
|
@@ -1021,6 +1017,12 @@ def __init__( | |
self.templates = {name: [] for name in DisplayTypes.names} | ||
self._display_type = normalize_display_type(display_type) | ||
|
||
if nested and self._display_type == DisplayTypes.detailed_screen: | ||
# All nested displays should be embedded by default. | ||
# Based on if they have subdevices, they may become detailed | ||
# during the template loading process | ||
self._display_type = DisplayTypes.embedded_screen | ||
|
||
instance_templates = { | ||
'embedded_screen': embedded_templates or [], | ||
'detailed_screen': detailed_templates or [], | ||
|
@@ -1053,15 +1055,6 @@ def __init__( | |
else: | ||
self.scroll_option = ScrollOptions.no_scroll | ||
|
||
@Property(bool) | ||
def composite_heuristics(self): | ||
"""Allow composite screen to be suggested first by heuristics.""" | ||
return self._composite_heuristics | ||
|
||
@composite_heuristics.setter | ||
def composite_heuristics(self, composite_heuristics): | ||
self._composite_heuristics = bool(composite_heuristics) | ||
|
||
@Property(_ScrollOptions) | ||
def scroll_option(self) -> ScrollOptions: | ||
"""Place the display in a scrollable area.""" | ||
|
@@ -1087,27 +1080,31 @@ def hideEmpty(self, checked): | |
if checked != self._hide_empty: | ||
self._hide_empty = checked | ||
|
||
@property | ||
def _layout_in_scroll_area(self) -> bool: | ||
"""Layout the widget in the scroll area or not, based on settings.""" | ||
if self.scroll_option == ScrollOptions.auto: | ||
if self.display_type == DisplayTypes.embedded_screen: | ||
return False | ||
return True | ||
elif self.scroll_option == ScrollOptions.scrollbar: | ||
return True | ||
elif self.scroll_option == ScrollOptions.no_scroll: | ||
return False | ||
return True | ||
|
||
def _move_display_to_layout(self, widget): | ||
if not widget: | ||
return | ||
|
||
widget.setParent(None) | ||
if self.scroll_option == ScrollOptions.auto: | ||
if self.display_type == DisplayTypes.embedded_screen: | ||
scrollable = False | ||
else: | ||
scrollable = True | ||
elif self.scroll_option == ScrollOptions.scrollbar: | ||
scrollable = True | ||
elif self.scroll_option == ScrollOptions.no_scroll: | ||
scrollable = False | ||
else: | ||
scrollable = True | ||
|
||
scrollable = self._layout_in_scroll_area | ||
if scrollable: | ||
self._scroll_area.setWidget(widget) | ||
else: | ||
self.layout().addWidget(widget) | ||
layout: QtWidgets.QVBoxLayout = self.layout() | ||
layout.addWidget(widget, alignment=QtCore.Qt.AlignTop) | ||
|
||
self._scroll_area.setVisible(scrollable) | ||
|
||
|
@@ -1292,12 +1289,12 @@ def size_hint(*args, **kwargs): | |
self.template_changed.emit(template) | ||
|
||
def minimumSizeHint(self) -> QtCore.QSize: | ||
if self._scroll_area is None: | ||
return super().minimumSizeHint() | ||
return QtCore.QSize( | ||
self._scroll_area.viewportSizeHint().width(), | ||
super().minimumSizeHint().height(), | ||
) | ||
if self._layout_in_scroll_area: | ||
return QtCore.QSize( | ||
self._scroll_area.viewportSizeHint().width(), | ||
super().minimumSizeHint().height(), | ||
) | ||
return super().minimumSizeHint() | ||
|
||
@property | ||
def display_widget(self): | ||
|
@@ -1447,19 +1444,20 @@ def search_for_templates(self): | |
logger.debug('Adding macro template %s: %s (total=%d)', | ||
display_type, template, len(template_list)) | ||
|
||
# 2. Composite heuristics, if enabled | ||
if self._composite_heuristics and view == 'detailed': | ||
if self.suggest_composite_screen(cls): | ||
template_list.append(DETAILED_TREE_TEMPLATE) | ||
|
||
# 3. Templates based on class hierarchy names | ||
# 2. Templates based on class hierarchy names | ||
filenames = utils.find_templates_for_class(cls, view, paths) | ||
for filename in filenames: | ||
if filename not in template_list: | ||
template_list.append(filename) | ||
logger.debug('Found new template %s: %s (total=%d)', | ||
display_type, filename, len(template_list)) | ||
|
||
# 3. Ensure that the detailed tree template makes its way in for | ||
# all top-level screens, if no class-specific screen exists | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting choice. It's useful but has added to my confusion about template names. Why do we do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it's like:
It's all confusing, for sure. Does that touch on your confusion on this aspect or is it something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's more that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that may have been unintentional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aaah ok the comment made it seem intentional, but that's not really a proper reading of the comment. Gotcha |
||
if DETAILED_TREE_TEMPLATE not in template_list: | ||
if not self._nested or self.suggest_composite_screen(cls): | ||
template_list.append(DETAILED_TREE_TEMPLATE) | ||
|
||
# 4. Default templates | ||
template_list.extend( | ||
[templ for templ in DEFAULT_TEMPLATES[display_type] | ||
|
@@ -1476,31 +1474,10 @@ def suggest_composite_screen(cls, device_cls): | |
composite : bool | ||
If True, favor the composite screen. | ||
""" | ||
num_devices = 0 | ||
num_signals = 0 | ||
for attr, component in utils._get_top_level_components(device_cls): | ||
num_devices += issubclass(component.cls, ophyd.Device) | ||
num_signals += issubclass(component.cls, ophyd.Signal) | ||
|
||
specific_screens = cls._get_specific_screens(device_cls) | ||
if (len(specific_screens) or | ||
(num_devices <= cls.device_count_threshold and | ||
num_signals >= cls.signal_count_threshold)): | ||
# 1. There's a custom screen - we probably should use them | ||
# 2. There aren't many devices, so the composite display isn't | ||
# useful | ||
# 3. There are many signals, which should be broken up somehow | ||
composite = False | ||
else: | ||
# 1. No custom screen, or | ||
# 2. Many devices or a relatively small number of signals | ||
composite = True | ||
|
||
logger.debug( | ||
'%s screens=%s num_signals=%d num_devices=%d -> composite=%s', | ||
device_cls, specific_screens, num_signals, num_devices, composite | ||
) | ||
return composite | ||
for _, component in utils._get_top_level_components(device_cls): | ||
if issubclass(component.cls, ophyd.Device): | ||
return True | ||
return False | ||
|
||
@classmethod | ||
def from_device(cls, device, template=None, macros=None, **kwargs): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the biggest pre-rel I've ever seen