Created attachment 87637 [details] [review]
Encapsulate bacon-resize.[ch] in an object

This patch converts bacon-resize.[ch] to using an object, but doesn't attempt to remove any of the gdk_screen_width() (etc.) calls.

I'm also a bit iffy on the name of the "bacon_resize_resize" function, but it's what logically follows from the original "bacon_resize" function.

On second thoughts, it might be better to have bacon_resize_new return the success boolean, and take a pointer for the new BaconResize as a parameter. I'll do this once I get some feedback on the issues above.

I would suggest to add have-xvidmode as a read-only property to the BaconResize object.

Created attachment 87748 [details] [review]
Make have-xvidmode a property

As Jan suggested, I've moved have-xvidmode to a property of the BaconResize object.

Created attachment 87786 [details] [review]
Use the screen and display from the related video widget.

This is not really tested.

Shouldn't bacon_set_video_widget actually be called bacon_resize_set_video_widget...or just set_video_widget, since it's private?

Wouldn't it also be a good idea to allow getting of the video-widget property, or am I just wanting this for completionness' sake? :-P

Hm, yes bacon_resize_set_video_widget would be better.

BaconResize is something like a private object of the video widget, so I don't see an use case for a readable property.

I don't think a readable property is needed, as the widget is only used to get a GdkDisplay and GdkScreen. But the code doesn't seem to take into account:
- screen changing after bacon_set_video_widget is called (ie. multi-head display, and the window being moved somewhere else)
- display changing

See bug 371544 and bug 369134

I'm a bit out of my depth here (multihead makes my head spin), but would a solution be to connect to the video widget's "screen-changed" signal, and then update the fullscreen-ness on the new screen?

(In reply to comment #9)
> I'm a bit out of my depth here (multihead makes my head spin), but would a
> solution be to connect to the video widget's "screen-changed" signal, and then
> update the fullscreen-ness on the new screen?

Yeah, that would be enough.

Updated patch status. Jan, if you don't have time to update your patch, I'll try and run with it sometime next week.

I don't have time to update.

There is a bug in the patch above. It should be
XF86VidModeGetModeLine (GDK_DISPLAY_XDISPLAY (display), GDK_SCREEN_XNUMBER (screen), &dotclock, &modeline)
instead of GDK_SCREEN_XSCREEN (screen).

Created attachment 100228 [details] [review]
GObjectify BaconResize (updated)

This builds on Jan's patch, including his changes from comment #12, as well as now listening for screen changes as per comment #10.

Hopefully this is OK; it works fine on my single-screen setup, but I don't have a multi-head setup with which to test it.

I've done the multi-head support myself in trunk (to help with bug 472494). Could you please update your patch?

Created attachment 102411 [details] [review]
GObjectify BaconResize (updated again)

Updated to merge your changes in with the changes from the last patch. I assume the listening to "screen-changed" is still required?
As usual, I haven't tested it on a multihead setup, as I don't have one. :-(

I broke it again... I'll test it and commit it when you've updated it again. Sorry about that.

Created attachment 102575 [details] [review]
GObjectify BaconResize (updated again)

Here we go.

David, any chance you could test it works for you, in SVN?

2008-01-11  Bastien Nocera  <hadess@hadess.net>

        * src/backend/bacon-resize.c: 
        * src/backend/bacon-resize.h:
        Patch from Philip Withnall <pwithnall@svn.gnome.org> to turn
        the bacon-resize code into an object

        * src/backend/bacon-video-widget-gst-0.10.c:
        (bacon_video_widget_unrealize),
        (bacon_video_widget_set_fullscreen),
        (bacon_video_widget_new):
        * src/backend/bacon-video-widget-xine.c:
        (bacon_video_widget_realize),
        (bacon_video_widget_unrealize), 
        (bacon_video_widget_set_fullscreen):
        Port to the new API (Closes: #370099)

Yup, this also works nicely on my 2 x 1680x1050 dual monitor setup.

Great, closing. Thanks for the work Philip.