GNOME Bugzilla – Bug 506350
GIO port
Last modified: 2009-01-05 13:32:37 UTC
This constitutes the first step towards a GIO integration within gedit.
Created attachment 101822 [details] [review] [PATCH] First draft of GeditGIODocumentLoader Issues: it seems there is a bug with the code that gets the mtime. --- configure.ac | 1 + gedit/Makefile.am | 2 + gedit/gedit-document-loader.c | 8 +- gedit/gedit-gio-document-loader.c | 334 +++++++++++++++++++++++++++++++++++++ gedit/gedit-gio-document-loader.h | 77 +++++++++ 5 files changed, 420 insertions(+), 2 deletions(-)
Created attachment 103837 [details] [review] [PATCH] GIO conversion for GeditDocument gedit/gedit-document-loader.c | 53 ++++--- gedit/gedit-document-loader.h | 18 +-- gedit/gedit-document-saver.c | 49 +++--- gedit/gedit-document-saver.h | 22 ++-- gedit/gedit-document.c | 288 ++++++++++++++------------------ gedit/gedit-document.h | 4 +- gedit/gedit-gio-document-loader.c | 22 +-- gedit/gedit-gnomevfs-document-loader.c | 14 +- gedit/gedit-gnomevfs-document-saver.c | 12 +- gedit/gedit-local-document-saver.c | 22 ++-- gedit/gedit-marshal.list | 4 +- gedit/gedit-metadata-manager.c | 23 ++- gedit/gedit-metadata-manager.h | 5 +- gedit/gedit-mmap-document-loader.c | 14 +- 14 files changed, 256 insertions(+), 294 deletions(-)
Created attachment 105914 [details] [review] gedit utils A bit hack to remove gnome vfs from two funcs in gedit-utils
Created attachment 105915 [details] [review] utils patch I don't know what damn i upload before. Sorry about that.
Ignacio: I think you should at least support gnome-open as well as xdg-open, since we are still in the gnome world... In the long term, what should disappear are all the URIs that are spat throughout gedit. The approach I took in my patches was to change the core while exposing the same API, and change it incrementally. But there is still much work to be done...
I may be wrong but isn't gnome-open in libgnome[ui]? if so just cut and paste that bit of code until there is a replacement in GLIB. Reason: we really want to KILL libgnome/bonobo deps because that libs is dragging in a hell lot of libs for example gnomevfs and that way we get more dependies than before GVFS was there and that is not what we want :-)
so, any news here? do we need a reworked patch, or what is the status?
yes, gnome-open IS in libgnome, also see http://live.gnome.org/LibgnomeMustDie#line-34 . > if so just cut and paste that bit of code until there is a replacement in GLIB we don't really kill libs by duplicating their code. from my POV we want to get rid of gnome-vfs; libgnome's deps on gnome-vfs is a different issue to deal with. So please get this in. Somehow. :-)
This seems relevant for the "utils patch": http://live.gnome.org/GnomeGoals/RemoveGnomeOpenGnomeHelp (use gtk_show_uri)
Created attachment 116143 [details] [review] GIO loader/saver Little update, most of the gnomevfs code has now been removed from the general parts of gedit (and plugins) as well as removing gnome_url_show and gnome_show_help (replaced by gtk_show_uri). This is already in the current trunk of gedit. The last parts (the loader, saver, error messages) can be found in the attached patch and need a more thorough review than the other small bits I already committed.
First round of comments: * strerror -> g_strerror * replace the get_mime_type method of saver/loader with get_content_type * the loader/saver load() and save() funcs should take a GFile directly (this however can be refined later incrementally) * the warning fix in the open-location-dialog can go in immediately * add yourself to authors/copiright of the files :) * is get_mount_operation_accumulator still needed? * I do not like gedit_document_saver_get_document_contents much, we should try to move away from the need to buffer a whole copy of the content
more comments: + /* we use GIO error codes, so start at some high number? */ this sounds wrong... why are we just using the error codes? If we get a GError from gio, we should "bubble it up" entirely with it's own domain The mount op factory function we discussed on irc should probably also take a GFunc to free the user data to be a bit more flexible... I am not sure if in our case it's a good idea to ref/unref the tab. Also please let's add a comment about what this func does and why it is needed. We use some idle func/timeouts to run the save/load, and this can interact with gtk to use the mount operation: I think the idle func content should be protected by the gdk lock as documented here http://library.gnome.org/devel/gdk/unstable/gdk-Threads.html Should we use GBufferedOutputStream instead of manually chunking the write ourself in the gio saver? (just an idea, not sure if it is a good idea or even doable)
Created attachment 116405 [details] [review] patch This is Jesse's patch updated to apply cleanly against head and with some minor problems fixed: in particular when reading an attribute from GFileInfo we cannot rely on the fact that it is there, we have to check with _has_attribute(). Most of the my previous comments still apply.
Created attachment 116408 [details] [review] patch Same patch as before, but saver/loader refactored to expose content_type and not mime_type (content_type -> mime_type conversion happens in gedit-document for now, later we should also expose content type from the doc and use it directly for the lang-manager)
I further tested and reviewed this... I think we should commit and release in order to maximize user testing and work incrementally to fix up the remaining issues. If anyone think this is a bad idea, please shout now :)
Created attachment 116492 [details] [review] committed patch Ok, no one shouted. This is the patch that was committed. I am not closing this bug since I think we should still work out some details, for instance the ones in the previus comments. The one about GErrors is the most pressing in my opinion
I've just committed a fix that propagates the gio errors properly. Things that need to be done: * Change gedit internals to use content type, we should only use mime_type for gsv * Check idle func/timeouts to run the save/load and the gdk lock * Factor the encoding conversion in a decoration buffer object (GeditEncodingBuffer?). We should also try to not copy the whole text buffer while saving, but incrementally encode and write the text buffer.
I think we can close this now