GNOME Bugzilla – Bug 139714
Image preview when adding new background
Last modified: 2005-01-09 23:59:43 UTC
Description of Problem: When you are browsing the file system for new background images it would very useful to be able see a preview of the image before using it. From what I understand this hopefully is quite easy now with the new file selector.
Created attachment 27286 [details] [review] little patch to fix this it doesn't use thumbnailing for the preview images, it just scales the images without this for not creating lots of unnecessary thumbnails. And obviously doesn't show any preview if the selected file is not an image nor if there are several selected files looks OK to commit?
doh! adding PATCH keyword, sorry for the spam
*** Bug 141679 has been marked as a duplicate of this bug. ***
*** Bug 142518 has been marked as a duplicate of this bug. ***
Rodney, no comments on the patch?
Sorry. I thought I had already added comments. Anyway, I don't like the patch as it is. The patch needs to use the thumbnailing system. And it shouldn't use a GtkFrame, or manually set the size request for it. The formatting also looks incorrect in some places. Nor is there an real reason to kill the GtkImage and re-create it every time.
It's ok with me, I didn't use thumbnails because I discovered that my .thumbnails directory had ~1900 files (which use about 35MB of my home directory), if normal desktop usage can create such number of files, I saw totally pointless to contribute to the clutter just for searching new backgrounds But anyway, by using thumbnails we get rid of the GtkFrame and the manual size request, and I'll fix the formatting problems, so I'll work in a new patch soon
Dobey, I've been giving some thinking to it, shouldn't the GtkFrame and it's manual size setting be necessary for previewing images of different sizes without resizing continously the whole dialog? just when I tried to remove this I remembered why did I put it
We shouldn't use a GtkFrame. Using a GtkVBox, and adding some extra information in the preview area might be useful though. I would ask what GIMP does, but I don't think it uses the GtkFileChooser API yet. EOG also does not have a preview widget. So, I'm not sure what to suggest exactly for that. Maybe we should just create a standard widget for doing image previews, for other people to use as well.
Created attachment 28217 [details] [review] Patch which uses thumbnailers The patch I created currently will show any info available in the thumbnail image, and some metadata (mime type, "Thumb::Image::{Width,Height}", "Description") if available. Currently GnomeThumbnailFactory does not appear to add this metadata, though it can be enhanced to do so w/o breaking anything, AFAIK.
Created attachment 28219 [details] [review] Fixed patch to properly get tExt::* extensions.
Created attachment 28248 [details] image previewer widget, .C file
Created attachment 28249 [details] image previewer widget, .H file
Created attachment 28250 [details] [review] patch to use the widget for previewing images the widget provides a simple API for previewing images, it could be used too in gnome-terminal BG selector, gnome-panel BG selector, eog, etcetera... James, we might integrate the file properties stuff and propose the widget for any app that needs it, that could rock :)
Well, it's possible, but what's needed for more generic "pick-an-image" stuff is a generic GtkImageChooser(Button/Dialog/Widget) that handles themed icons and thumbnailed image files. There's discussions at http://bugzilla.gnome.org/show_bug.cgi?id=128723 and http://mail.gnome.org/archives/desktop-devel-list/2004-January/msg00675.html (For what's suggested there to work properly, thumbnailing of GdkPixbuf-supported types should be moved into gdk-pixbuf [and I think the md5 stuff should be moved into glib, personally]. A GtkImagePreview would certainly be desirable then. June 15 is the cutoff date for new features in GTK+, and there's going to be a meeting next Monday at 20:00 UTC in #gtk-devel. E-mail me at jcape@ignore-your.tv [or find me -- Jimbob -- on irc.gnome.org] if you're interested.) So far as the given patches are concerned, my only thought (aside from getting a label widget into the widget that uses the metadata) is that they shouldn't use the "Gnome" namespace (GnomeWP would be preferred, I think). I'm thinking that for this case they may be overkill since code to handle all the thumbnailing and such already exists.
I don't like the hiding/showing behaviour of the preview. It's very confusing, as it won't be there when you first open the dialog, but will be if you select an image in the file list. And if you scroll down quickly in a directory that has a lot of image and non-image files, the preview will continually hide and show itself, creating a lot of flicker. We should just always show the area. I haven't heavily reviewed the code for the widget itself or the patch, but this just what I get from building it and trying it.
Created attachment 31998 [details] updated image preview widget, .c file
Created attachment 31999 [details] updated image preview widget, .h file
Created attachment 32000 [details] [review] patch to use the new[er] widget for previewing files and using GtkFileFilters I took a stab at updating the previous preview widget integration. The flickering issue is now gone. I also added 2 file filters, so the dialog should only show images by default. Right now a group of image mimetypes is just hardcoded in, until I can find a better solution for masking on them. GtkFileFilter doesn't seem to support just adding a filter on the supertype.
Please remove the file filter code from this patch. It is orthogonal to the bug, and needs to be implemented separately, as it is more complicated. We also don't want to just handle image/* anyway either, since we may not support all image types, and most certainly don't.
Created attachment 32051 [details] updated image preview widget, .h file Just moved all the file chooser integration code into the preview widget's space.
Created attachment 32052 [details] updated image preview widget, .c file
Created attachment 32053 [details] [review] updated patch (remove file filters) I updated the patch to remove the filter filters. I also restructured the code slightly so that the all of the file chooser integration code now resides within the GnomeImagePreviewer, so you can just call a funtion on your file chooser and it will set it all up.
*** Bug 154011 has been marked as a duplicate of this bug. ***
Another patch was proposed in bug 154011.
Created attachment 33704 [details] [review] updated patch from 3205[123] this patch regroups the 3 previous one and fixes some build warnings. I've tested it on HEAD an it works fine, could we get it in now ?
Created attachment 35691 [details] [review] simple patch This is a quite simple version of the patch based on one of them provided in the bug (and integrated in the fedora and ubuntu packages). I'm not sure than this capplet is the right place to add a new widget and feature freeze is monday, is that ok to commit this version now ? - it's pretty simple - the size of the preview area doesn't change and display an icon on non/picture I think we should get this for 2.10 and try to work on the common widget for 2.12.
What do we need the label for filename for? It's already visible in the list, so I don't see why we need to duplicate it next to the preview thumbnail. What happens if the thumbnail is larger than 128 pixels wide? Does it get cut off? Do we resize anyway? Also, it looks like the spacing is wrong for the extra widgets. They are adding extra padding. I don't like this. Most of this code doesn't look like it's going to work, unless the image is already in the capplet anyway. Let's just fix it so that we have a centered image, and no labels.
Created attachment 35716 [details] [review] new patch New version of the patch without the filename and the padding. Are you sure you want to center the thumbnail ? It'll move depending of the height of the thumbnail. Gimp and eog also put it on the top for example. About the thumbnail size: capplet->thumbs = gnome_thumbnail_factory_new (GNOME_THUMBNAIL_SIZE_NORMAL); and in libgnomeui/gnome-thumbnail.c: gnome_thumbnail_factory_generate_thumbnail (GnomeThumbnailFactory *factory, ... size = 128; The size for GNOME_THUMBNAIL_SIZE_NORMAL is 128 so there is no reason to get a thumbnail larger than that. > Most of this code > doesn't look like it's going to work, unless the image is already in the capplet > anyway. I don't get why. The code is pretty simple and I've tested the fileselector on different files/dir which are not in the capplet without any issue here. Where is the issue exactly ?
All of the parsing to get metadata out of the png headers is useless unless the image was already in the capplet at some point, since nothing else that I know of, actually sets that metadata. And in fact, gdk-pixbuf doesn't preserve the data, unless the application using it, pulls all the data out, and restructures it with any new data, and writes it back out as well. Ah well. This patch doesn't really look any different than the previous one, aside from a missing gtk_label_set_label () call, and a new call to turn off some label inherent in the preview widget API. I'm applying the patch and will regenerate a new one with a lot less code, shortly.
Created attachment 35741 [details] [review] Simpler patch for preview widget Here's the patch I'm going to commit to CVS for the 2.10 feature freeze. If we can get a standard widget created and in libgnomeui for 2.12 then we can switch to use it then. Until then, this is it.
> All of the parsing to get metadata out of the png headers is useless unless the > image was already in the capplet at some point, since nothing else that I know Ok, I was not sure about this part, I just kept if from the previous version of the patch. > Ah well. This patch > doesn't really look any different than the previous one, aside from a missing > gtk_label_set_label () call, and a new call to turn off some label inherent in > the preview widget API. I'm applying the patch and will regenerate a new one > with a lot less code, shortly. No, as said before it's based on one of the previous one. The main change was to set a stock icon for non-picture files and fix the size to 128 to avoid the flicking effect. > Here's the patch I'm going to commit to CVS for the 2.10 feature freeze. nice, thanks. BTW do you want to add some informations like the size/the dimensions of the picture ? Just to know if that's worth working on a patch to do this.
OK. I've committed the patch. Marking this as Fixed. > BTW do you want to add some informations like the size/the dimensions of the > picture ? Just to know if that's worth working on a patch to do this. Nope. Not for 2.10 anyway. Let's figure out a standardized preview widget for 2.12 separate from this bug, and then get it into libgnomeui and fix the capplet to use it then. I don't think we need to put that much in the preview right now.