Update the widget info to allow simpler selection of PV's #3057

shroffk · 2024-06-20T20:44:26Z

Based on some of use case discussion we had during the codeathon. I have updated the WidgetInfo in the display builder to simplify the process of selecting a set of PV's from an screen or a widget

kasemir

I can't complain, looks good

abrahamwolk

Apart from the commented-out line of code, I think the implementation looks good!

I would like to ask if the changes introduced by this PR are intended to complement the new column under the "PVs" tab in the Widget Info window that is introduced in #2998, or if the changes introduced by this PR are intended as an alternative solution to the same problem?

abrahamwolk · 2024-06-24T08:24:20Z

...avafx/src/main/java/org/csstudio/display/builder/representation/javafx/WidgetInfoDialog.java

+                        // reset the selection
+                        SelectionService.getInstance().setSelection(table, old);
+                    } catch (Exception ex) {
+                        //logger.log(Level.WARNING, "Failed to execute action " + entry.getName(), ex);


Commented-out code.

shroffk · 2024-06-25T15:35:56Z

I am hoping this is an alternate solutions which can address some of the points from the #2998

We might have to add some more features, but this was a simple change which would address some of the use cases which lead to the above PR

abrahamwolk · 2024-06-26T08:45:50Z

@shroffk Thanks for clarifying! I will bring forward and discuss this alternative solution with the stakeholders here at ESS.

abrahamwolk · 2024-07-03T08:07:32Z

@shroffk, @kasemir : I have now discussed the solution implemented in this PR with the stakeholders here at ESS, and the conclusion is that this solution works for them for copying PV names in the Widget Info window.

1. Functionality to copy & append PV names to the clipboard
Regarding the other functionality implemented in #2998 (i.e., point 1. in the description of that PR): at the codeathon there seemed to be consensus about implementing the "Append PV Name to the Clipboard" functionality directly into the context menu that appears when right-clicking on a widget in an OPI. About the functionality to copy and append PV names together with descriptions I think you suggested that this functionality could be implemented as a plug-in.

Do you think the following solution is good?

I change CSSTUDIO-2071 & CSSTUDIO-2081: Add options to copy & append PV names (with descriptions) to Display Runtime & to WidgetInfoDialog #2998 to only add the "Append PV Name to the Clipboard" to the context menu of widgets in OPIs.
The functionality to copy and append PV names together with descriptions is implemented as an ESS-specific plug-in.

I am not familiar with the plug-in architecture; do you happen to know which interface needs to be implemented to realize point 2?

2. Functionality to paste PV names into the Data Browser
Regarding the functionality implemented in #3000: I have also discussed the drag'n'drop functionality with the stakeholders here at ESS, and the drag'n'drop functionality is insufficient for their use-case and they would like to see something similar to the functionality implemented in #3000, i.e., the ability to paste a list of PV names from the context menu into the Data Browser. Therefore, I would like to ask what you think about the implementation in #3000?

kasemir · 2024-07-23T12:43:40Z

copy & append

I'd prefer to stay with just the "copy". Keeps the context menu smaller. If you add "append", there'll always be somebody who prefers a different concatenation.

paste PV names into the data browser

Sure, there could be a "Paste PV" context menu entry which ends up calling what you now get when you drag/drop something like "One, Two Three", making a simple attempt to split the text into PV names:

abrahamwolk · 2024-07-23T12:54:47Z

Regarding 1 (copy): For us, it's OK either with or without "append", since we can implement the other options as site-specific plugins.

Regarding 2 (pasting into the Data Browser): We would like to have the ability to paste also Display Names associated with the PVs. Unfortunately, there is no plug-in architecture for implementing such a functionality in the Data Browser. Would you be OK with having support for optional Display Names built into the pasting functionality? I think it would make sense from a UI perspective to be able to enter/edit Display Names when pasting PVs into the Data Browser.

kasemir · 2024-07-23T13:03:13Z

How do you want to parse PV names and display names from the pasted text?

abrahamwolk · 2024-07-23T13:53:19Z

They would be delimited using a comma character.

My understanding is that a PV name is either:

A record name [1]
A record name and a field name, separated by a . (a field name seems to have to be a valid C-identifier)
Either of the above with a filter [2]

I think it should be possible to parse the PV Name in all cases (in the case of filters either fully, or by parsing opening and closing brackets and ", and it is possible some other characters need to be parsed as well) and then parse a , after that, and then finally the Display Name.

[1] https://epics.anl.gov/base/R3-16/2-docs/AppDevGuide.pdf (p. 114)
[2] https://epics.anl.gov/base/R7-0/2-docs/filters.html

kasemir · 2024-07-24T16:08:21Z

I thought comma already separates PV names.
Is Temperature, Pressure a list with two PV names, or is it PV Temperature with description "Pressure"?

abrahamwolk · 2024-07-25T08:58:37Z

That is a good point!

Then we do not currently support filters when pasting PV names into the Data Browser?

I just tried to create a widget in an OPI with a PV name that also specifies a filter, but it was unsuccessful; do we support filters in Phoebus?

kasemir · 2024-07-25T13:24:49Z

.. do not currently support filters ..

We do support them. In fact we're future-proof in a sense because we simply pass anything, as entered, as a PV name.

Run softIocPVA -d app/display/model/src/main/resources/examples/pvaccess/example.db and then show both counter01.{"dbnd":{"abs":5.0}} and counter01 in probe or data browser, and you'll see the different update rates:

Works with pva://... as well. It does work because the filtering is done on the IOC end. We simply pass that whole PV name on as given. We don't try to understand what rsrv, qsrv, or qsrv2 on the IOC, or pcaspy, p4p, pvaPy, ... in a similar looking CA or PVA server, or other protocols like mqtt://.., sim://... might have in mind when they look at a PV name.

This does mean if you have an archive engine configured to sample counter01, we'll only find archived data when you try to plot counter01. If you entered counter01.{"dbnd":{"abs":5.0}}, we'll send that as a name to the archive data server which will likely return nothing, because it doesn't match counter01.

Trying to parse filters would be tricky. You probably don't want to understand the complete filter syntax, since that would require updates whenever something new is added on the IOC end. So you simply remove anything between { and the closing '}', right? Well, there are sites which use something like BR:C001-MG:1{QDP:D}Fld-SP as PV names, the {QDP:D} is not a filter, you must not strip that out.

abrahamwolk · 2024-07-26T10:30:23Z

I see! But then the current functionality to split PV names when pasted into the Data Browser could split the filters in two? JSON can contain commas as well as spaces, I believe, and I think we split PV names on both of those in the current logic.

Trying to parse filters would be tricky. You probably don't want to understand the complete filter syntax, since that would require updates whenever something new is added on the IOC end. So you simply remove anything between { and the closing '}', right? Well, there are sites which use something like BR:C001-MG:1{QDP:D}Fld-SP as PV names, the {QDP:D} is not a filter, you must not strip that out.

