GNOME Bugzilla – Bug 336973
[eog-ng] Migration of the collection pane to GtkIconView and GtkListStore
Last modified: 2006-10-05 23:32:06 UTC
According with the comments on [1], I paste here the first version of the EogThumbView and EogListStore implementation, more some needed glue to port EOG to it. The first version compiles cleanly, removes the dependence on EogWrapList and EogImageList, but it still have some issues, and crashes, ie. when entering to fullscreen mode. I attach the files here, so I can get comments and a first review, before committing to the eog-ng branch. The crash in the fullscreen mode it is because I am using GtkTreeIter's to keep reference to items in the store, and I think that I should switch to GtkTreePaths. I will play with it during the next week. I also rewrote the directory monitoring code, so now the EogThumbView widget gets notified when images are added, removed, or changed (EogWrapList only gets notified with images added and removed). The file test-eog-tb.c it is a little test for the code. It should be called with a list of uris as parameters (in the same way eog is called). This is my first *big* patch to eog, so there could be a lot of conceptual mistakes. I will appreciate every comment a lot. http://live.gnome.org/EyeOfGnome/PlaceForNewIdeas#head-a1c0714d67bfd06f7002187503f4e3828f3b4cd1
Created attachment 62637 [details] libeog/eog-thumb-view.c
Created attachment 62638 [details] libeog/eog-thumb-view.h
Created attachment 62639 [details] libeog/eog-list-store.c
Created attachment 62640 [details] libeog/eog-list-store.h
Created attachment 62642 [details] libeog/test-eog-tb.c
Created attachment 62643 [details] [review] big, big, glue
Hey Claudio, this is a very nice initial work on migration to GtkIconView. Of course, there are several issues that can be improved. However, the code is reasonably good. The really critical bugs I detected were the already mentioned fullscreen crash and the initial image is not getting set (eog starts with no image loaded). So, I suggest you to commit it on eog-ng branch and keep improving/fixing the migration code there. I'm near to commit some *heavy* refactoring on eog-ng code and it would be good to have this new code already there, ok? Thanks dude!
Lucas, I commited to eog-ng branch. I will work during the week on the critical things, any other bug that arises, please report it here or in a new bug. Thank you!
(In reply to comment #7) > The really critical bugs I detected were the already mentioned > fullscreen crash and the initial image is not getting set (eog starts with no > image loaded). Now the initial image is being set. That is fixed in CVS.
Claudio, three litle issues: 1. Make the view circular: when last item is selected, "Next Image" goes to the first item; 2. Coding style: have a look at eog-application.[hc] - which use the CS that can be considered "official" from now on. 3. I'd like to experiment applying the iconview a one-row layout with just a horizontal scrollbar and a fixed height.
(In reply to comment #10) > 2. Coding style: have a look at eog-application.[hc] - which use the CS that > can be considered "official" from now on. Done. > 3. I'd like to experiment applying the iconview a one-row layout with just a > horizontal scrollbar and a fixed height. Done. It still uses too much space, IMHO, but that can be easily fixed (the icons are way too much bigger than in EogWrapList, I still wonder the right way to resize the thumbs, will check it tomorrow).
(In reply to comment #10) > Claudio, three litle issues: > > 1. Make the view circular: when last item is selected, "Next Image" goes to the > first item; Done. (In reply to comment #11) > Done. It still uses too much space, IMHO, but that can be easily fixed (the > icons are way too much bigger than in EogWrapList, I still wonder the right way > to resize the thumbs, will check it tomorrow). Done. Lucas, what are your impressions of the one row layout? The captions of the filenames use too much space, IMHO. I will check how to fix that tomorrow.
Hey Claudio, some additional issues: 1. The collection should have a fixed height and should not change when window is resized. (Note: the current window implementation is incomplete so the current code makes the collection pane dimensions don't work well). 2. Thumbnails are too large. The "normal" dimensions of thumbs are 128x128. The old code resizes the thumbs to 96x96 which is a better choice. See libeog/eog-collection-item.c to see the code you can reuse for this task. 3. Add some eyecandiness to thumbnails by adding a rounded-shadow border like ShowImg does (See http://www.jalix.org/projects/showimg/) 4. Move thumbnail loading into a job. Thumbnailing should run in a separate thread. See libeog/eog-jobs.[ch]. The code skeleton is already there. Just fill it with the code. shell/eog-window.c has some examples of how to use eog's new job queue. 5. Don't load thumbnails that are not visible in collection pane. I'd like to only load thumbnails that are actually visible in collection pane. See evince code (shell/ev-sidebar-thumbnails.c) as a reference.
Claudio, just to let you know, bug #338603 and bug #338608 are very related to 1.
Lucas, There seems to be problems with the thumbnail job handling. I moved the thumbs loading to the job as you suggested, but after loading the thumbnails the app stays blocked, as if there were still jobs pending. I won't commit it, but paste here the patch. Can you please take a look at it? I think I understood right the rationale of the job queue, but I may be wrong. Thank you.
Created attachment 63715 [details] [review] move the thumbnailing to a job
Created attachment 63800 [details] [review] Patch to add a dropped shadow to the images. This patch adds a dropped shadow to the thumbnails, much in the way nautilus does, but not exactly the same. I took the code from gnome-utils, I like the way it looks. What do you think?
Claudio, some eye candy issues: 1. Coding style: make it compliant with eog's. 2. Add the correct copyright header like you did with eog-list-store, for example. 3. I still would like to see something more like Showimg: rounded border plus drop shadow. Also, It'd be good to add some padding :-)
(In reply to comment #18) > 3. I still would like to see something more like Showimg: rounded border plus > drop shadow. Also, It'd be good to add some padding :-) Lucas, I coded a little more and get what you want, but I have to be honest and say that I don't really like it. I find the style too away from the GNOME, and would prefeer something more close to the nautilus style, for example. Here are some screenshots: http://www.gnome.org/~csaavedr/images/test-thumb-view-clean.png http://www.gnome.org/~csaavedr/images/test-thumb-view-line.png [I like this one] http://www.gnome.org/~csaavedr/images/test-thumb-view-rounded.png
Marking attachment 63800 [details] [review] as obsolete, as I already commited more recent eye candy code.
Looks really nice so far.:) Only two things I found: 1. The GError* in eog_window_construct_ui is not initialized to NULL before using it. This can lead to strange error messages, if eog is not installed correctly: (eog:18812): GLib-WARNING **: GError set over the top of a previous GError or uninitialized memory. This indicates a bug in someone's code. You must ensure an error is NULL before it's set. The overwriting error message was: Failed to open file '/home/felix/eog-ng-test/share/eog/eog-ui.xml': No such file or directory ** (eog:18812): WARNING **: building menus failed: `\x8a\u0005\u0008HX\u000b\u0008 2. The collection pane needs a minimum size. At the moment when the collection pane is enabled in the menu and I open a picture for the first time (File->Open or command line) it's so small that it's unusable. I need to disable and reenable it to be able to use it. Btw, that rounded-border-example looks kinda strange, possibly because the border is white and practically disappears in the top left image area. How about using the border style from the second image (that's the one used now I think) and only round the corners (without making the border thicker)?
(In reply to comment #21) > Looks really nice so far.:) Thank you :-) > Only two things I found: > > 1. The GError* in eog_window_construct_ui is not initialized to NULL before > using it. This can lead to strange error messages, if eog is not installed > correctly: > (eog:18812): GLib-WARNING **: GError set over the top of a previous GError or > uninitialized memory. > This indicates a bug in someone's code. You must ensure an error is NULL before > it's set. > The overwriting error message was: Failed to open file > '/home/felix/eog-ng-test/share/eog/eog-ui.xml': No such file or directory > > ** (eog:18812): WARNING **: building menus failed: > `\x8a\u0005\u0008HX\u000b\u0008 > This is already fixed in CVS. :-) > 2. The collection pane needs a minimum size. At the moment when the collection > pane is enabled in the menu and I open a picture for the first time (File->Open > or command line) it's so small that it's unusable. I need to disable and > reenable it to be able to use it. I did some tweaking to the default values of the pane. Now it should look much better. > > Btw, that rounded-border-example looks kinda strange, possibly because the > border is white and practically disappears in the top left image area. How > about using the border style from the second image (that's the one used now I > think) and only round the corners (without making the border thicker)? > We discussed with Lucas about it, and decided to go for the one that is the default now. However, if you have spicy ideas, feel free to make mockups and/or hack libeog/eog-thumb-shadow.[ch], where the shadows are created.
Created attachment 65512 [details] [review] Implementation of lazy thumbnail loading. This is a implementation of a lazy thumbnail loading system. It works by loading only the thumbnails that are visible on the pane, and unloading from memory those that are no longer visible. It makes EOG load very quick, even in directories with large number of images, and also saves some memory. The alignment of the images change when the thumbs are not loaded. I guess this may be fixed by tweaking the IconView visual properties.
Created attachment 65514 [details] [review] 2nd implementation 2nd version. We don't really to store the scrolled window and the adjustment in the private data of EogThumbView.
Commited attachment 65514 [details] [review] with a few changes: - Connecting also the signal "value-changed" for the vertical adjustment of the EogThumbView widget (in case somebody wants to use the widget with both horizontal and vertical adjustments). - Instead of using g_new0 to create the private data, use EOG_THUMB_VIEW_GET_PRIVATE. - Removed the EOG_THUMB_VIEW_THUMB_SET column from the model. It is not really needed -- if properly set, the EOG_THUMB_VIEW_THUMBNAIL column can serve for the same purpose.
How about setting the invisible thumbnails to a generic image icon. Possible icons could be "gnome-mime-image". That way it wouldn't look "broken" if EOG lags behind reloading the thumbs. This could also be the default icon for thumbnails which haven't yet been loaded from disk.
(In reply to comment #26) > How about setting the invisible thumbnails to a generic image icon. Possible > icons could be "gnome-mime-image". That way it wouldn't look "broken" if EOG > lags behind reloading the thumbs. This could also be the default icon for > thumbnails which haven't yet been loaded from disk. > Agreed. I wanted to do it but I haven't had time for it. There is an icon that is used with the previous implementation, it is still in the branch (eog/art/loading.png). I want to use that one, but haven't had time to implement it.
So the implementation would change in the following way: - We would actually need the EOG_THUMB_VIEW_THUMB_SET column in the model. - When adding a image (in eog_list_store_append_image ()) set the thumbnail to the default one, and the EOG_THUMB_VIEW_THUMB_SET column to FALSE. - When finally setting the thumbnail (eog_job_thumbnail_cb ()), change the EOG_THUMB_VIEW_THUMB_SET value to TRUE. The opposite in eog_list_store_thumbnail_unset () for the unsetting case. - When unsetting the thumbnail (eog_list_store_thumbnail_unset ()), instead of setting it to NULL, pass the default one. - Do the checking about a thumbnail already being set using the EOG_THUMB_VIEW_THUMB_SET column instead of getting the thumbnail and checking if it is NULL or not. So Felix do you want to go for it? ;-)
Created attachment 65847 [details] [review] implement busy pixbuf Okay, this should add the busy pixbuf. Based on your instructions and the old loading code from eog-collection-item.c. Two things I am not so sure about: 1. Is it okay to add the busy pixbuf to the ListStore with out reffing it? 2. Is it okay to store the pixbuf in the ListStores private data?
Oh, I think the pixbuf needs to be readded to the Makefile.
Felix, I reviewed your patch. It is very good. I only did some format cleaning, removed some unnecesary checks in eog_list_store_{set,unset}_thumb, and added the image file to Makefile.am. Regarding your questions, > 1. Is it okay to add the busy pixbuf to the ListStore with out reffing it? Yes, because the model does the ref'ing when you add it, and the unref'ing when you replace it. > 2. Is it okay to store the pixbuf in the ListStores private data? Looks fine to me, it is a good place to store it. There is only one aditional issue (not really important, though). If you test libeog/test-thumb-view, it won't load the pixbuf, and will give some warnings. That is because the minimalist test code has no idea where to get the image file. If you could only review that, it would be great. I applied your patch, many thanks!
Created attachment 65853 [details] [review] update list-view-test for busy pixbuf (In reply to comment #31) > > > There is only one aditional issue (not really important, though). If you test > libeog/test-thumb-view, it won't load the pixbuf, and will give some warnings. > That is because the minimalist test code has no idea where to get the image > file. If you could only review that, it would be great. > > I applied your patch, many thanks! > This updates the test to initialize libgnome. But we should replace the loading code by something better anyway. I'll have to check out if it's possible to generate the path in the same way it's done for the UIManager xml.
Created attachment 65884 [details] [review] hardcode busy_pixbuf This one hardcodes the pixbuf path and thus removes gnome_locate_file(). The test-app should work without changes.
Thank you Felix, I commited your patch. The last remaining issue, would be to have an icon whose height is the same than the EOG_LIST_STORE_THUMB_SIZE define, but as long as we don't define precisely the size of the thumbnails, it doesn't worth to work on that.
(In reply to comment #34) > The last remaining issue, would be to have an icon whose height is the same > than the EOG_LIST_STORE_THUMB_SIZE define, but as long as we don't define > precisely the size of the thumbnails, it doesn't worth to work on that. I commited now code to simply use the "image-loading" icon, that comes with gnome-icon-theme. This way we don't need to ship an aditional icon and rely in this themeable icon. Still to do (mostly a personal reminder): - If a image is added to a directory while eog is monitoring that directory, and its thumbnail gets visible just because this image name is in the visible range, the icon doesn't get loaded. To do: get sure that the images added this way to the list store gets its thumbnail loaded when they have to. (if this confuses you, try the following: $ eog ~/ & $ cp some-image.jpg ~/0.jpg and see how the new 0.jpg doesn't have a thumb in the pane.) - When the thumb view is hidden, the icons are not loaded in the images. Therefore, when switching between images, the application icon goes to the default icon instead of a preview of the image. The problem here is as follows: If we let the icons loaded in the EogImages all the time, then there is no sense on unloading the icons from the icon view, as they will have still a ref_count = 1, and its memory won't be freed. To do: think about a smart way of getting sure that the thumb of the current image is always loaded, even when the collection pane is hidden.
Also have a look at the bug #341703 for remote images thumbnailing, ok? The current on-demand thumbnail loading is totally bad for remove images.
Created attachment 68154 [details] [review] patch to maintain in the thumbnail queue only jobs that are really needed This patch checks that scheduled jobs for loading thumbnails that are no longer needed are removed from the queue. This improves performance heavily, because we will only have in the thumbnailing queue the jobs that are really needed. This improves the performance over slow networks. A big part of the code (specially modifications in libeog/eog-job-queue.c) is only for debugging purposes. Comments?
I just commited the following to eog-ng: 2006-10-05 Felix Riemann * libeog/eog-image.c: (eog_image_class_init), (eog_image_set_thumbnail): * libeog/eog-image.h: Replace old unused thumbnail signals by a single one being emitted once the thumbnail is set. * shell/eog-window.c: (image_thumb_changed_cb), (eog_window_display_image): Set window icon using the new signal. Make it work when the collection view is hidden. This should set the window icon correctly and should make sure a thumbnail is loaded for the currently loaded image even when the collection view is not displayed. Interestingly the collection view still seems to see ~2 files in a visible range (tb_on_visible_range_changed_cb() get's called while clicking through the files) although it's hidden.
(In reply to comment #38) > Interestingly the collection view still seems to see ~2 files in a visible > range (tb_on_visible_range_changed_cb() get's called while clicking through the > files) although it's hidden. Do you mean while switching images when the collection pane is hidden? I think that happens, because in eog_thumb_view_select_single () (the function used to switch the visible image) we scroll the pane although it is hidden, triggering the callback for the adjustment for the "range-changed" signal.
I just commited this: (Though I am not 100% happy with the network performance, because sometimes, even when the job for certain image has been cancelled, the gnome-vfs functions keep on getting the info of it.) 2006-10-05 Claudio Saavedra <csaavedra@alumnos.utalca.cl> - Store EogJobs to load thumbnails in the EogListStore model, so we can easily cancel them when they are no longer needed. This improves network performance, though it is not optimal yet. - Move the code to add eye candy to thumbnails inside the thumbnailing job. This improves responsiveness. * libeog/eog-jobs.c: (eog_job_thumbnail_run): Move the code to add eye candy to thumbnails inside the thumbnailing job. * libeog/eog-list-store.c: + New mutex element in structure, to make jobs storing/removing operations atomic. (eog_list_store_dispose): Free the mutex. (eog_list_store_init): Initialize the mutex. (eog_job_thumbnail_cb), (eog_list_store_thumbnail_set), (eog_list_store_thumbnail_unset): Removed eye candy code. Store/remove thumbnailing jobs in the model. * libeog/eog-list-store.h: Add EOG_LIST_STORE_EOG_JOB column. * libeog/eog-list-store.c: (eog_list_store_init): Add a column to store the EogJobs. (eog_job_thumbnail_cb): Once the EogJob is finished, remove it from the model. (eog_list_store_thumbnail_set): When scheduling the job, save it to the model. (eog_list_store_thumbnail_unset): If current scheduled job is still not complete, cancel it. * libeog/eog-list-store.h: Add a column to store the EogJob that loads the thumbnail.
I think the transition is ready. This is a good time to close this bug. If there are still more issues to solve, we can fill individual bugs to keep a better track of them. Thank you very much Felix and Lucas for your feedback and code!