Merge lp:~vikoadi/pantheon-files/fix-1041633 into lp:~elementary-apps/pantheon-files/trunk

[Viko Adi Rahmawan]

Update thumbnails after file changes. Fixes lp:1041633.

This code removes outdated icons from the cache in icon_info when a file deletion or change occurs.

clear thumbnail cache in icon_info on every file deletion and changes

[Danielle Foré (danrabbit)]

This works great for me. The effect is instantaneous!

[Sergey "Shnatsel" Davidoff (shnatsel)]

I can't tell from the code if it deletes the thumbnail on every file change. If so, we've tried that approach already and gave up on it because it yeilds 100% CPU load on continuously written files (e.g. large downloads).

Instead, inotify provides a "close_write" event, which is emitted when *closing* a file that was written to. Please use such an event instead.

review: Needs Information

[Viko Adi Rahmawan (vikoadi)]

i think you misunderstood the cache cleaning here. the cache is only in memory (see it in marlin-icon-info.c) not the thumbnail file in .thumbnail that is being managed by tumbler.
before this cache cleaning, thumbnail file is already generated by tumbler but the cache in marlin-icon-info is not updated because because there is already with same name.
i dont know whether it will cause 100% load i will checked it later, because right know i just call the cleaning function from fm-directory-view, (just before tumbler update get requested). if it cause overload, ill try to move it to close_write

[Viko Adi Rahmawan (vikoadi)]

Okay so the cache cleaning is only done in every FileMonitorEvent.CHANGES_DONE_HINT. it should not be as often as FileMonitor.changed but it wouldnot be as good as inotify close_write one. (I still dont understand how CHANGES_DONE_HINT supposed to work).
I dont think its worth it to convert FileMonitor to inotify, and i have test it and its not overload the CPU in my i3 2GB-ram laptop.

[Victor Martinez (victored)]

Make sure 'icon_file' and 'icon' are freed before returning from 'remove_all_size_cache'. You only need to call g_object_unref:

[...]

ret_val = g_icon_equal (lookup_key->icon, icon);

g_object_unref (icon_file);
g_object_unref (icon);

return ret_val;

[...]

Regarding the implementation, traversing the _whole_ icon Hash Table every time a thumbnail update occurs just to determine whether an item should be removed is too inefficient. Please consider that 'reap_cache' is already doing that to free cache elements older than 30 secs. If we wanted linear access time we'd have used a linked list from the beginning.

You can lookup the GFileIcon you want directly in constant time from marlin_icon_info_remove_updated_icon:

MarlinIconInfo *icon_info;
LoadableIconKey lookup_key;

lookup_key.icon = icon;

/* desired icon size should be requested as parameter */
lookup_key.size = size;

/* if the underlying GFileIcon file is the same, this lookup will return the icon info corresponding to icon->file */
icon_info = g_hash_table_lookup (loadable_icon_cache, &lookup_key);

if (icon_info != NULL)
    g_hash_table_remove (loadable_icon_cache, &lookup_key);

Avoid 'g_hash_table_foreach_remove' at all costs, and make sure the code style is consistent with the rest of the file.

review: Needs Fixing

[lp:~vikoadi/pantheon-files/fix-1041633]

1469. By Viko Adi Rahmawan on 2014-05-07

dont iterate through all hast table

1470. By Viko Adi Rahmawan on 2014-05-07

iterate through all ZOOM_LEVEL

[Viko Adi Rahmawan (vikoadi)]

everythings changed.
no more iterating through hash table

[Sergey "Shnatsel" Davidoff (shnatsel)]

review: Abstain

[lp:~vikoadi/pantheon-files/fix-1041633]

1471. By Viko Adi Rahmawan on 2014-05-07

remove uneeded include, fix style

[Victor Martinez (victored)]

Thanks for your great work!

Some suggestions:

marlin_icon_info_remove_cache: it's OK to try to remove things that don't exist in the Hash Table. So the function can call 'g_hash_table_remove' directly without calling 'marlin_icon_info_lookup' first. 'icon_info' is not necessary either.

Since the code is using 'loadable_icon_key_new' we need to call 'loadable_icon_key_free' after using the 'lookup_key'. Otherwise the memory for 'lookup_key' and 'lookup_key->icon' is leaked.

fm-directory-view.c:

Duplicate code should be merged in a single function.

Coding style doesn't try to match the rest of the file. This is important. You'll notice opening braces go on a separate line.

[lp:~vikoadi/pantheon-files/fix-1041633]

1472. By Viko Adi Rahmawan on 2014-05-08

combine duplicated code, code style cleaning, no nore marlin_icon_info_lookup, memory freeing

[Viko Adi Rahmawan (vikoadi)]

sorry for bothering alot, im not familiar with c and memory management, but this bug is really annoy me.
would love to see more suggestions.

[Viko Adi Rahmawan (vikoadi)]

why its alwasy "\ No newline at end of file" everytime i changed file? i use Scratch
and should i just commit to fix conlicts?

[Victor Martinez (victored)]

This looks great!

It appears to work for me, after solving the merge conflicts.

Using Scratch:

1) Delete diff lines 80 and 81
2) Diff line 88 should use "const char* path = ..." instead of "char* path = ..."
3) Make sure diff line 98 only has a single character: '}'

Suggestions:

'remove_marlin_icon_info_cache' should be 'static void' so that it's private to 'fm-directory-view.c'.

Commit the changes with bzr.

Merge trunk again with 'bzr merge'

Push the changes here :)

[Victor Martinez (victored)]

I agree it's kind of annoying Scratch always changes the EOF character. Gedit seems to keep the one in the original file before saving. Not a big deal for us though. We prefer Scratch anytime.

[lp:~vikoadi/pantheon-files/fix-1041633]

1473. By Viko Adi Rahmawan on 2014-05-10

use static void remove_marlin_icon_info_cache

1474. By Viko Adi Rahmawan on 2014-05-10

merge with trunk

[Victor Martinez (victored)]

Great work Viko!

The bugs described in the bug report have indeed been fixed, even after changing the zoom level :D

I can confirm this fix doesn't introduce a performance penalty.

review: Approve

=== modified file 'libcore/marlin-icon-info.c'
--- libcore/marlin-icon-info.c	2014-03-12 23:46:52 +0000
+++ libcore/marlin-icon-info.c	2014-05-10 16:08:52 +0000
@@ -730,3 +730,19 @@
     }
 }
 
+void marlin_icon_info_remove_cache (const char *path, int size)
+{
+    GFile *icon_file;
+    GIcon *icon;
+    LoadableIconKey *lookup_key;
+    
+    icon_file = g_file_new_for_path (path);
+    icon = g_file_icon_new (icon_file);
+    lookup_key = loadable_icon_key_new (icon, size);
+    
+    g_hash_table_remove (loadable_icon_cache, lookup_key);
+    
+    g_object_unref (icon_file);
+    g_object_unref (icon);
+    loadable_icon_key_free (lookup_key);
+}

=== modified file 'libcore/marlin-icon-info.h'
--- libcore/marlin-icon-info.h	2013-08-10 20:20:23 +0000
+++ libcore/marlin-icon-info.h	2014-05-10 16:08:52 +0000
@@ -65,6 +65,7 @@
 
 void                marlin_icon_info_clear_caches               (void);
 void                marlin_icon_info_infos_caches               (void);
+void                marlin_icon_info_remove_cache               (const char *path, int size);
 
 G_END_DECLS
 

=== modified file 'src/fm-directory-view.c'
--- src/fm-directory-view.c	2014-04-18 00:56:20 +0000
+++ src/fm-directory-view.c	2014-05-10 16:08:52 +0000
@@ -213,6 +213,8 @@
 
 void            dir_action_set_sensitive (FMDirectoryView *view, const gchar *action_name, gboolean sensitive);
 
+static void 	remove_marlin_icon_info_cache (GOFFile *file);
+
 G_DEFINE_TYPE (FMDirectoryView, fm_directory_view, GTK_TYPE_SCROLLED_WINDOW);
 #define parent_class fm_directory_view_parent_class
 
@@ -289,6 +291,8 @@
     g_return_if_fail (file->exists);
 
     g_debug ("%s %s %d\n", G_STRFUNC, file->uri, file->flags);
+    //remove thumbnail cache so new thumbnail would be generated
+    remove_marlin_icon_info_cache (file);
     fm_list_model_file_changed (view->model, file, directory);
     guint id;
     marlin_thumbnailer_queue_file (view->details->thumbnailer, file, &id, /* large */ FALSE);
@@ -298,6 +302,8 @@
 file_deleted_callback (GOFDirectoryAsync *directory, GOFFile *file, FMDirectoryView *view)
 {
     g_debug ("%s %s", G_STRFUNC, file->uri);
+    //remove thumbnail cache so new thumbnail would be generated
+    remove_marlin_icon_info_cache (file);
     fm_list_model_remove_file (view->model, file, directory);
     /* Remove from gof-directory-async cache */
     if (gof_file_is_folder (file)) {
@@ -4031,4 +4037,20 @@
     //view->details->templates_invalid = TRUE;
 
     update_menus (view);
-}
\ No newline at end of file
+}
+
+static void 
+remove_marlin_icon_info_cache (GOFFile *file)
+{
+    if (gof_file_get_thumbnail_path (file) != NULL){
+        const char* path = gof_file_get_thumbnail_path (file);
+        MarlinZoomLevel zoom_level;
+        for (zoom_level = MARLIN_ZOOM_LEVEL_SMALLEST;
+             zoom_level <= MARLIN_ZOOM_LEVEL_LARGEST;
+             zoom_level++)
+             {
+                int icon_size = marlin_zoom_level_to_icon_size (zoom_level);
+                marlin_icon_info_remove_cache (path, icon_size);
+             }
+    }
+}