**** BEGIN LOGGING AT Wed Feb 22 16:01:15 2006

Feb 22 16:01:31 <mclasen>	hey, do we have federico ?
Feb 22 16:07:33 <federico>	la la la
Feb 22 16:07:45 <mclasen>	we have federico !
Feb 22 16:08:13 <mclasen>	now if rambokid was here, too...
Feb 22 16:08:36 <kris>	i am here
Feb 22 16:08:40 <mclasen>	federico: one filechooser related question I had 
Feb 22 16:08:47 <federico>	mclasen: what's up?
Feb 22 16:09:06 <mclasen>	is if we have any plan for handling unreadable directories and similar error conditions
Feb 22 16:09:28 <mclasen>	currently it seems we silently display an unreadable directory like an empty one
Feb 22 16:09:34 <federico>	mclasen: if we don't handle them correctly right now, it's a bug
Feb 22 16:10:00 <federico>	mclasen: ok, that's the intended behavior (we shoudl probably display "this folder is unreadable" in the list or something)
Feb 22 16:10:26 <kris>	federico: right now you usually get an error dialog when _get_folder() reports an error
Feb 22 16:10:30 <federico>	mclasen: the idea is that for --x directories, you can type a full filename in the location entry, even if you can't see the folder contents.
Feb 22 16:10:33 <kris>	(which is the case for the current gtk+ backend)
Feb 22 16:10:52 <kris>	but for the gnome-vfs backend, get_folder doesn't report an error, but the async loading code does detect one but can't report it
Feb 22 16:11:11 <mclasen>	federico: it would certainly be nice to decorate the folder icon with an emblem to mark it as unreadable, too
Feb 22 16:13:30 <mclasen>	kris: should get_folder by fixed to report the error, or do we need some way to report the error asynchronously ?
Feb 22 16:13:52 <federico>	mclasen: oooh, the icon would be nice
Feb 22 16:13:55 <federico>	mclasen: file a bug?
Feb 22 16:14:10 <mclasen>	ok
Feb 22 16:14:22 <federico>	so:
Feb 22 16:14:27 <kris>	mclasen: looking at the new _get_folder() behaviour in the async branch I  think it would be good to have a way to be able to asynchronously report the errors
Feb 22 16:14:35 <kris>	s/the errors/errors/
Feb 22 16:14:37 <federico>	1. kris's async patch should tell us "the folder is not readable" in the async callback
Feb 22 16:15:03 <federico>	2. we should still be able to do gtk_file_system_get_info("/file/in/unreadable/folder/foo.txt") and the async callback should report success
Feb 22 16:15:34 <federico>	3. the location entry code should be able to handle (2)
Feb 22 16:15:40 <kris>	we can also do 1. instead of my suggestion
Feb 22 16:15:50 <federico>	kris: I'm reviewing your patch.  I wanted to do it yesterday, but novell put me to fight fires :)
Feb 22 16:15:58 <federico>	kris: want my comments so far?
Feb 22 16:16:34 ---	ebassi|dinner is now known as ebassi|out
Feb 22 16:16:43 <kris>	I am not sure if I tackled 2/3 already
Feb 22 16:17:04 <kris>	federico: comments are always welcome ;)
Feb 22 16:18:25 <mclasen>	federico: yes, share your comments with us
Feb 22 16:21:22 <federico>	okay
Feb 22 16:21:39 <federico>	- gtkfilesystem.c
Feb 22 16:21:39 <federico>	  + The cache of rendered icons gets attached to the GtkIconTheme
Feb 22 16:21:39 <federico>	    (gtkfilesystem.c:get_cached_icon()).  This means that the icons
Feb 22 16:21:39 <federico>	    use memory all the time after a file chooser has been opened.  Can
Feb 22 16:21:39 <federico>	    we have instead gtk_file_info_get_icon_name(), and put the cache
Feb 22 16:21:39 <federico>	    in the caller?  We can have a cache-object that is shared by the
Feb 22 16:21:41 <federico>	    file system model, the shortcuts pane, the path bar, etc.
Feb 22 16:23:25 <federico>	... and then the file system implementations can be pretty dumb about icons
Feb 22 16:23:43 <federico>	  + gtk_file_system_unix_get_folder() creates a GtkFileFolderUnix and
Feb 22 16:23:43 <federico>	    queues an idle for load_folder().  It never removes this idle.  If
Feb 22 16:23:43 <federico>	    the folder is unreffed/finalized before we hit the idle loop,
Feb 22 16:23:43 <federico>	    we'll crash in load_folder().
Feb 22 16:23:50 <kris>	The caching which happens in gtkfilesystem.c right now is basically the same what happened in the old backends
Feb 22 16:23:58 <kris>	but the backends have their own cache for the volume icons ...
Feb 22 16:25:00 <kris>	how do you want to put the cache in the caller?
Feb 22 16:25:11 <kris>	wouldn't that make things in the file chooser dialog implementation overly complex?
Feb 22 16:26:30 <federico>	we should have volume_get_icon_name() as well, then
Feb 22 16:26:43 <federico>	ok, just keep this in mind.  We can move the cache upwards later.
Feb 22 16:26:46 <federico>	next:
Feb 22 16:26:58 <kris>	the second thing is just a bug ;)
Feb 22 16:27:02 <federico>	okay
Feb 22 16:27:04 <federico>	next:
Feb 22 16:27:14 <federico>	in gtkfilesystemunix
Feb 22 16:27:31 <federico>	you have dispatch_get_info_callback(), where the caller must *not* free the file_info
Feb 22 16:27:41 <mclasen>	why do we need to cache icons in the first place ?
Feb 22 16:27:49 <federico>	you have dispatch_get_folder_callback(), where the caller *must* unref the folder object
Feb 22 16:28:16 <federico>	I don't like that disparity.  All such functions should give the caller's callback something that it *must* unref/free.
Feb 22 16:28:55 <federico>	mclasen: so that we can ref a single instance of each icon instead of having many copies
Feb 22 16:29:18 <kris>	hrm, I think it makes more sense that the dispatch routine would unref/free all stuff it passed to the callback when the callback returns
Feb 22 16:29:29 <kris>	and if the receiver of the callback wants to use that data later on it would ref/copy it
Feb 22 16:29:43 <mclasen>	federico: but with the icon cache, we should only have a single instance of each icon anyway, right ? unless they're svg or scaled... 
Feb 22 16:30:53 <kris>	but we probably need to match here what the rest of gtk+ does
Feb 22 16:33:30 <federico>	kris: that's another possibility.  In any case, it should be consistent across callbacks.
Feb 22 16:33:43 <kris>	yep
Feb 22 16:34:00 <federico>	mclasen: yeah, and it looks like our goddamn artists want svg for everything :(
Feb 22 16:34:27 <mclasen>	federico: I need to bring that up on the themes list. That seriously defeats our icon caching
Feb 22 16:34:29 <federico>	okay, next.
Feb 22 16:34:37 <federico>	kris: the unix backend blocks.  do you care about it?
Feb 22 16:34:48 <mclasen>	federico: maybe we need to make gtk-update-icon-cache render the svgs at common sizes ?
Feb 22 16:34:49 <federico>	i.e. gtk_file_system_unix_get_info() does the stat()s right there.
Feb 22 16:35:37 <kris>	My reasoning was that it would be pretty hard to support try asyncness in the unix backend, so it would just not be truly async and just implement the new api
Feb 22 16:35:41 <federico>	mclasen: that would be pretty cool, actually
Feb 22 16:35:45 <kris>	for true asyncness one would have to use the gnome-vfs backend
Feb 22 16:36:01 <federico>	mclasen: I'd like to know how often we hit the icon cache, and how often we don't because someone requested a weird size.
Feb 22 16:36:15 <federico>	since GtkIconSize never has what you need e.g. for a list view
Feb 22 16:36:29 <mclasen>	federico: I'll try to collect some data 
Feb 22 16:36:29 <federico>	kris: ok
Feb 22 16:36:34 <federico>	mclasen: awesome, thanks
Feb 22 16:38:21 <federico>	kris: next
Feb 22 16:38:35 <federico>	  + In gtk_file_system_unix_get_info():
Feb 22 16:38:35 <federico>		+  /* Get info for "/" */
Feb 22 16:38:35 <federico>		+  if (!path)
Feb 22 16:38:35 <federico>		+    {
Feb 22 16:38:35 <federico>		+      info = file_info_for_root_with_error ("/", &error);
Feb 22 16:38:36 <federico>		+
Feb 22 16:38:38 <federico>		+      g_object_ref (handle);
Feb 22 16:38:40 <federico>		+      queue_get_info_callback (callback, handle, info, error, data);
Feb 22 16:38:42 <federico>		+
Feb 22 16:38:44 <federico>		+      return handle;
Feb 22 16:38:46 <federico>		+    }
Feb 22 16:38:48 <federico>	        I hate to change my mind all the time, but now that get_info()
Feb 22 16:38:50 <federico>	        gets a path rather than a folder *and* a subpath, can we make
Feb 22 16:38:52 <federico>	        path==NULL be an error?  If the caller wants info about "/",
Feb 22 16:38:54 <federico>	        it should pass exactly that path.
Feb 22 16:39:21 <kris>	oh, i would really love to make path==NULL an error
Feb 22 16:39:42 <kris>	IIRC the gtk+ and gnome-vfs backends had different ideas about handling path==NULL, which was sort of confusing
Feb 22 16:41:59 <federico>	if they do, that's a big bug :)
Feb 22 16:42:13 <federico>	just make it an error.  If the caller wants info about the root, have it pass the root URI.
Feb 22 16:43:35 <kris>	ok
Feb 22 16:44:50 <federico>	you leak the handles in a few of the caller callbacks when you exit prematurely from the callback
Feb 22 16:48:18 <kris>	oh, yeah
Feb 22 16:48:20 <kris>	missing unrefs?
Feb 22 16:48:47 <federico>	yup
Feb 22 16:48:58 <federico>	which leads me to...
Feb 22 16:49:21 <federico>	should the caller really get new references for the handles?
Feb 22 16:50:54 <mclasen>	ok, need to leave you now. Keep up the nice review...
Feb 22 16:51:25 <kris>	not sure;
Feb 22 16:51:30 <mclasen>	one final note, I'll probably do stable releases Friday night or sometime on Monday
Feb 22 16:51:41 <kris>	maybe we should just make it consistent with whatever we decide on the callback arguments (as discussed above)
Feb 22 16:51:57 <mclasen>	and it'll probably be Glib 2.10.0, so if you have any things in GLib to fix before that, let me know
Feb 22 16:52:10 <kris>	wow, 10.0 already
Feb 22 16:57:29 <kris>	but not reffing the handle for a callback will probably make the callback code easier
Feb 22 17:01:53 <federico>	kris: ok, next
Feb 22 17:02:13 <federico>	  + In create_file_info():
Feb 22 17:02:13 <federico>		+  if (types & GTK_FILE_INFO_ICON)
Feb 22 17:02:13 <federico>		+    {
Feb 22 17:02:13 <federico>		+      IconType icon_type;
Feb 22 17:02:13 <federico>		+      char *icon_name;
Feb 22 17:02:14 <federico>		+      const char *mime_type;
Feb 22 17:02:15 <federico>		+
Feb 22 17:02:18 <federico>		+      icon_type = get_icon_type_from_path (folder_unix, filename, &mime_type);
Feb 22 17:02:20 <federico>		The passed folder_unix can be NULL since you call
Feb 22 17:02:22 <federico>		create_file_info() with a NULL folder in some places.
Feb 22 17:02:24 <federico>		Downstream, get_icon_type_from_path() returns a generic "file"
Feb 22 17:02:26 <federico>		icon if folder_unix==NULL.  Since you already have the stat
Feb 22 17:02:28 <federico>		info here (i.e. from create_file_info()), make
Feb 22 17:02:30 <federico>		get_icon_type_from_path() take that stat info if the folder is
Feb 22 17:02:32 <federico>		NULL.
Feb 22 17:06:06 <kris>	I think that will work correctly
Feb 22 17:06:08 <kris>	not 100% sure
Feb 22 17:06:57 <kris>	as far as I can see now it should be fine
Feb 22 17:09:37 <federico>	  + In load_folder():
Feb 22 17:09:37 <federico>		+  if ((folder_unix->types & STAT_NEEDED_MASK) != 0)
Feb 22 17:09:37 <federico>		+    fill_in_stats (folder_unix);
Feb 22 17:09:37 <federico>		+
Feb 22 17:09:37 <federico>		+  if ((folder_unix->types & GTK_FILE_INFO_MIME_TYPE) != 0)
Feb 22 17:09:38 <federico>		+    fill_in_mime_type (folder_unix);
Feb 22 17:09:40 <federico>	    GTK_FILE_INFO_ICON needs the stat and mime type; you should add
Feb 22 17:09:42 <federico>	    GTK_FILE_INFO_ICON to STAT_NEEDED_MASK.
Feb 22 17:10:36 <kris>	ok
Feb 22 17:17:54 <federico>	hmm
Feb 22 17:18:10 <federico>	kris: I'm writing this as I go in your patch - should I mail it to you and the list instead?
Feb 22 17:18:50 <kris>	federico: sure; might even be easier to have it all in one mail
Feb 22 17:21:07 <federico>	okay
Feb 22 17:21:17 <federico>	other than that, the patch looks awesome
Feb 22 17:21:29 <federico>	and it works - I tried bookmarks to unreachable sites, etc.
Feb 22 17:22:01 <federico>	one semi-major change is that I would prefer it if cancelled handles did *not* invoke the callback
Feb 22 17:22:03 <federico>	so that you can do
Feb 22 17:22:09 <federico>	file_handle_cancel(handle);
Feb 22 17:22:11 <federico>	unref_handle);
Feb 22 17:22:18 <kris>	we discussed the cancellation policy on the list
Feb 22 17:22:21 <federico>	and forget about it
Feb 22 17:22:39 <kris>	we decided that it would be better to always call the callback, so the callback would get a chance to free any user_data
Feb 22 17:23:18 <federico>	hmmmmmmmm
Feb 22 17:23:19 <kris>	the other option would be passing a destroy notify together with the callback and user_data
Feb 22 17:23:38 <federico>	okay
Feb 22 17:23:48 <federico>	leave it like it is for now; I'll remember to audit the callbacks
Feb 22 17:24:17 <federico>	kris: after this goes in, let's profile the hell out of it :)
Feb 22 17:24:27 <kris>	agreed ;)
Feb 22 17:24:28 <federico>	kris: did you have a chance to work on the unit tests?
Feb 22 17:24:39 <kris>	and look into progressively loading stuff
Feb 22 17:24:45 <federico>	yeah
Feb 22 17:24:53 <kris>	didn't have time yet to look into the unit testing ....
Feb 22 17:27:10 <federico>	ok
Feb 22 17:27:56 <federico>	ok, so I'll finish the review and mail you
Feb 22 17:28:03 <federico>	kris: again, thanks for putting out this awesome work
Feb 22 17:28:16 <kris>	thanks for reviewing ;)
Feb 22 17:28:22 <kris>	i hope we can land it in head soonish