Skip to content

Conversation

@skyfrench
Copy link
Contributor

Implemented support for multiple traces in WaveformView

  • Possible to select more than one PV for display
  • Traces are now drawn as a line with no points

Modification made to handle events not coming in at the same time for different plotted PVs:

  • the bottom slider remains a linear slider which scrolls through the samples available in the first plotted item (i.e. the single waveform display behaves as it does currently, and when there are multiple traces the first plotted behaves in the same was as it would were it plotted alone)
  • the samples plotted for the other item(s) are then the preceding waveform(s) from the timestamp of the waveform sample in the first item

cc @willrogers

@willrogers willrogers requested review from berryma4 and kasemir August 13, 2018 08:58
Copy link
Contributor

@kasemir kasemir left a comment

Choose a reason for hiding this comment

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

As for MultiSelectCombo, there is already org.csstudio.ui.util.widgets.MultipleSelectionCombo and MultipleSelectionComboDemo, can you use that?

@kasemir
Copy link
Contributor

kasemir commented Aug 13, 2018

Pull request #2473 was submitted just before this one, and since it has been merged, there are merge conflicts on this one. Please check into using the existing MultipleSelectionCombo class, and the merge conflict. Otherwise I think this is a great idea, thanks!

Copy link
Contributor

@kasemir kasemir left a comment

Choose a reason for hiding this comment

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

Not sure about this code:

public void itemRemoved(final ModelItem item)
{
    for (ModelItem model_item : model_items) 
        if (item == model_item) 
            model_items.remove(model_item);

It modifies a list that's being iterated.

Safer and actually simpler:

  model_items.remove(item);

@willrogers
Copy link
Contributor

I resolved the merge conflict and did the simpler list removal. The MultipleSelectionCombo sounds useful.

@kasemir
Copy link
Contributor

kasemir commented Aug 13, 2018

Yes, MultipleSelectionCombo<ModelItem> directly holds the model items, removes the need for most of getModelItems().

kasemir added a commit to kasemir/org.csstudio.display.builder that referenced this pull request Aug 13, 2018
kasemir added a commit to ControlSystemStudio/phoebus that referenced this pull request Aug 13, 2018
@berryma4 berryma4 closed this Aug 13, 2018
@berryma4 berryma4 reopened this Aug 13, 2018
Copy link
Contributor

@kasemir kasemir left a comment

Choose a reason for hiding this comment

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

Perfect, except that it still includes the unused org.csstudio.trends.databrowser2.multiselectcombo, having switched to the existing org.csstudio.ui.util.widgets.MultipleSelectionCombo

Copy link
Contributor

@kasemir kasemir left a comment

Choose a reason for hiding this comment

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

Thanks!

@willrogers
Copy link
Contributor

I do find the org.csstudio.ui.util.widgets.MultipleSelectionCombo a little tricky to use. Could we edit it to use the checkboxes like in Sky's version?

@kasemir
Copy link
Contributor

kasemir commented Aug 14, 2018

You're right. With check boxes, you just tick or un-tick the boxes. With the multiselectioncombo, you need to Shift-or-Ctrl-Click until you get carpal tunnel syndrome.
I'm in favor of updating the multiselectioncombo to use combo boxes.
The phoebus version already uses combo boxes, which is probably another indication of their superiority.

@skyfrench
Copy link
Contributor Author

I have committed a modification to the MultipleSelectionCombo to use check boxes. Hopefully it's an improvement on what was there previously?

@kasemir
Copy link
Contributor

kasemir commented Aug 15, 2018

Does the MultipleSelectionComboDemo still work?

@skyfrench
Copy link
Contributor Author

Does the MultipleSelectionComboDemo still work?
Yes, I believe it does (comparing behaviour before and after).

@willrogers
Copy link
Contributor

That looks nicer. Two comments:

  • I don't seem able to get rid of the dropdown by clicking elsewhere (your previous version worked)
  • Maybe it should say "- Select items -" or something when nothing is selected

@willrogers
Copy link
Contributor

I think this is now ready. You could squash the commits if you like.

@berryma4 berryma4 merged commit c211acd into ControlSystemStudio:master Aug 16, 2018
@berryma4 berryma4 added this to the 4.6.0 - testing (master) milestone Aug 16, 2018
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