Skip to content

Conversation

@abrahamwolk
Copy link
Collaborator

@abrahamwolk abrahamwolk commented Apr 24, 2024

This PR implements point 3 described in the discussion #2972, i.e. 3. Modified and extended functionality to paste several PV names (with an optional display name) into the Data Browser. (The PR #2998 implements points 1 and 2 of that discussion.)

In particular, this PR adds a new context-menu item "Add PV(s) from the Clipboard" to the Data Browser that allows one to paste a collection of newline-separated PV names, optionally together with associated Display Names, into the Data Browser:
contextMenu

The collection of pasted PV names can either be added to one axis only that they share, or they can be given individual axes; the following is a screenshot of the dialog that is opened by clicking on "Add PV(s) from the Clipboard":
screenshot

EDIT (30 July 2024): The PR has now been updated to implement the proposal described in #3057 (comment).

The the original parsing mechanism (that existed before this PR) is now used when parsing PV names, 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).

…ction DroppedPVNameParser.parseDroppedPVs().
…e Messages.Name (which now has no references).
… PV" window, and rename "layout" -> "gridPane".
…me", and do not display the UI-elements for entering "Display Name.
@georgweiss
Copy link
Collaborator

General comment: this cannot be fully tested unless #2998 is merged first.

In DroppedPVNameParser, should there be another case on top of \r\n, i.e. \n, or even System.lineSeparator()?

AddPVsFromTheClipboardMenuItem has hard coded message strings.

@abrahamwolk
Copy link
Collaborator Author

abrahamwolk commented May 8, 2024

After a more careful reading of the code, I think it already handles both the case \r\n and \n: if either of these characters appears, the parser stops parsing the display name.

I have now added labels for strings in AddPVsFromTheClipboardMenuItem to messages.properties.

The PR is ready for review again.

…n DroppedPVNameParser.parseDroppedPVs(). Instead, interpret names that start and end with single quotes as names for the preceding PV name.
…e. This allows for spaces and other separators inside single quotes.
…oard when pasting PVs into the Data Browser.
@abrahamwolk
Copy link
Collaborator Author

abrahamwolk commented Jul 30, 2024

The PR has now been updated to implement the proposal described in #3057 (comment).

The the original parsing mechanism (that existed before this PR) is now used when parsing PV names, 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).

@shroffk
Copy link
Member

shroffk commented Aug 29, 2024

I am adding this to the discussion topics for Sept 16th

…gle quote. This allows for spaces and other separators inside single quotes."

This reverts commit 07d4fcf.
…e clipboard by surrounding them with sinqle-quote characters. If a Display Name is not entered, use the default for the Display Name.
…rowser3/assign_pvs_from_clipboard_to_the_same_axis_by_default'.
…ult' to 'databrowser_preferences.properties'.
@abrahamwolk
Copy link
Collaborator Author

abrahamwolk commented Nov 15, 2024

@shroffk I have now updated the pull request to align with our discussion on 2024-10-08. In particular, I have:

  1. Removed the possibility to represent display names on the clipboard. Only PV names can be represented on the clipboard, and they are parsed using the existing mechanism of the drag'n'drop functionality.
  2. I have added the configuration option org.csstudio.trends.databrowser3/assign_pvs_from_clipboard_to_the_same_axis_by_default. This option configures whether the "add all PVs to the same axis" is selected by default or not. If the option is not set, it defaults to false.
  3. The entering of Display Names works together with the functionality implemented in Define DisplayName in databrower with PV descriptions if this information is available. #3135 and Make DESC field use in databrowser non-default #3190: If the Display Name is left empty, the DESC field is used if this functionality is enabled and PV Access is used to connect to PVs.

Copy link
Collaborator

@georgweiss georgweiss left a comment

Choose a reason for hiding this comment

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

I realize this is more work, but the AddPVDialog would in my view merit layout management in fxml.

@abrahamwolk
Copy link
Collaborator Author

I realize this is more work, but the AddPVDialog would in my view merit layout management in fxml.

While I generally think that layout using FXML is preferable, this pull request modifies the existing AddPVDialog which is implemented by extending Dialog<Boolean>. I think it is outside the scope of this pull request change the implementation to use FXML instead.

@georgweiss
Copy link
Collaborator

I suggested FXML as a means of "continuous improvement" since this ticket is changing the layout and thus offers an opportunity to migrate away from layout managed in code.

@abrahamwolk
Copy link
Collaborator Author

I suggested FXML as a means of "continuous improvement" since this ticket is changing the layout and thus offers an opportunity to migrate away from layout managed in code.

I agree that it would be an improvement. However, I think there's more value per effort to be had in working on other tasks at the moment.

@shroffk
Copy link
Member

shroffk commented Nov 20, 2024

I think we should aim to close this PR soon and push all improvements to future PR's

I believe this is ready to merge, any objections?

@abrahamwolk abrahamwolk merged commit 30cc0f5 into master Nov 21, 2024
2 checks passed
@abrahamwolk abrahamwolk deleted the CSSTUDIO-2072 branch November 21, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants