NS_THEME_RESIZER implementation (GTK)

VERIFIED FIXED in mozilla1.9beta3

Status

()

--
enhancement
VERIFIED FIXED
17 years ago
9 years ago

People

(Reporter: ian, Assigned: ventnor.bugzilla)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla1.9beta3
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
This covers the implementation of NS_THEME_RESIZER for XUL resizers on GTK.
(Reporter)

Updated

17 years ago
Blocks: 117584

Updated

15 years ago
Blocks: 233462

Comment 1

15 years ago
taking
Assignee: blizzard → p_ch
(Assignee)

Comment 2

11 years ago
stealing
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Assignee: p_ch → ventnor.bugzilla
Status: ASSIGNED → NEW
(Assignee)

Comment 3

11 years ago
Created attachment 287785 [details] [diff] [review]
Patch

Done.
Attachment #287785 - Flags: superreview?(roc)
Attachment #287785 - Flags: review?(roc)
Comment on attachment 287785 [details] [diff] [review]
Patch

easy stuff
Attachment #287785 - Flags: superreview?(roc)
Attachment #287785 - Flags: superreview+
Attachment #287785 - Flags: review?(roc)
Attachment #287785 - Flags: review+
Attachment #287785 - Flags: approval1.9+
Component: Skinability → Widget: Gtk
QA Contact: pmac → gtk
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10

Comment 6

11 years ago
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".
(Assignee)

Comment 7

11 years ago
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

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

Comment 9

11 years ago
I filed bug 403887 for the Firefox statusbar resizer issue.
(Assignee)

Comment 10

11 years ago
The resizer has been disabled again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Depends on: 403887

Comment 11

11 years ago
Created attachment 292155 [details] [diff] [review]
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.
Attachment #292155 - Flags: review?(roc)
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.
Attachment #292155 - Flags: superreview?(roc)

Comment 13

11 years ago
Created attachment 292167 [details] [diff] [review]
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
Attachment #292155 - Attachment is obsolete: true
Attachment #292167 - Flags: superreview?(roc)
Attachment #292167 - Flags: review?(roc)
Attachment #292155 - Flags: superreview?(roc)
Attachment #292155 - Flags: review?(roc)
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.
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

11 years ago
Created attachment 292496 [details] [diff] [review]
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.
Attachment #292167 - Attachment is obsolete: true
Attachment #292496 - Flags: superreview?(roc)
Attachment #292496 - Flags: review?(roc)
Attachment #292167 - Flags: superreview?(roc)
Attachment #292167 - Flags: review?(roc)
+           // 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

11 years ago
Created attachment 292514 [details] [diff] [review]
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.
Attachment #292496 - Attachment is obsolete: true
Attachment #292514 - Flags: superreview?(roc)
Attachment #292514 - Flags: review?(roc)
Attachment #292496 - Flags: superreview?(roc)
Attachment #292496 - Flags: review?(roc)
I agree, I didn't mean you to use that there.
Comment on attachment 292514 [details] [diff] [review]
updated patch for gtk2 resizing based on roc's comments

lovely
Attachment #292514 - Flags: superreview?(roc)
Attachment #292514 - Flags: superreview+
Attachment #292514 - Flags: review?(roc)
Attachment #292514 - Flags: review+
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.
Attachment #292514 - Flags: approval1.9?
Comment on attachment 292514 [details] [diff] [review]
updated patch for gtk2 resizing based on roc's comments

a=mconnor on behalf of drivers
Attachment #292514 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: mozilla1.9 M10 → mozilla1.9 M11

Comment 24

11 years ago
Verified fixed on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011604 Minefield/3.0b3pre
Status: RESOLVED → VERIFIED
Depends on: 465628
Why did nobody think to implement this, or at least file bugs, on other OSes?

Comment 26

9 years ago
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.
You need to log in before you can comment on or make changes to this bug.