Yes, care must be taken to parse correctly!

Are you sure names like BR:C001-MG:1{QDP:D}Fld-SP are in use? According to the EPICS documentation, BR:C001-MG:1{QDP:D}Fld-SP is not a valid record name [1]. It would be good to know what characters actually can appear as part of a PV name.

[1] https://epics.anl.gov/base/R3-16/2-docs/AppDevGuide.pdf (p. 114)

kasemir · 2024-07-26T14:19:48Z

the current functionality to split PV names

Yes, it would split on spaces or commata. It's not claiming to be perfect. That's in part why it offers what we think we got as PVs in the "Add PV" dialog, so end user can then copy/paste/edit the names back together.

Are you sure names like BR:C001-MG:1{QDP:D}Fld-SP are in use?

@shroffk, that's from your channel finder examples...

shroffk · 2024-07-26T15:21:18Z

Are you sure names like BR:C001-MG:1{QDP:D}Fld-SP are in use?

yes, these are very much in use and this pattern is applicable to 100,000s of PV's here

abrahamwolk · 2024-07-29T12:43:44Z

the current functionality to split PV names

Yes, it would split on spaces or commata. It's not claiming to be perfect. That's in part why it offers what we think we got as PVs in the "Add PV" dialog, so end user can then copy/paste/edit the names back together.

What do you think of the following idea?: We keep the logic to separate a string containing PV names into a list of PV names. However, if one of the PV names is a name enclosed in single quotes, then it stands for the display name of the PV that appeared before it.

