118299 - NS_THEME_RESIZER implementation (GTK)

Status: VERIFIED FIXED

(Core :: Widget: Gtk, enhancement)

Description

[Hixie (not reading bugmail)]

This covers the implementation of NS_THEME_RESIZER for XUL resizers on GTK.

Comment 1

[Pierre Chanial]

taking

Comment 2

[Michael Ventnor]

stealing

Comment 3

[Michael Ventnor]

Attachment: Patch

Done.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 287785 [details] [diff] [review]
Patch

easy stuff

Comment 5

[Reed Loden [:reed]]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.34; previous revision: 1.33
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.32; previous revision: 1.31
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.108; previous revision: 1.107
done

Comment 6

[Sylvain Pasche]

I see a strange behavior while trying to use the new resizer: while dragging the resizer it does not follow the mouse, and the window starts to "jump around".

Comment 7

[Michael Ventnor]

Thats a problem with the actual resizer code, not this cosmetic change. I don't know what to do about that since I don't know that code.

Comment 8

[Sylvain Pasche]

Do you know if it is filed? I found bug 189435 but that's not exactly what I'm seeing.

Comment 9

[Sylvain Pasche]

I filed bug 403887 for the Firefox statusbar resizer issue.

Comment 10

[Michael Ventnor]

The resizer has been disabled again.

Comment 11

[Ian McKellar]

Attachment: Patch to use window manager's resize

This patch re-adds resizing support on gtk2.

Firstly, it reverses the patch in bug 403887 so that resizers show up again and are painted by the theme.

Next it adds an interface nsIWindowResize that has an enum describing direction and a single method beingResize(direction) that requests that a resize begins.

There's an implementation of that interface on gtk2's nsWindow. It uses gtk's gdk_window_begin_resize_drag to ask the window manager to begin a drag.

Finally this is tied into nsResizerFrame. On NS_MOUSE_BUTTON_DOWN it checks if the nsIWindowResize interface is present on the event's widget. If it is then that interface is used instead of the current nsResizerFrame code path.


This is a port of a patch that's going into Songbird right now. I'm happy to rework the style of the patch. I added an interface because that seemed like the least intrusive approach.

Comment 12

[Reed Loden [:reed]]

Comment on attachment 292155 [details] [diff] [review]
Patch to use window manager's resize

>+    printf ("CaptureMouse\n");
>+

That shouldn't be there. :)

Don't forget to request superreview, too.

Comment 13

[Ian McKellar]

Attachment: Add gtk2 resizing by talking to the window manager

remove some debug code I'd left in the last patch.
see my comments on the last patch:
https://bugzilla.mozilla.org/show_bug.cgi?id=118299#c11

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is fairly awesome, thanks!!!

Don't do nsIWindowResize in IDL. IDL is only necessary when you think the interface should be scriptable, and none of our widget-related code is or should be scriptable. In fact I'd just add BeginResize to nsIWidget. Return NS_ERROR_NOT_IMPLEMETED in nsBaseWidget. Instead of direction constants, how about just taking two PRInt32s, one for each direction, each -1, 0 or 1? Use an array of PRInt8 pairs to convert from eDirection to those values.

nsResizerFrame should do the same thing instead of its eDirection thing, but that should be another cleanup bug. I'll file it.

+           // and we're done with this event
+           *aEventStatus = nsEventStatus_eConsumeNoDefault;
+           doDefault = PR_FALSE;

This code can be shared by both paths.

+    // get the gdk window for this widget
+    GdkWindow* gdk_window = mDrawingarea->inner_window;
+    if (!GDK_IS_WINDOW(gdk_window)) {
+      return NS_ERROR_FAILURE;
+    }
+
+    // find the top-level window
+    gdk_window = gdk_window_get_toplevel(gdk_window);
+    if (!GDK_IS_WINDOW(gdk_window)) {
+      return NS_ERROR_FAILURE;
+    }
+
+    // get the current (default) display
+    GdkDisplay* display = gdk_display_get_default();
+    if (!GDK_IS_DISPLAY(display)) {
+      return NS_ERROR_FAILURE;
+    }

Do we really need these _IS_ checks? They seem redundant.

You could pass an nsGUIEvent* down to BeginResize. The raw X event is accessible via void* nativeMsg on that struct (or at least it can be if you set that up in nsWindow where the event is dispatched to Gecko). Then you wouldn't have to make up a time. You can get the nsGUIEvent* from the nsIDOMEvent by QIing to nsIPrivateDOMEvent and calling GetInternalNSEvent.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess you would have to make up a time when some of that stuff is null. Still worth doing though.

Filed bug 407616 on nsResizeFrame cleanup. It would make a good first bug for someone.

Comment 16

[Ian McKellar]

Attachment: An update to the gtk2 resizer patch, based on roc's comments

