On an vaguely related note, I noticed the methodsAxes.{get,set}_cursor_props, which seem to have no effect and have never been used (according togit log -S). Would it be OK to just get rid of them or should we go through a deprecation cycle as usual?

... or we make it actually do something. It is fully functional and
documented... it's state just doesn't seem to get accessed anywhere else,
though.

On Sat, Jun 18, 2016 at 4:48 AM, Antony Leenotifications@github.com
wrote:

On an vaguely related note, I noticed the methods
Axes.{get,set}_cursor_props, which seem to have no effect and have never
been used (according to git log -S). Would it be OK to just get rid of
them or should we go through a deprecation cycle as usual?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6603 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AARy-KQywk9bptlSbK-_WNfGswv8aOfBks5qM7DHgaJpZM4I45VE
.

I am curious of the history when those methods were added and why. There are a surprising number if bread crumb trails in the codebase.

Also, this is ahuge UX improvement this should be backported to 2.0

Perhaps you're better than me atgit log -{G,S} but at least I didn't find anything :-( Just seems like a completely unused attribute.
Who are the Tk and WX specialists there? The feature doesn't work with these backends (nothing's broken but the cursor doesn't change) so it'd be nice but not obligatory to fix that first.

Are the Cairo backends also handled here?

Good catch. A quick look seems to indicate that the Cairo backend does not overridedraw; where should the cursor change go?

Maybe on theon_draw_event in gtk3cairo andexpose_event in gtk_cairo?

Done. The cursor switches to a spinwheel, but is not animated, for a reason I don't understand.

Side notes:

• The GTK3Cairo backend prints the warning "(-c:14995): Gtk-WARNING **: Allocating size to GtkToolItem 0x5564c90a5490 without calling gtk_widget_get_preferred_width/height(). How does the code know the size to allocate?" when picking the zoom tool.
• It would probably be worth upping the minimum PyGTK version requirement to something like 2.12 (2009) to get rid of conditional branches that support old APIs.

@OceanWolf@fariza can you address the gtk questions?

Right now it looks like Qt4Agg Qt5Agg GtkCairo and Gtk3Cairo all work (I don't see the warning mentioned in#6603 (comment) anymore). In theory Tk and WX "should" be supported too but don't actually work. Could we push this forward with just support for these backends?

Ping@fariza@anntzer; what should we do about this? Seems useful...

Rebased. Now works on py3: qt4 qt5 tk gtk3 and py2: wx gtk (haven't tested other combos). See the latest commit for the reason (we needed to trigger the event loop on some backends).

@fariza What owns the cursor in the new tool model? It does make a certain amount of sense for the toolbar to do so (as it holds the other things that change the state of the cursor. The other logical option is for the manager to own it.

Not sure I like putting this all the way down inAgg, seems like it should be fully owned by the GUI layer.

A decorator like:

defwating_cursor(meth):@contextlib.contextmanagerdefspin_cursor(manager):managerandmanager.set_cursor(cursor.WAIT)try:yieldfinally:managerandmanager.set_cursor(cursor.POINTER)@functools.wraps(meth)definner(self,*args,**kwargs):mager=getattr(self,'manager',None)withspin_cursor(manager):returnmeth(self,*args,**kwarg)

would make this easy and safe to apply.

@anntzer is the problem of returning to previously selected cursor solved?

@tacaswell the cursors doesn't belong to the toolbar, right now there is a tool that sets the cursor when in axesbackend_tools.SetCursorBase._set_cursor_cbk.

We can just trigger an event to inform the tool that we are waiting. But this seems a little bit overkill and maybe too much resources?

Now it is :) see latest commit

@tacaswell I'm not convinced that adding a contextmanager around each and every call to draw() from an interactive backend is the solution (e.g. people sometimes call figure.canvas.draw() themselves)... or I guess interactive backends could derive from a common InteractiveCanvas class (earlier than FigureCanvasAgg in the MRO) which definesdraw() as

def draw(...):    <sets cursor>    super().draw(...)    <unsets cursor>

but then you'reactually relying on multiple inheritance to do your job correctly (which I am actually totally fine with).

For the toolmanager, I think it can be done using thetrigger method of theToolSetCursor, we can use that method to override the active cursor... I will try to find some time this week to give it a try.

@tacaswell what would be the problem movingset_cursor from thetoolbar to thefirgurecanvas? it seems a more logical place for it to be, and more useful.

(fwiw I agree that it may make sense to allow ourselves to change the cursor even if there is no toolbar (ie in some embedding cases))

The canvas class makes more sense than the manager class so that direct embeddings can take advantage of it, but we will have to be careful to not mess with anything end-user may already be doing with the cursor.

Fair enough.

Rebased.

So we are not moving theset_cursor to the canvas? I think this is the right time and place to do it

We can do that later, I think it is worth getting the busy-spinner in now.

is this ready to go? looks like features are there...time to merge this 1-year-old PR? :-)

(I think this is good to go.)

rebased

Allowing myself to mark this as release critical as it's been around for very long, it's been approved, and@tacaswell says it's "ahuge UX improvement" :-)

I am happy this went in 👍