A minor comment: the mplot3d toolkit is a relatively new addition to matplotlib.  Perhaps its import can be wrapped in a try block so that users of old versions of matplotlib can still use replot.

Created attachment 169604 [details] [review]
replot: Implement more matplotlib commands and add 3D support

Rebase of patch to worksheet-printing patch

Created attachment 169605 [details] [review]
replot: Copy method name and docs to generated functions

When we are auto-generating functions based on Axes methods, we want
them to have proper __name__ and __doc__ attributes so that help works
usefully in Reinteract.

Review of attachment 169604 [details] [review]:

Generally, nice improvements. I agree with Robert that if mplot3d is recent (newer than 0.98) you should catch the import failing and skip the 3D part. Beyond that:

* You should add your copyright information to the header
* I think this patch should be split into three pieces:
  
  1) The dpi syncing code (It would make sense to me to add custom_result.ResultWidget as a base class to avoid duplicating complex code between this and your sympy patch and whatever else needs that.)
  2) The autogeneration of methods
  3) The 3d support

::: lib/replot.py
@@ +49,3 @@
+            self.axes = mplot3d.axes3d.Axes3D(self.figure)
+        else:
+            self.axes = self.figure.add_subplot(111)

Rather than using runt-time type checks here (and duplicating in the printing code as I did when I rebased to the printing branch) I think it would probably be better to have a function create_axes() function on the base result class that the subclasses override

@@ +52,3 @@
+
+        self.add_events(gtk.gdk.BUTTON_PRESS_MASK   | \
+                        gtk.gdk.BUTTON_RELEASE_MASK | \

The backslashes are unnecessary with the parentheses

@@ +54,3 @@
+                        gtk.gdk.BUTTON_RELEASE_MASK | \
+                        gtk.gdk.POINTER_MOTION_MASK | \
+                        gtk.gdk.POINTER_MOTION_HINT_MASK)

MOTION_HINT_MASK is highly evil. It's a broken idea.

When you get a motion event with is_hint, you need to query the server using XQueryPointer so you get more motion events. The question is, do you use the coordinates in the event you got, or the coordinates returned from XQueryPointer.

Coordinates from XQueryPointer are wrong:

 user moves pointer
   motion event with is_hint is sent
 user releases mouse
                      process motion event, get coordinates
                      process mouse release

The coordinates when we process are the motion coordinates *after* the user has released the mouse - we've overshot the release position.

Coordinates in event are wrong, *too*:

 user moves pointer
  event with is_hint is sent
                            
 user moves pointer
   no event sent
                      process motion event, use old coordinates

The position is "stuck" - and we won't update the position until the user releases the mouse or moves the mouse again.

Here's the correct way to do compression (maybe gtk3 will do this automatically...)

 Don't specify MOTION_HINT_MASK
 When you get a mouse event, save it, and add an idle with priority GTK_PRIORITY_RESIZE + 1
   (that's glib.PRIORITY_HIGH_IDLE + 21)
 If the idle fires before any other events, deliver the motion event
 Otherwise:
  If you get another motion event, replace the saved event with the new event
  If you get a button press or release deliver a saved motion event immediately, then deliver the button press or release

(In many cases, rather than that complexity, it's better to skip compression. But compression is obviously needed here because of the slowness of redraw.)

@@ +56,3 @@
+                        gtk.gdk.POINTER_MOTION_HINT_MASK)
+
+        self.caching_surface = None

The rename of cached_contents to caching_surface doesn't seem necessary to me. (Your name isn't worse, but it isn't, IMO, much better and the default should be not to change.)

@@ +66,3 @@
+        self.window.invalidate_rect(None, False)
+
+    def do_screen_changed(self, previous_screen):

See comments on sympy submission

@@ +313,3 @@
         canvas = _PlotResultCanvas(figure)
 
+        if isinstance(self, Axes3D):

See comment above

@@ +360,3 @@
+        getattr(axes, method_name)(*args, **kwargs)
+        return axes
+    return _func

See my attached patch that improves this by propagating __name__ and__doc__

@@ +364,2 @@
+for method_name in ('bar', 'boxplot', 'errorbar', 'contour', 'hist', 'imshow', 'plot', 'quiver', 'specgram'):
+    setattr(sys.modules[__name__], method_name, _create_method(Axes, method_name))

I think you can do this as

 globals()[method_name] = _create_method(...)

Which is a bit cleaner.

Created attachment 169942 [details] [review]
Patch #1: Implement result_widget.CustomResult base class

Here's a patch implementing a result_widget.CustomResult base class and has replot derive from it. I'll do the rest of the patches (and update resympy) once this is through.

Review of attachment 169942 [details] [review]:

This looks very good. I've gone ahead and pushed it. Couple of things that I thought about when reviewing it:

 - Docs for sync_dpi and sync_style would be good. See how I documented CustomResult.print_result in the
   same file for the general style.

 - It's unclear from the code why you call queue_resize() after calling sync_style, but not after sync_dpi.
   Thinking about it, I remembered that when the DPI changes, GTK+ fakes a style change on all widgets
   (the call to gtk_rc_reset_styles() in gtk/gtksettings.c:gtk_settings_notify()), but if that's what
   you were thinking of, it might be nice to have a comment

    # we don't need to queue a resize here since GTK+ always forces a style change when the DPI changes

   In the place where the queue_resize() would otherwise be.

If you want to make a follow-up patch with those improvements, would be cool.

The other thing I thought about was whether the function should be _sync_style and _sync_dpi because they are internal functions that shouldn't be called by callers outside of the call itself.

See the way that non-public functions for child classes to override are handled in editor.py (like _get_display_name())

I think probably not - for stuff that we are "exporting" out of Reinteract to people creating external components - it's probably best to keep underscore prefixing for "internal details you don't need to touch". (And it wouldn't hurt if someone did call sync_dpi/sync_resolution.)

Created attachment 219827 [details] [review]
New patch wrapping additional matplotlib commands

This is a rebase of the patch from 2 years ago, including the method name and docs change you suggested.

I did not add mplot3d checks as it has been around for a while now.

P.S.  I've been using reinteract quite a lot over the past years, and it would be very nice not to have to patch in simple changes like this every time I do an upgrade ;-)

Reinteract is not under active development anymore and had its last code changes
in early 2012: http://git.fishsoup.net/cgit/reinteract/log/

Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect
reality. Please feel free to reopen this ticket (or rather transfer the project
to GNOME Gitlab, as GNOME Bugzilla is deprecated) if anyone takes the
responsibility for active development again.