FIX: metadata handling for enums

ZLLentz commented 3 months ago

Description

Fix three issues related to enum/metadata handling in the SignalConnection part of the SignalPlugin and implement simplified unit tests for these cases.

Switch the order of the metadata/value setup for new listeners to ensure widgets have the correct contextual information. This is exactly the fix proposed by @canismarko in #605.
Fix an issue where string=True enum EpicsSignals would reliably break on casting in cases where the enum strings were numerically the same as other indices in the enum_strs tuple. This builds upon the fix proposed by @canismarko in #608
Fix a related (unreported) issue where the previous point could also break for string=False EpicsSignals.

Motivation and Context

closes #605 closes #608

How Has This Been Tested?

Unit tests were added. I originally tried to make a larger unit test where I instantiated real/fake EpicsSignal instances and connected them to the combobox widgets as reported in the issues, but I couldn't get this to work properly in a unit testing context. Rather than dump additional dev hours into this, I came up with simpler tests, possibly these could be better.

I will need to test with a real EPICS PV too.

Where Has This Been Documented?

I need to write some release notes

Pre-merge checklist

[x] Code works interactively
[x] Code contains descriptive docstrings, including context and API
[x] New/changed functions and methods are covered in the test suite where possible
[x] Code has been checked for threading issues (no blocking tasks in GUI thread)
[x] Test suite passes locally
[x] Test suite passes on GitHub Actions
[x] Ran docs/pre-release-notes.sh and created a pre-release documentation page
[x] Pre-release docs include context, functional descriptions, and contributors as appropriate

ZLLentz commented 3 months ago

Interactively, applying this fixed the mix-up between the enum values for putting to a ("1", "2", "5") enum signal but it actually increased the number of error messages I got at startup...

Before:

[2024-06-24 18:02:00,325] [ERROR   ] - Can not change value to '5'. Not an option in PyDMComboBox
[2024-06-24 18:02:00,326] [ERROR   ] - Can not change value to '5'. Not an option in PyDMComboBox

After:

[2024-06-24 18:03:02,868] [ERROR   ] - Can not change value to '1'. Not an option in PyDMComboBox
[2024-06-24 18:03:02,869] [ERROR   ] - Can not change value to '1'. Not an option in PyDMComboBox
[2024-06-24 18:03:02,869] [ERROR   ] - Can not change value to '1'. Not an option in PyDMComboBox

ZLLentz commented 3 months ago

Scratch that- the extra spam errors come from the signal not being fully connected when the widget comes online. Waiting for connection resolves the spam. This isn't something to overlook but it's unrelated to the problem the PR is fixing.

ZLLentz commented 3 months ago

Locally at LCLS we hadn't hit this yet for a couple reasons:

Most of our screens that use EPICS go through the PyDM EPICS plugins instead of through the Typhos signal plugins
We typically hadn't been setting string=True

ZLLentz commented 3 months ago

Interestingly, as I was testing this, I found that there was also an equivalent issue for string=False signals, but whether this caused an issue depended on the precise implementation of the widget that we're connecting to. In the case of PyDM's combobox the values are sent through as integers, so the int-native signal was unaffected and the string-native signal was broken. A widget which sent the values through as strings would have broken int-native signals in the same way.

pcdshub / typhos