Completed implementing a secondary Y axis - Fix for #2026 #2131
Conversation
|
This looks nifty. 😄 |
|
Cool! |
|
Thanks for this contribution. Very useful. I've been testing it, and it works perfectly. I've made also a quick review of the code and what should be improved is the loading of previous project format. With the current state of the implementation, the compatibility with the older format is broken. Would you mind making that improvement? |
|
Thanks @mgrojo - Yea, looks like I didn't address that. Sure, I'll get it fixed. |
src/TableBrowser.h
Outdated
| stream >> object.plotXAxis; | ||
| stream >> object.plotYAxes; | ||
| stream >> object.plotY1Axes; | ||
| stream >> object.plotY2Axes; |
There was a problem hiding this comment.
This is the old stream format, which we abandoned in 3.11, but the loading is still kept here for backwards compatibility, so it shouldn't be affected by this change. I suppose it would work simply by using object.plotY1Axes where object.plotXAxis was used.
If it were possible, it would be cool that you could also verify that it can load a project file saved with 3.10.
There was a problem hiding this comment.
I'll make the modification. However, it appears like the backwards compatibility for stream is broken in master branching point itself. I'm able to load the project file - but the settings and preferences are lost.
There was a problem hiding this comment.
Uh oh, that doesn't sound good. That'll need fixing regardless (not sure whether in this PR, or separately). 😉
There was a problem hiding this comment.
Probably is due to differences in the type dumped in the stream. It was one of the problems with this binary format, which could be inadvertently modified. I'd like to take a look, but maybe it has a difficult solution. We might end up assuming that part of the compatibility of the 3.10 format will be lost in this version.
| } else { | ||
| columnitem->setCheckState(PlotColumnY2, Qt::Unchecked); | ||
| } | ||
|
|
There was a problem hiding this comment.
Haven't thought much about it, but maybe there is an easy way to avoid this code duplications by having some function calls, where the appropriate Y axis is passed. It would easy future maintenance, but I don't consider it an impediment for merging. In fact, it could risk a greater risk for introducing some bug if we want to release this with the imminent 3.12 release. I'd like to know what @MKleusberg thinks about it.
There was a problem hiding this comment.
Yep, there is a lot of code duplication. I had earlier decided against optimizing this because
- I wanted to keep the style same
- Since the secondary y axis is completely independent entity, keeping it totally separate might help in the future for selective modifications.
However, I'll push a commit with the code optimizations.
There was a problem hiding this comment.
@mgrojo I'm fine either way but tend towards avoiding the duplication even when including it in the 3.12 release because duplicated code could contain hard-to-spot copy & paste errors too.
There was a problem hiding this comment.
I like the code refactoring. It's more maintainable and easier to read now. I've also tested it and found no issue. I consider it ready for merging but will wait some time so others have time to review. I think it could be merged and released in 3.12.
Thanks @anandsanto!
There was a problem hiding this comment.
I'm a huge fan of this project and what you guys do here - particularly because I have been using DB4S extensively for a while now. I'm happy that I could make a minimal contribution to this project :)
|
@anandsanto After some time without receiving additional feedback, I felt that your contribution was worthy of entering our next release. Thanks again for it. |
|
Sorry for not responding to this earlier - I've been travelling the last couple of days. But after a just quick look I absolutely agree with merging your PR before the release 👍 If you ever feel like it you're super welcome to open more PRs, @anandsanto 😄 |
Secondary Y axis
When analysing datasets with multiple correlated columns, it would be very comfortable to view the graphical relationship of two parameters without having to manually scale the parameters. A secondary y axis helps achieve this and makes it very easy to comprehend relationship between two columns in the dataset.