Sat 07 Jan 2017 12:05:27 AM UTC, comment #15:
@pantxo: I checked in the last cset here (http://hg.savannah.gnu.org/hgweb/octave/rev/5dca1727ce94).
I made a few changes. It appears that _go_draw_axes_ in graphics.cc is completely unused, so I deleted it. It also appears that axes::properties::set_defaults is never called without mode == "reset". I put in an assert statement to guarantee that. If there are no complaints then in 6 months we can delete the code paths that are testing for it.
|
Fri 06 Jan 2017 04:30:59 PM UTC, comment #14:
Yes, I was going to write back and suggest this strategy because newplot.m does know about the state of the axes. I will try and review the latest patch sometime later today.
|
Fri 06 Jan 2017 09:51:58 AM UTC, comment #13:
Now I see there is a much simpler alternative than the one I described in comment #12. The way we generally use newplot is as follows (excerpt from plot.m):
Which means we expect newplot to create the new axes/figure itself if it doesn't already exist. The simple approach is thus if newplot created the axes, then it knows it hasn't got to reset it. I attached a modified patch that implements this approach.
Now there is still the case
In this case newplot will still reset already clean figure/axes but I think we can live with it. Thoughts?
(file #39389)
|
Thu 05 Jan 2017 09:46:07 PM UTC, comment #12:
There could be a hidden base_property, e.g. "__is_clean__", that Freset can use to decide what to do.
At first sight, set, addproperty and addlistener (and the internal _go_axis_init_) are the only functions where one can alter a graphics object from the interpreter; but there may be others and we need to identify everyone... Those functions should take care of turning "__is_clean__" off, but only if necessary (set can return without error and without having actually changed any value).
At the end of the day the question is "is calling reset on eventually clean axes such an overhead?" At least we have lived with it up to now.
|
Thu 05 Jan 2017 06:24:44 PM UTC, comment #11:
I applied the first patch to stable after fixing the reject errors (http://hg.savannah.gnu.org/hgweb/octave/rev/14f1738a56ae). That takes care of the regression in this bug report.
On the dev branch, I modified your patch slightly. Instead of calling
from within newplot.m, I changed the code to directly issue the equivalent commands
This is a performance enhancement because all of the first part of cla.m is input validation and trying to determine which axes to act on before issuing the above commands. Since we already know the axes in question, there is a significant time savings to just issue the commands.
Also, I added
just to see what is happening during plotting.
So it seems that newplot.m needs to be altered to understand when a brand new axes has been created (in which case values are already at defaults) and when the axes of an existing plot is being modified.
@Pantxo: I'm uploading the latest version of the patch as rm_listeners3.cset so you can start from there.
(file #39370)
|
Thu 05 Jan 2017 01:59:53 PM UTC, comment #10:
Sorry, the previous comment should read "should not rely on m listeners.
|
Thu 05 Jan 2017 01:58:21 PM UTC, comment #9:
Whatever the outcome I still think that the first patch is worth applying: IMHO the behavior of base graphics objects like axes should rely on m listeners.
|
Thu 05 Jan 2017 09:00:09 AM UTC, comment #8:
I was sure I had modified cla.m to use reset instead of _go_axes_init_. That is what I did in the attached updated patch.
(file #39367)
|
Thu 05 Jan 2017 05:22:02 AM UTC, comment #7:
I tried the fourth patch in comment #6, but it fails the test code from the original poster.
This is probably because
eventually calls the same _go_axes_init_ code. If you look at cla.m you will find the following
do_reset will be true for "cla reset" so the else branch will be taken.
|
Wed 04 Jan 2017 09:53:55 PM UTC, comment #6:
>> ... there is nothing in the language that requires a programmer to be that forward thinking.
Yes average users should not have to bother with this and I agree that having "newplot" reset the listeners is the way to go.
In newplot.m I see this note:
I don't understand why we have to use _go_axes_init_ and why it has to delete and recreate labels, does someone have any insight?
Why not something like:
Currently "cla reset" will call axes::reset_default_properties which in turn follows approximately the same route as _go_axes_init_ so the attached cset would do the trick.
(file #39366)
|
Wed 04 Jan 2017 05:58:54 PM UTC, comment #5:
A good coding approach, but there is nothing in the language that requires a programmer to be that forward thinking.
The audience for Octave is not principally computer programmers, and many would assume that plotting something new into an existing figure would remove all traces of the previous plot. That is the mental model I would use.
I think the best path forward would probably be to modify the updaters patch so that it applies cleanly on stable (there were rejects when I tried). And then on the development branch implement the code to remove all listeners.
Does that sound like an okay plan?
|
Wed 04 Jan 2017 05:43:05 PM UTC, comment #4:
My approach in general (see annotation.m for figure listeners) is something like this:
|
Wed 04 Jan 2017 04:40:55 PM UTC, comment #3:
@Pantxo: Yes, using C++ updaters also works.
In general, however, shouldn't listeners be removed from axes objects when the axes is reset? What happens if someone uses addlistener for an axes object in their own m-file? I believe that will still cause problems which means switchi to C++ updaters is really only a partial fix.
|
Wed 04 Jan 2017 10:29:53 AM UTC, comment #2:
How about the attached cset which makes use of internal c++ (private) updaters instead of m listeners?
It will be API/ABI compatible and thus can be pushed to stable
(file #39364)
|
Wed 04 Jan 2017 01:44:44 AM UTC, comment #1:
Confirmed.
The problem is that there is a listener added to the axes object which will change the color of the labels when the axes color property is changed.
When you call any graphics function, one of the first things it does it call newplot() to either get a new axes object or reuse an existing one. If newplot decides to reuse an existing object it resets the default properties of the object. The sequence of events is hard to follow but eventually the C++ function _go_axes_init_ in graphics.cc is called. The issue is that the C++ function does not include removing listeners on the axes object.
When an attempt to change the axes color property is made by plotyy the old callback fires, but the ylabel that it pointed to is gone and so you get an error.
A simpler example to reproduce the error is:
I have two possible fixes. The correct way to fix this is to call remove_all_listeners() on the axes graphics_object. Unfortunately, the required function is part of the class base_graphics_object and is not exposed. In order to call it, the API for the class graphics_object needs to have a public function remove_all_listeners which calls the base_grahpics_object function of the same name. This would be a change to the internal API which is normally only made when Octave changes minor versions (say from 4.2 to 4.4). I've added jwe to the CC list so that he can comment on whether it would be acceptable in this instance. The attached file rm_all_listen.patch implements this approach.
The second approach modifies the callback function to check whether the handle is valid before calling set() to change the color. The attached file cb_color.patch implements this method. The problem with this is that the callback will still be called every time something modifies the color properties of the axes. If you do a series of several plots which re-use the same axes and all have labels or titles then the callback could be invoked a lot.
(file #39360, file #39361)
|
Tue 03 Jan 2017 10:16:09 PM UTC, original submission:
this script
produces this console output with both dev and 4.2.0 (not with 4.0.3)
The plotyy output looks normal.
The error is not shown when a "close all" is inserted just before the plotyy, or when the ylabel call is removed
|