Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] [Feature] Plotly extension #693
base: develop
Are you sure you want to change the base?
[WIP] [Feature] Plotly extension #693
Changes from 4 commits
1e04263
48ea5f1
8a729d0
0979b75
e533114
a7ce2af
99c8e84
8560b71
2230068
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so the stream has to live outside this class. It is like in
svg_mapper
so I understand that you replicated the existing design. This is error-prone since when the object outside is destroyed then this is a dangling reference.So I'm wondering about a different design, i.e. owning the stream instead of the reference. E.g.:
What do you think? Do you have different ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that seems better. I was thinking why the above design was implemented in
svg_mapper
. The only reason I could find is if some external changes are required by the user that isn't supported bysvg_mapper
. This isn't required byplotly_plotter
at all then. Can you tell me that actual reason for this design?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know I have only a few ideas though. @barendgehrels do you remember why exactly does the
svg_mapper
store the reference to the stream instead of owning it?Back then perfect forwarding was impossible. The code in my previous comment requires C++11 but Boost.Geometry right now requires C++03 (this will change soon). And various streams can have various interfaces, e.g. no
open()
member function so there must be a way of passing parameters into its constructor.In some cases the stream objects are already created, e.g.
std::cout
,std::cerr
, etc. AFAIU it is possible to account for that by creating astd::ostream
from another stream'sstreambuf
:But this would be more confusing than simply calling:
With non-owning relationship the same stream can be shared by several mappers/plotters. I don't know if that makes sense but it is possible nevertheless.
The user can keep and e.g. close and reopen
ofstream
buffer instead of creating thesvg_mapper
/plotly_plotter
object each time.Right, though the owned stream could be exposed with some getter function if needed.
Btw, I'm not saying that you should start implementing what I have proposed above. I'm only brainstorming. E.g. now after listing the potential reasons above I'm not sure whether or not my proposal was good.
On the other hand maybe passing a reference as a template parameter could work with the design I proposed. So the user would be able to do both things, store a stream object or a reference. Unless there are some issues I'm not taking into account.
And last but not least I'm not sure if the design should be different than
svg_mapper
because the users may expect that the tools work the same and either expect that a reference is stored inplotly_plotter
or that an object is stored insvg_mapper
. We should probably be consistent one way or another.@mloskot do you have some thoughts about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I have put it inside the class itself as I think it's better to make it safe than just copy the existing design as:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I can't remember exactly, I think @awulkiew has figured it out right.
It was to allow user to plug in a stream object of their choice, and with simple interface.