Some comments. Firstly, I'll point out that the body part of your message, both the text/plain and text/html parts were only viewable as attachments. I'm not sure if this is a bug in Evolution or in dtmail, but one of them should show up as default, and neither should show up as attachments with the expansion arrow buttons. Also, please do not send rpm packages in your attachments. It makes your mail fairly huge, and is not very useful for people on non-rpm systems, or in this case, on non-ix86 systems also. Please only put a URL to the download location for the source code of the dependency you are adding. Now, on to the patch and README.idn... > Download build and install the idnkit library from > http://www.nic.ad.jp/ja/idn/idnkit/download/. > I have provided a rpm along with this to use, which's a slightly > modified version of the download from above, plus a .pc file etc, which > makes it possible for pkg-config to find out about it. Please do not add something that depends on patches to the new library, in order to work, especially patches that are only available in the i386 binary rpm attachment. > -EVO_SET_COMPILE_FLAGS(E_UTIL, gthread-2.0 gconf-2.0 libxml-2.0 > libbonoboui-2.0 libglade-2.0 gal-2.2 >= $GAL_REQUIRED libgnomeui-2.0 > libgnome-2.0 libgnomecanvas-2.0 $mozilla_nspr, $THREADS_CFLAGS > $MANUAL_NSPR_CFLAGS, $THREADS_LIBS $MANUAL_NSPR_LIBS) > +E_UTILS_DEP=" gthread-2.0 gconf-2.0 libxml-2.0 libbonoboui-2.0 > libglade-2.0 gal-2.2 >= $GAL_REQUIRED libgnomeui-2.0 libgnome-2.0 > libgnomecanvas-2.0 $mozilla_nspr" > + > +if test "x$enable_idn" = "xyes"; then > + E_UTILS_DEP="idnkit $E_UTILS_DEP" > +fi > + > +EVO_SET_COMPILE_FLAGS(E_UTIL, $E_UTILS_DEP, $THREADS_CFLAGS > $MANUAL_NSPR_CFLAGS, $THREADS_LIBS $MANUAL_NSPR_LIBS) This bit should probably be avoided, since it depends on a specific patch which is only available in the patched i386 binary rpm, and not in the upstream source code. This means that --enable-idn will not work with the original upstream source, even though the README.idn suggests it will. If the upstream source does not provide a pkgconfig pc file, then you should use the available means that it does provide, for determining the CFLAGS and LIBS variables to add to Evolution. You aren't checking for a version anywhere, so I don't see what using pkg-config would gain us over using the normal AC_TRY_COMPILE that you are already using above the EVO_SET_COMPILE_FLAGS() changes. If there is a need to check for a version, you should get the pc file generation added upstream, and then use that, in which case, the AC_TRY_COMPILE stuff can be removed, I believe, and then the new version of idnkit would be required. Or, if idnkit provides any version information in the header files or in an idnkit-config script, or similar, you should use that. If it provides an idnkit-config script, you should probably be using it anyway. > +#ifdef ENABLE_IDN > +#include <e-util/e-url.h> > +#endif There shouldn't be a need to do this. This code is in Evolution anyway. Just go ahead and always include it. You should only do the #ifdef check in the code in e-url.c that actually needs it, rather than in every file that displays a URL in the UI somewhere. > + if (t) g_free (t); This is redundant and won't guard against crashing, since g_free () guards against NULL already. If the memory passed to g_free () is corrupt, it will still crash, and it will be non-NULL, so will get past the if (t) check anyway. > +#ifdef ENABLE_IDN > +#include <idn/api.h> > +#include <errno.h> > +#include <libgnome/gnome-url.h> > +#include <glib.h> > +#include <gal/widgets/e-unicode.h> > +#endif The <idn/api.h> is the only thing here that should be in the #ifdef, I think. Also, I believe in glib 2.0+ there are appropriate unicode functions to do what you are using e-unicode for. We are trying to avoid adding extra dependencies on gal in the code. Please use the glib equivalent functions for character set detection and encoding. > +#ifdef ENABLE_IDN > +char *e_uri_encode_host (char *utf8_name); > +char *e_uri_decode_host (char *ace_name); > +void e_uri_replace_with_decd_host (char **uri_string); > +void e_uri_replace_with_encd_host (char **uri_string); > +void e_uri_gnome_show_idn_url (const char *uri_string, GError > **err); > +#endif Do not put public functions inside of an #ifdef. These functions should all just do the right thing internally in the !ENABLE_IDN case, as well as handle the ENABLE_IDN case. You might consider just renaming the url_show function to "e_url_show", and wrapping gnome_url_show to do the correct thing. Then you can just replace all calls to gnome_url_show in the Evolution code to call e_url_show. Also, what is the difference between the e_uri_{encode,decode}_host and e_uri_replace_with_{encd,decd}_host functions, and why don't the latter have encode and decode spelled out? The replace functions seem redundant to me. > - "%s://%s%s%s%s%s%s%s:%d%s%s%s", > + "%s://%s%s%s%s%s%s%s:%d%s%s%s%s", > uri->protocol, > uri->user ? uri->user : "", > uri->authmech ? ";auth=" : "", > @@ -319,12 +538,13 @@ > uri->user ? "@" : "", > uri->host ? uri->host : "", > uri->port, > + uri->path ? "/" : "", > uri->path ? uri->path : "", It seems like you are trying to work around a bug here. Shouldn't uri->path always begin with a "/" character? > text/x-sun-c-file attachment (idn.c), "idn.c" Shouldn't this code be a test case/utility that comes with idnkit? It seems that it may be useful. Perhaps it should be pushed upstream and included with the idnkit tarball? These are the things that are obvious to me. The code seems pretty straightforward, but a lot of it seems redundant and duplicated also. I'll leave actual per-component acceptance up to the component maintainers. But judging from the build side of things, I would clean it up a lot before I put it in. -- dobey
Attachment:
signature.asc
Description: This is a digitally signed message part