Skip to content

Add long names #627

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.

Sign up for GitHub

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

Merged
ZLLentz merged 8 commits into from
Feb 6, 2025
Merged

Add long names #627

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5b94a78
ENH: Add support for "long_names" on signal panels
aberges-SLAC Feb 6, 2025
4a9c69d
Merge branch 'pcdshub:master' into add_long_names
aberges-SLAC Feb 6, 2025
03d69aa
ENH: Repackage hacky long_name checks into a func
aberges-SLAC Feb 6, 2025
c17aa28
DOC: Add release notes
aberges-SLAC Feb 6, 2025
3f0df86
Merge branch 'add_long_names' of github.com:aberges-SLAC/typhos into …
aberges-SLAC Feb 6, 2025
a47a751
MNT: Fix white space
aberges-SLAC Feb 6, 2025
63832eb
ENH: fix allocation in _get_long_name
aberges-SLAC Feb 6, 2025
9024491
MNT: Be consistent with annotations
aberges-SLAC Feb 6, 2025
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
61 changes: 51 additions & 10 deletions typhos/panel.py
View file
Open in desktop

@ZLLentz ZLLentz Feb 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Missing here is the pre-release notes docs step, same as how we've done it in pcdsdevices with pre-release-notes.sh

@aberges-SLAC aberges-SLAC Feb 6, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah derp forgot to commit that, let me fix that after above changes

Original file line number Diff line number Diff line change
Expand Up @@ -235,16 +235,38 @@ def _got_signal_widget_info(self, obj, info):
for _, sig_info in signal_pairs):
self.loading_complete.emit([name for name, _ in signal_pairs])

def _create_row_label(self, attr, dotted_name, tooltip):
"""Create a row label (i.e., the one used to display the name)."""
label_text = self.label_text_from_attribute(attr, dotted_name)
def _create_row_label(self, attr, dotted_name, tooltip, long_name=None):
"""
Create a row label (i.e., the one used to display the name).
If an alternative (human-readable) long name is defined, use that instead
and add the dotted_name to the tooltip for hutch python ease of use.

Parameters
-----------
attr: any
Name of the signal
dotted_name: any
Full dotted name of the signal
tooltip: str
Doc string to add to signal
long_name: str, optional
Long form (human readable) name to use for the signal row label text.
"""
if long_name:
label_text = long_name
else:
label_text = self.label_text_from_attribute(attr, dotted_name)
label = SignalPanelRowLabel(label_text)
label.setObjectName(dotted_name)
if tooltip is not None:
label.setToolTip(tooltip)
if long_name:
_tooltip = dotted_name + '<br>' + round(1.75*len(dotted_name))*'-' + '<br>' + tooltip
else:
_tooltip = tooltip
label.setToolTip(_tooltip)
return label

def add_signal(self, signal, name=None, *, tooltip=None):
def add_signal(self, signal, name=None, long_name=None, *, tooltip=None):
"""
Add a signal to the panel.

Expand Down Expand Up @@ -276,8 +298,8 @@ def add_signal(self, signal, name=None, *, tooltip=None):
return

logger.debug("Adding signal %s (%s)", signal.name, name)

label = self._create_row_label(name, name, tooltip)
label = self._create_row_label(attr=name, dotted_name=name, long_name=long_name, tooltip=tooltip)
loading = utils.TyphosLoading(
timeout_message='Connection timed out.'
)
Expand Down Expand Up @@ -332,8 +354,18 @@ def _add_component(self, device, attr, dotted_name, component):

logger.debug("Adding component %s", dotted_name)

# Hacky workaround until Ophyd.Component.long_name PR comes through
long_name = None
try:
if hasattr(getattr(device, attr), 'long_name'):
long_name = getattr(device, attr).long_name
except AttributeError:
# Then maybe we have a nested component and can't touch the signal
if hasattr(getattr(device, dotted_name), 'long_name'):
long_name = getattr(device, dotted_name).long_name

@ZLLentz ZLLentz Feb 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this bit is a hacky workaround, and we needed it twice, it should be pulled out into its own function. After pulling out into its own function this is a reasonable thing to write a short unit test for to make sure we've covered all the cases we expect.

I'm wondering if there's some way to revise this to make it more readable but I don't have any immediate suggestions. Some of the hasattr calls probably aren't needed I guess.

@aberges-SLAC aberges-SLAC Feb 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, it's probably worth some:

def _get_long_name(self, device, attr, dotted_name) -> str:
"""
Check if the signal has implemented a long_name and return it. Will check both the device and the component signal for the long_name if it exists.
"""
    long_name = None
    try:
            if hasattr(getattr(device, attr), 'long_name'):
                long_name = getattr(device, attr).long_name
    except AttributeError:
            # Then maybe we have a nested component and can't touch the signal
            if hasattr(getattr(device, dotted_name), 'long_name'):
                long_name = getattr(device, dotted_name).long_name
    return long_name

I think I ended up needing both hasattr due to it getting squirrelly when you build a screen with multiple component devices where the long name is actually in device.component.attr.long_name instead of device.attr.long_name — and the fact that long_name is not in the Ophyd object by default yet. Once they implement it we can probably just replace it with some try block around if getattr(device, attr).long_name instead.

label = self._create_row_label(
attr, dotted_name, tooltip=component.doc or '')
attr=attr, dotted_name=dotted_name, long_name=long_name,
tooltip=component.doc or '')
row = self.add_row(label, None) # utils.TyphosLoading())
self.signal_name_to_info[dotted_name] = dict(
row=row,
Expand Down Expand Up @@ -662,8 +694,17 @@ def _maybe_add_signal(self, device, attr, dotted_name, component):
logger.warning('Failed to get signal %r from device %s: %s',
dotted_name, device.name, ex, exc_info=True)
return

return self.add_signal(signal, name=attr, tooltip=component.doc)
# Hacky workaround until Ophyd.Component.long_name PR comes through
long_name = None
try:
if hasattr(getattr(device, attr), 'long_name'):
long_name = getattr(device, attr).long_name
except AttributeError as ex:

@ZLLentz ZLLentz Feb 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small note: the pre-commit CI job is flagging ex as an unused variable and a couple instances of trailing whitespace

# Then we must have a component signal, so try the dotted_name
if hasattr(getattr(device, dotted_name), 'long_name'):
long_name = getattr(device, dotted_name).long_name
return self.add_signal(signal=signal, name=attr, long_name=long_name,
tooltip=component.doc)

return self._add_component(device, attr, dotted_name, component)

Expand Down