GNOME Bugzilla – Bug 93585
IOChannel wrappers for glibmm
Last modified: 2004-12-22 21:47:04 UTC
Attached is a patch to add wrappers for IOChannels in glibmm. Please comment on them; I still think there are some issues WRT code design and formatting. These wrappers are also derivable; one can create his own IOChannels in C++ TODO: Documentation. Tassos
Created attachment 11164 [details] [review] Glib::IOChannel implementation patch
I can't read that file. Is it a .tar, or a .tar.gz, maybe? Please tell us the filename in the description.
The attachment's name should be "iochannel.patch.gz"
I don't have many comments because I don't have much knowledge about this kind of thing. I will leave it to someone such as Daniel Elstner to decide about this. But: * You need to patch the ChangeLog. * Please use Doxygen-style comments. You should just need to use /** instead of /*. * Please remove the flippant comments such as "Oh, Lord, please forgive me." They are not helpful. * A cvs diff might be better, if possible. Otherwise, well done. It probably wasn't easy to figure out how the gtkmmproc macros can be used.
Created attachment 11176 [details] [review] iochannel.patch
This should be a more proper patch. Includes ChangeLog entry, basic documentation and a better iostream-IOChannel.
As I promised on the ml I created a patch that modifies the Glib::Source implementation so that it is possible to wrap existing GSource objects. See ChangeLog and comments in the source for details. This patch doesn't actually add a wrap function because the GSource objects must be wrapped in the derivations of Glib::Source. I will provide another patch that adds a Glib::IOSource::create(const IOChannel&) creator function as soon as IOChannel is added. Is it okay, to commit the patch (it compiles and the timeout and thread examples work)?
Created attachment 11218 [details] [review] glibmm-source.diff: Modify implementation of Glib::Source to allow for Glib::Source::Source(GSource*, GSourceFunc) ctor.
Martin, does your patch replace Tassos's patch? Or is it additional? Please provide more explanation. Daniel will probably decide what should be applied.
My patch is stand-alone. It prepares the current Glib::Source implementation for _future_ addition of Glib::IOSource::create(const IOChannel&) and for addition of other derivations of Glib::Source that wrap existing GSource objects in glibmm or projects depending on glibmm. As a reminder: Glib::Source objects can be attached to any Glib::MainContext to listen to all kinds of event sources (timeouts, idle, io).
Martin, I have applied your patch. Sorry for the delay - I really wanted Daniel to take care of this. So, will the next patch, that you mentioned, be a replacement or an addition to Tassos's patch?
It will be an addition. Tassos's patch provides Glib::IOChannel, my addition would be Glib::IOSource::create(const IOChannel&). Unfortunately I'm not the ideal person to review Tassos's patch because my knowledge of stl streams is too little to write one.
First of all, sorry for the delay. OK, I had a look at your patch and decided to do a major rewrite (sorry), which is still based on your work though. Thanks for that. Roughly, I made the following changes: 1. I chose a different class/file structure. You'll find class StreamIOChannel in the public API which implements most of the stuff you had in iochansup.cc. Also, the internal layout (particularly wrt. reference counting) is now nearly the same as for Glib::Source (or rather as it was before Martin's recent changes ;) 2. Several methods got renamed, some removed. E.g. it seems you misunderstood the g_io_channel_unix_new() vs. g_io_channel_win32_new_fd() issue. The docs should explain why it doesn't make sense to decide which one to use at compile time. 3. There were several problems in your implementation, for instance mixing up characters/bytes as well as relying on undefined behaviour. (I.e. in one place you tried writing to the buffer returned by string::c_str()). 4. Indentation style -- uhm, well, your preferred style seems to differ at least slightly from the gtkmm one ;) Next time, please adapt to the style used thorough the library. There is one problem: I didn't have time to test the new code, so it's likely that there are some errors lurking. It'd be nice if you could test it (I'll do so myself too, but that might take some time). So, if there's something in the code you think is plain wrong, missing, different from your intentions, etc. please complain, so we can discuss it. --Daniel
IOChannel::create_watch() is still missing! I'm attaching a patch that adds a) overloads for IOSource::create() and SignalIO::connect() that take a const Glib::RefPtr<IOChannel>& and b) IOChannel::create_watch() to match the glib API. Daniel, is it OK to commit?
Created attachment 11564 [details] [review] iosource-iochannel.diff: Add IOChannel::create_watch() and overloads for IOSource::create() and SignalIO::connect()
Created attachment 11565 [details] [review] iosource-iochannel.diff: Add IOChannel::create_watch() and overloads for SignalIO::connect() and IOSource::create().
Sorry, I accidently added the same patch twice. galeon2 is not very stable with bugzilla...
--- glib/src/iochannel.hg 15 Oct 2002 00:02:28 -0000 1.1 +++ glib/src/iochannel.hg 15 Oct 2002 18:27:18 -0000 @@ -40,6 +40,8 @@ { class Source; +class IOSource; +enum IOCondition; Incomplete enum types is a GCC extension. I'm wondering how it could compile for you since the GCC 3.2 documentation states that this extension is *not* supported by GNU C++. Perhaps this has been a recent change. Otherwise it looks good. Just #include <glibmm/main.h> instead and go ahead.
Just FYI, I never experienced any problems with Galeon2 (using Mozilla 1.1 + Xft) and bugzilla.
Martin, I committed your patch myself to make sure it's in before the freeze. --Daniel
Thanks a lot, I've been unavailable until just now. By the way, my Mozilla version is CVS head (rv:1.2b) :-)