Re: [evolution-patches] IDN patch for Evolution Head



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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]