E.g., the string pv-name-1 'display-name-1' pv-name-2 would be parsed as the following two items by the Data Browser:
PV Name: pv-name-1, Display name: display-name-1 and PV Name: pv-name-2, Display name: pv-name-2. (Since no display name is specified for pv-name-2, the default is used, which is to use the PV name itself for the display name.)

I think this would:

Probably not interfere with existing PV-names. (At least PV names cannot contain ' based on the documentation [1]. But it is of course possible that they are used regardless; if someone happens to know, please write!)
Not interfere more with JSON than the current functionality. (In JSON only double quotes are used for delimiting strings [2].)

[1] https://epics.anl.gov/base/R3-16/2-docs/AppDevGuide.pdf p. 114
[2] https://www.json.org/json-en.html

kasemir · 2024-07-29T17:48:43Z

pv-name-1 'display-name-1' pv-name-2 looks like the most practical approach.
In any case, it should result in the "Add PV" dialog so user can check if it makes sense, maybe tweak name, description, axes, .., then "OK" (or "Cancel")

kasemir · 2024-07-29T17:49:26Z

.. "Add PV" dialog which needs to be extended to include display name ..

abrahamwolk · 2024-07-30T07:15:12Z

.. "Add PV" dialog which needs to be extended to include display name ..

Yes, the "Add PV" dialog needs to be extended to include Display Name.

abrahamwolk · 2024-07-30T13:28:35Z

@kasemir , @shroffk : I have now updated the PR #3000 to implement the proposal I wrote here yesterday.

The the original parsing mechanism is now used, with one modification only: when encountering a single quote character ', the parser will continue to the matching single quote character ', in order to allow for spaces etc in display names; this is how double quotes " and brackets () were handled already.

PV names that start and end with single quote characters are interpreted as the display name of the preceding PV name (if it exists).

I would like to ask for a review of or comments on the PR again with these changes.

Update the widget info to allow simpler selection of PV's

89c9591

shroffk requested review from abrahamwolk, georgweiss and kasemir June 20, 2024 20:44

kasemir approved these changes Jun 20, 2024

View reviewed changes

shroffk marked this pull request as ready for review June 21, 2024 13:52

abrahamwolk reviewed Jun 24, 2024

View reviewed changes

shroffk merged commit abd2826 into master Jun 26, 2024

abrahamwolk mentioned this pull request Jul 23, 2024

CSSTUDIO-2071 & CSSTUDIO-2081: Add options to copy & append PV names (with descriptions) to Display Runtime & to WidgetInfoDialog #2998

Closed

shroffk deleted the pv_selection branch July 23, 2024 19:02

abrahamwolk mentioned this pull request Jul 30, 2024

CSSTUDIO-2072 Modified and extended functionality to paste several PV names (with an optional display name) into the Data Browser #3000

Merged

Update the widget info to allow simpler selection of PV's #3057

Update the widget info to allow simpler selection of PV's #3057

Uh oh!

Conversation

shroffk commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kasemir left a comment

Choose a reason for hiding this comment

Uh oh!

abrahamwolk left a comment

Choose a reason for hiding this comment

Uh oh!

abrahamwolk Jun 24, 2024

Choose a reason for hiding this comment

Uh oh!

shroffk commented Jun 25, 2024

Uh oh!

abrahamwolk commented Jun 26, 2024

Uh oh!

abrahamwolk commented Jul 3, 2024

Uh oh!

kasemir commented Jul 23, 2024

Uh oh!

abrahamwolk commented Jul 23, 2024

Uh oh!

kasemir commented Jul 23, 2024

Uh oh!

abrahamwolk commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kasemir commented Jul 24, 2024

Uh oh!

abrahamwolk commented Jul 25, 2024

Uh oh!

kasemir commented Jul 25, 2024

Uh oh!

abrahamwolk commented Jul 26, 2024

Uh oh!

kasemir commented Jul 26, 2024

Uh oh!

shroffk commented Jul 26, 2024

Uh oh!

abrahamwolk commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kasemir commented Jul 29, 2024

Uh oh!

kasemir commented Jul 29, 2024

Uh oh!

abrahamwolk commented Jul 30, 2024

Uh oh!

abrahamwolk commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shroffk commented Jun 20, 2024 •

edited

Loading

abrahamwolk commented Jul 23, 2024 •

edited

Loading

abrahamwolk commented Jul 29, 2024 •

edited

Loading

abrahamwolk commented Jul 30, 2024 •

edited

Loading