GNOME Bugzilla – Bug 506350
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]
A bit hack to remove gnome vfs from two funcs in gedit-utils
Created attachment 105915 [details] [review]
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.
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":
Created attachment 116143 [details] [review]
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
+ /* 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]
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]
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]
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