Detached legend improved interactivity#1655
Conversation
- Opening a detached legend now hides the initial legend in the `FormsPlot` resulting in a true *detaching* of the `Legend` (fix ScottPlot#1651). - This requires the addition of a `isDetached` flag to `Legend` defaulting to `false` - This initial visibility is stored in a new `FormsPlotLegendViewer` property `InitialLegendVisibility` - Upon closing of the detached Legend, the `InitialLegendVisibility` is used to re-display the original `Legend` if relevant - A plottable can now be highlighted, deleted or re-styled by opening a right-click menu on a `LegendItem`. This implementation is very basic though. - Made some refactors to `GetLegendItemUnderMouse` to accomodate different location sources.
|
@swharden This is a very basic implementation of what is quite useful is Matlab :
|
swharden
left a comment
swharden
left a comment
There was a problem hiding this comment.
Hi BambOoxX, thanks for this PR!
My biggest concern is just the switching/casting that has to be done because the interfaces aren't there to do it a better way.
I'd probably recommend using separate PRs for implementing those interfaces and extending the menu to support customization vs. a PR which specifically resolves #1651
- Opening a detached legend now hides the initial legend in the `FormsPlot` resulting in a true *detaching* of the `Legend` (fix ScottPlot#1651). - This requires the addition of a `isDetached` flag to `Legend` defaulting to `false` - This initial visibility is stored in a new `FormsPlotLegendViewer` property `InitialLegendVisibility` - Upon closing of the detached Legend, the `InitialLegendVisibility` is used to re-display the original `Legend` if relevant - A plottable can now be highlighted, deleted or re-styled by opening a right-click menu on a `LegendItem`. This implementation is very basic though. - Made some refactors to `GetLegendItemUnderMouse` to accomodate different location sources.
…b.com/BambOoxX/ScottPlot into DetachedLegend-Improved-Interactivity
…`IHasLine`, `IHasMarker`) - Removed casting to `ScatterPlot` or `SignalPlot`, but some castings to the interfaces remain, not sure how to avoid them - Removed useless `HighlightedPlottabes` and `ClickedIndex` - Highlight logic now hidden in `Render` of each specific `IIsHighlightable` - Added `GetHighlightedItems` to `Legend` to retrieve the list of `LegendItem`s associated with highlightable plottables
|
Here is a preview of the extended functionalities test3.mp4 |
|
@swharden I tested this branch this morning, and I believe everything works as intended, do you think it is worth merging as such or do you see other improvements ? |
I haven't looked at this one in detail yet. My commits so far have just been to ensure this branch keeps building successfully, and my goal is to get the lower-level Marker refactoring done before moving to this higher-level GUI-related PR, but hopefully I'll finish all these over today and tomorrow 👍 |
|
@swharden Allright, just tell me if you need help on this. |
|
Hi @BambOoxX, thanks for the follow-up! The short answer is that I haven't had a long enough time block for me to sit down and review this PR yet. I've had smaller bits of free time I've devoted to quicker/prioritized tasks in this and other repos (hence my recent GitHub activity, but inaction on this specific PR), but I still haven't gotten the time I need to review this yet. I don't want to short-change this work by picking it up and putting it down a bunch of times - it would be best if I can review it all in one sitting. Complicating things is that there's been a work-related issue that has been greatly encroaching on my free time to do open-source work. I've been at the laboratory late every day and am working through this weekend there to try to resolve it, but it's likely to persist for a few more days, and after that I'll be a lot more free to work on this PR. Thanks again for your work here and recent efforts here and on #1689, #1690, and #1692! All of these are fantastic improvements 🚀 |
Oh, and technically merging is blocked because the build is failing due to the autoformatter not being run 🙃 dotnet tool update -g dotnet-formatdotnet-format |
|
Haha, the famous autoformatter. I believe I can solve that easily 😅. I understand your position, it was just a gentle poke to see if I was going in the right direction or not. I have a feeling this kind of interactive behavior can attract new users to this library. I'll let you review this whenever you can, no big deal. If you see something I can improve in the meantime, don't hesitate to indicate it. Finally, thanks for the kind words. This is really the first C# project I'm actively contributing to, so it's really nice to be able to learn whil being helpful. Cheers! |
Favored switch expressions over switch statements and removed XML docs where function names were descriptive enough to convey functionality and purpose. Code comments were retained where code was exceptionally confusing.
|
Hi @BambOoxX, thanks for this PR! I was able to review it and I'll merge it in shortly. I'll share some of my thoughts here so you can learn more about my line of thinking (not as a criticism of your work), and to clarify why I was a bit slow to get to this PR and why I'm often hesitant to extend GUI-related features
Yes... but it's also really hard to maintain as a library maintainer 😅 GUI code like this is a bit finnicky (and difficult to test), and if the Legend, LineStyle, or Marker code, etc. ever change in the future, it may also require re-testing (and possibly new code edits) in this module. I'm not super concerned about this particular PR because the GUI features are tucked away under an overridable right-click menu, but in general I try to avoid adding GUI features to this library. Also there's a goal of achieving consistent behavior across the WinForms control, WPF control, Avalonia control, and Eto control, so technically anything we add in one means multiplying the effort across all 4 of these. Let's not worry about it for this feature, but FYI that's another reason I hesitate to add new GUI functionality. It's pretty easy for users to write GUI code to satisfy their use cases, so I try to lean wherever possible toward making it easy for users to customize their own GUI rather than predict all their use cases and support them from this side of the library.
Wow! This is a complicated project, and this PR touches some advanced topics and obscure WinForms behavior, and you're doing fantastic! 🚀 Are you a student? If you're interested in getting plugged into some more circles feel free to email me [email protected] and I can share a few resources with you that I found helpful. Thanks again for your work on this PR! |
Haha, no I'm not a student, but most of my work before last year was focused on low-level computational stuff. I just discovered the upper GUI layer world. What dou you mean by plugged into some circles ? |
Ah okay, that makes sense. My offer sounded more cryptic than was intended! I was just going to list some some resources I found useful for getting plugged-in a little deeper to the C#/programming community. If you were looking for more projects to contribute to or wanted to get some perspectives related to entering the job market for the first time (useful information for students) they might be helpful. I'll list some info here in case someone else following is interested:
|
|
@swharden Thanks for the helpful links, I'll check them when I get some spare time. Though I'm not a student anymore, I'm learning new stuff every day, also by trying to contribute ! I'm not sure I can contribute, but whenever I get the chance I try to participate to the extent of my abilities, a way to return the favor to the persons initiating such projects I use like yourself ! |
2 participants
Purpose:
Note :
isHighlightedproperty of aLegendItemwould probably be better, but I wanted to propose this PR as such before changing that.