Fix bug: qt terminal launches unexpectedly when saving plot (#362) #472
Conversation
| void gnuplot::lazy_init_pipe() { | ||
| int perr; | ||
| if constexpr (windows_should_persist_by_default) { | ||
| perr = pipe_.open("gnuplot --persist"); |
There was a problem hiding this comment.
So it's not only opening it later but also calling it every time we show the figure?
There was a problem hiding this comment.
Sorry, maybe I should check whether the pipe is already open before opening it again, to avoid reopening it when calling show() or save() multiple times.
There was a problem hiding this comment.
Yes. True. If the point of the PR is only to make it lazy, that makes more sense.
But I was checking because I'm not so familiar with the changes.
There was a problem hiding this comment.
There's one point, this change will revise the render haviour. For example, you can run the examples/data_distribution/hisrogram14, the original code will draw step by step. But the changed code will render once during the whole time.
There was a problem hiding this comment.
So this means it'll only render the first time and never update it?
There was a problem hiding this comment.
Sorry. That's what I understood so far, please correct me if I'm wrong:
- Before this PR, draw() updates/renders the plot (especially important in "quiet" mode) and show() would do the same and also block
- After this PR, draw() wouldn't do anything until you call show() and show() would have the same expected behavior
Is my understanding correct?
I'm sorry to be insisting. I don't want to block a PR because I don't understand it. But also can't merge a PR I don't understand clearly.
did you take into account the need to display one subplot at a time?
Yes. That should be possible. To only one subplot at a time, but one thing at a time. That's why the distinction between 'draw' and 'show' exists. That's not even my design. It comes from all other analogous libraries.
There was a problem hiding this comment.
Yes, your understanding is correct.
But if this use case (displaying one subplot at a time) was indeed taken into account, then this PR might need to be reconsidered.
This issue seems quite tricky to fix and may require further discussion to find a proper solution.
There was a problem hiding this comment.
if I understant the plot logic correctly, I would like to recommend to remove the render code when adding child, and it should occus just in show or draw.
There was a problem hiding this comment.
then this PR might need to be reconsidered
Exactly. I think so. draw() should update the plot pretty much like in matplotlib.
There was a problem hiding this comment.
OK, I will close the PR, hope you can fix this in the future.
source/matplot/core/figure_type.cpp
Outdated
| void figure_type::show() { backend_->show(this); } | ||
| void figure_type::show() { | ||
| // open the gnu pipe when show method is called | ||
| backend_->lazy_init_pipe(); |
There was a problem hiding this comment.
If this is going to be called every time we call "show", it's it easier to conceptually make it part of the gnuplot backend only and call it at the beginning of the show function?
There was a problem hiding this comment.
Yep, but there isn’t an API implementation for showing in gnuplot.cpp. We just use flushcommand() to flush the buffer and display the plot. However, you’re right -- it might be more reasonable to initialize the pipe in backend_interface instead of figure_type.
There was a problem hiding this comment.
Sorry. I'm a little confused. The very next line does backend_->show(this);. Isn't this calling "an API implementation for showing in gnuplot.cpp"? Do you mean the gnuplot backend doesn't override show()?
There was a problem hiding this comment.
That's the truth what I want to say. But, it seems that the backend_interface::show doesn't need to be overrided. It's unnecessary.
There was a problem hiding this comment.
OK. So if I understand you correctly, matplot::backend::gnuplot doesn't define matplot::backend::gnuplot::show(), so that means matplot::backend::gnuplot::show() is equivalent to matplot::backend::backend_interface::show().
If that's correct, we could override and define matplot::backend::gnuplot::show() so that 1) it calls lazy_init_pipe() and only then 2) calls matplot::backend::backend_interface::show(). The advantage of this would be that the gnuplot fix would be limited to the scope of the gnuplot backend, and figure_type would make reference to a property that doesn't belong to all backends.
There was a problem hiding this comment.
Yes, that's a nice strategy.
|
Actually, I think the best way to fix the problem is that we shouldn't run the command while adding the child, instead just run the commands once in the show or save. |
… backend_interface::show()
I agree. This is what called "quiet mode" or something in this implementation. In retrospect, this should be the only mode possible. But it's too late to change that unless there's a matplot++ v2. |
2 participants
As described in #362, when the user only wants to save the plot, the terminal is still set to qt and the graphic view is invoked.
This behavior should not occur when the plot is only exported without interactive display.
Root cause
Fix