Comment 3 for bug 198861

Revision history for this message
Loïc Minier (lool) wrote :

Here's a quick review:

1) the debdiff touches debian/patches/clam-mail.1.patch, doesn't seem to change the meaning of the patch; would be nice to drop this gratuitous change to ease review for the other claws-mail people which might want to review this (e.g. the Ubuntu folks with interest in claws-mail or the Debian folks if we send them the patch)

2) I wonder whether it would make sense to:
a) not use ENABLE_UME / ENABLE_HILDON high level defines but instead use ENABLE_$feature for granular features; e.g. ENABLE_LARGE_WINDOWS or ENABLE_POWER_SAVING; I think this is better style autofoo which results in less hardcoding of platforms in all the source code; this might result in more readables "#ifdef HAVE_hildon_gtk_entry_set_input_mode" style
b) define MAEMO when you define UME
c) drop the UME define altogether and instead make sure you define MAEMO but not CHINOOK

what do you think?

3) An AC_ARG_ENABLE(ume, ...) is needed before using things like ac_cv_enable_ume

4) I have to admit I don't know about multiple PKG_CHECK_MODULES(FOO, ...) calls, are these additive?!

5) The AC_DEFINE(UME, 1, Build for maemo) should be "Build for ume"

Generally looks good, but I have having us to maintain the pile of UME ifdefs just to avoid the CHINOOK ifdef and the a couple of hildon specific functions; are these used for other things? Are we missing some CHINOOK stuff? Why are we missing these functions?

Thanks for your work!