(In reply to comment #14)
> Don't do nsIWindowResize in IDL. IDL is only necessary when you think the
> interface should be scriptable, and none of our widget-related code is or
> should be scriptable. In fact I'd just add BeginResize to nsIWidget. Return
> NS_ERROR_NOT_IMPLEMETED in nsBaseWidget. Instead of direction constants, how
> about just taking two PRInt32s, one for each direction, each -1, 0 or 1? Use an
> array of PRInt8 pairs to convert from eDirection to those values.

I did most of this. I don't know quite what you mean about the PRInt8 array though. Do you have an example of what you'd like me to do there? I just used a bunch of if statements.
> 
> +           // and we're done with this event
> +           *aEventStatus = nsEventStatus_eConsumeNoDefault;
> +           doDefault = PR_FALSE;
> 
> This code can be shared by both paths.

Good point - I fixed that.
> 
> +    // get the gdk window for this widget
> +    GdkWindow* gdk_window = mDrawingarea->inner_window;
> +    if (!GDK_IS_WINDOW(gdk_window)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    // find the top-level window
> +    gdk_window = gdk_window_get_toplevel(gdk_window);
> +    if (!GDK_IS_WINDOW(gdk_window)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    // get the current (default) display
> +    GdkDisplay* display = gdk_display_get_default();
> +    if (!GDK_IS_DISPLAY(display)) {
> +      return NS_ERROR_FAILURE;
> +    }
> 
> Do we really need these _IS_ checks? They seem redundant.

The main thing I want them for is to test for NULL. The added gtype check seemed like it was worthwhile. I don't think it's expensive - I think it gets the type code by reading a global variable and tests one field on the object.
> 
> You could pass an nsGUIEvent* down to BeginResize. The raw X event is
> accessible via void* nativeMsg on that struct (or at least it can be if you set
> that up in nsWindow where the event is dispatched to Gecko). Then you wouldn't
> have to make up a time. You can get the nsGUIEvent* from the nsIDOMEvent by
> QIing to nsIPrivateDOMEvent and calling GetInternalNSEvent.

I've already got the nsGUIEvent* and I found that nsGUIEvent already has time on it so I don't even need to attach the GdkEvent.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+           // we're tracking.

Make this clearer. E.g. "No native resize support, implement resizing ourselves". Probably you should check rv == NS_ERROR_NOT_IMPLEMENTED instead of NS_FAILED(rv).

+         PRInt32 horizontal = 0;
+         PRInt32 vertical = 0;
+         switch(mDirection)
+         {
+            case topleft:
+              horizontal = -1;
+              vertical   = -1;
+              break;

Do something like this?

static const PRInt8 directions[][2] = {
  {-1, -1}, {0, -1}, ...
};
  PRInt32 horizontal = directions[mDirection][0];
  PRInt32 vertical = directions[mDirection][1];

Comment 18

[Ian McKellar]

Attachment: updated patch for gtk2 resizing based on roc's comments

I don't see an elegant way to turn the {-1, 1} style directions into Gdk window edges in nsWindow.cpp. All the ideas I had turned into messes of trinary operators and shifts.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree, I didn't mean you to use that there.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 292514 [details] [diff] [review]
updated patch for gtk2 resizing based on roc's comments

lovely

Comment 21

[Reed Loden [:reed]]

Comment on attachment 292514 [details] [diff] [review]
updated patch for gtk2 resizing based on roc's comments

Fixes resizer problems and reenables it on Linux.

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 292514 [details] [diff] [review]
updated patch for gtk2 resizing based on roc's comments

a=mconnor on behalf of drivers

Comment 23

[Reed Loden [:reed]]

I revved the IID on nsIWidget on check-in.

Checking in layout/xul/base/src/nsResizerFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsResizerFrame.cpp,v  <--  nsResizerFrame.cpp
new revision: 1.38; previous revision: 1.37
done
Checking in widget/public/nsIWidget.h;
/cvsroot/mozilla/widget/public/nsIWidget.h,v  <--  nsIWidget.h
new revision: 3.166; previous revision: 3.165
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.122; previous revision: 1.121
done
Checking in widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.241; previous revision: 1.240
done
Checking in widget/src/gtk2/nsWindow.h;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.h,v  <--  nsWindow.h
new revision: 1.80; previous revision: 1.79
done
Checking in widget/src/xpwidgets/nsBaseWidget.cpp;
/cvsroot/mozilla/widget/src/xpwidgets/nsBaseWidget.cpp,v  <--  nsBaseWidget.cpp
new revision: 1.166; previous revision: 1.165
done
Checking in widget/src/xpwidgets/nsBaseWidget.h;
/cvsroot/mozilla/widget/src/xpwidgets/nsBaseWidget.h,v  <--  nsBaseWidget.h
new revision: 1.91; previous revision: 1.90
done

Comment 24

[Elmar Ludwig]

Verified fixed on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011604 Minefield/3.0b3pre

Comment 25

[[e-mail]]

Why did nobody think to implement this, or at least file bugs, on other OSes?

Comment 26

[Ian McKellar]

This is all from memory since I did this stuff at Songbird about a year ago and right now I'm traveling the world. (Hi from Salzburg, Austria)

I think the issue is that other toolkits (Win / Mac) don't really have this concept. On Windows you can begin a resize drag in response to a mousedown event, but I'm not sure that there's a widget you can draw for it. On the Mac I think you can turn on and off the window corner resize drawing and behaviour, but I don't think it's easy to do arbitrary resize widgets like you can on Linux. The code for all this is in Songbird though and somebody could probably dig it out if there was enough interest.