Timothy Arceri Assignee
	Description • 11 years ago

As originally report here: https://bugzilla.gnome.org/show_bug.cgi?id=534976

The GTK filepicker does not rotate the preview image base on the exif orientation tag.
This can be corrected using gdk_pixbuf_apply_embedded_orientation() which was added in GTK 2.12. As the current build requirement is only 2.10 I'm proposing this be added as part of the port to GTK 3.0 as it hardly seems worth bumping the GTK 2 version at this point.

	Karl Tomlinson (:karlt)
	Comment 1 • 11 years ago

Most Mozilla apps are now built against GTK+-2.18 or newer.
http://www.mozilla.org/en-US/firefox/21.0/system-requirements/

The 2.10 build requirement remains just because of bug 795354.

However, I expected GTK code to determine image previews for its filepicker.  I'm surprised if there is Gecko code providing the previews for the GTK filepicker.

	Timothy Arceri Assignee
	Comment 2 • 11 years ago

GTK does not do not provide previews by default the widget is created on the application side and passed to the GTK filechooser. I have not got around to testing this yet but I think the fix is to add a call to gdk_pixbuf_apply_embedded_orientation() in the UpdateFilePreviewWidget() function of nsFilePicker.cpp

http://hg.mozilla.org/mozilla-central/file/6e45e9f62d21/widget/gtk2/nsFilePicker.cpp#l82

	Philip Chee
	Comment 3 • 11 years ago

> Depends on: 795354
In general SeaMonkey bugs should not block core bugs. We appreciate your concerns however. CC: Callek.

	Philip Chee
	Comment 4 • 11 years ago

> In general SeaMonkey bugs should not block core bugs
After discussing this with Callek, if we (SeaMonkey) break, we'll manage one-way-or-another.
However there are other reasons for the 2.10 dependency, e.g. older linux distros Mozilla still wants to support,

	Timothy Arceri Assignee
	Comment 5 • 11 years ago

(In reply to Philip Chee from comment #4)
> However there are other reasons for the 2.10 dependency, e.g. older linux
> distros Mozilla still wants to support,

The last time the version was bumped up support for older distros was discussed in bug 418885 in the end it looks like the consensus was that the older distros should package the required librarys. However it the case of that bug 2.10 had only been around for around 1 year. 2.18 seems to be the suggested next jump and has been around for almost 4 years (Sep 2009) so I wouldnt expect as greater uproar or shock.

Either way its obviously not my call. Karl are you happy to update the dependency to 2.18?

	Karl Tomlinson (:karlt)
	Comment 6 • 11 years ago

Firefox and Thunderbird are built against 2.18 and have a run-time dependency > 2.10.  (I can't remember exactly.)

I recommend surrounding the gdk_pixbuf_apply_embedded_orientation code with a preprocessor conditional block using GTK_CHECK_VERSION.

https://developer.gnome.org/gtk2/2.24/gtk2-Feature-Test-Macros.html#GTK-CHECK-VERSION:CAPS

	Timothy Arceri Assignee
	Comment 7 • 11 years ago

Thanks for your help Karl. This patch fixes the issue.

----------------------------------------------------------------------------
Unrelated to this patch. When I did a grep for GTK_CHECK_VERSION it came up with a number of what looks to be old checks for gtk 2.10 and under. Are these still required or would you like it if I created a new bug for this and made a patch to remove any old code used in these calls.

./widget/gtk2/nsScreenManagerGtk.cpp:#if GTK_CHECK_VERSION(2,2,0)
./widget/gtk2/nsScreenManagerGtk.cpp:#endif // GTK_CHECK_VERSION(2,2,0)
./widget/gtk2/nsNativeThemeGTK.cpp:#if GTK_CHECK_VERSION(2,10,0)
./widget/gtk2/nsNativeThemeGTK.cpp:#endif // GTK_CHECK_VERSION(2,10,0)
./widget/gtk2/nsScreenGtk.cpp:#if GTK_CHECK_VERSION(2,0,0)
./toolkit/xre/nsAppRunner.cpp:#if GTK_CHECK_VERSION(2,8,0)

	Karl Tomlinson (:karlt)
	Comment 8 • 11 years ago

(In reply to Timothy Arceri from comment #7)
> When I did a grep for GTK_CHECK_VERSION it came up
> with a number of what looks to be old checks for gtk 2.10 and under. Are
> these still required or would you like it if I created a new bug for this
> and made a patch to remove any old code used in these calls.
> 
> ./widget/gtk2/nsScreenManagerGtk.cpp:#if GTK_CHECK_VERSION(2,2,0)
> ./widget/gtk2/nsScreenManagerGtk.cpp:#endif // GTK_CHECK_VERSION(2,2,0)
> ./widget/gtk2/nsNativeThemeGTK.cpp:#if GTK_CHECK_VERSION(2,10,0)
> ./widget/gtk2/nsNativeThemeGTK.cpp:#endif // GTK_CHECK_VERSION(2,10,0)
> ./widget/gtk2/nsScreenGtk.cpp:#if GTK_CHECK_VERSION(2,0,0)
> ./toolkit/xre/nsAppRunner.cpp:#if GTK_CHECK_VERSION(2,8,0)

A patch to remove these would be great, thanks.

	Karl Tomlinson (:karlt)
	Comment 9 • 11 years ago

Comment on attachment 755042 [details] [diff] [review]
Use correct preview orientation if set in tag

gdk_pixbuf_apply_embedded_orientation() doesn't actually modify or unref the src pixbuf
https://developer.gnome.org/gdk-pixbuf/stable/gdk-pixbuf-Utilities.html#gdk-pixbuf-apply-embedded-orientation
and so another g_object_unref() is required.  An additional pointer variable will be necessary because the src and return value may be different pixbufs.

	Timothy Arceri Assignee
	Comment 10 • 11 years ago

Yeah sorry I realised my mistake after posting the patch. Hopefully this one should do the trick.

	Timothy Arceri Assignee
	Comment 11 • 11 years ago

Sorry for the noise. This patch does the same thing as the v2 one Karl reviewed but I fixed up the issue with it needing to be applied after the v1 (this is only the second time I've used Mercurial I didnt realise that creating a patch commited the change locally)

	Philip Chee
	Comment 12 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d50edb6a79aa

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/d50edb6a79aa