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

[Suresh Chandrasekharan <Suresh Chandrasekharan Eng Sun COM>]

Thanks for the comments dobey. I have included my answers within '****'

>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.

*****
It is not a bug in evolution, dtmail does not handle html, I compiled the mail 
in evolution-1.5 and accidently sent it to hackers alias instead of patches, 
then when I realized the mistake, forwarded the mail from dtmail to the patches 
alias, that why. dtmail converts html to attachments.
*****

>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)

*****

Actually I was planning to include the .srpm version of the source initially, 
but the legal dept. needs or closer review on it seems. Certainly if I can't 
send the .srpm, then I will modify the configure.in to work with the idnkit 
library available in opensource and also will provide the source patches needed.

*******

>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

****
Will do this.
****

>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.

****
Will change similar occurances.
****

>
>> +#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.

****
OK
****

>
>> +#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.

*****
Agreeing with the first part of the comments that these functions should notbe 
ifdef'ed and e_uri_gnome_show_idn_url should be renamed as e_uri_show ( not 
e_url_show, since all functions in that file starts like that ). The difference 
between e_uri_decode_host and e_uri_encode_host is the first functions converts 
an ASCII Compatible Encoded hostname to a UTF-8 hostname, and the second 
function does vice versa. These functions only handle hostnames, the second set 
of functions take in urls and change the hostname with UTF-8/ACE depending on 
the function, it does the change in place. I can expand decd/encd...
******
>
>> -                       "%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?

******
I'm not sure it's a bug, but the current code makes paths without a leading '/' 
when it parses in the stuff to EUri struct, when trying to make a url out of it 
again this piece of code is needed.
******

>
>> 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?

****
I will try to do this too.
*****
>
>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.
*****
Waiting on other comments so that I dont habe to incoporate things in a 
piecemeal basis.
******

>
>-- dobey
>

Thanks & Regards,
Suresh