[Aptitude-devel] Review: 001-build-skeleton

[Daniel Burrows]

  Ok, here goes.  (resending because I typed the wrong list address
the first time)

> diff --git a/configure.ac b/configure.ac
> index d5b5059..2060d18 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -538,6 +538,91 @@ fi
>
>  AC_DEFINE_UNQUOTED(SIGC_VERSION, ["$(pkg-config --modversion sigc++-2.0)"], [The version of libsigc++ with which the program was compiled])
>
> +########################################################################
> +
> +WANT_HAVE_QT=1

  Could you default this to off, actually, and change the help text
accordingly?  It can be enabled by default once it's more stable.

  (snip)

> +  # required version of Qt libraries
> +  QTCORE_REQUIRED=4.6.0
> +  QTGUI_REQUIRED=4.6.0

  (snip)

  Are these versions actually required by our code?

> +  # test for qt4-moc
> +  AC_PATH_PROG(MOC, moc-qt4)
> +  if test ! -x "$MOC"; then
> +    AC_PATH_PROG(MOC, moc)
> +    if test ! -x "$MOC"; then
> +      AC_MSG_WARN([Qt MOC hasn't been found.])
> +      HAVE_QT=0
> +    else
> +      AC_MSG_NOTICE([Qt MOC has been found.])
> +    fi
> +  fi
> +else
> +  AC_MSG_NOTICE([Disabling the Qt frontend at your request (--disable-qt).])

  If Qt is disabled by default, this should change to something like
"Qt not enabled; use --enable-qt to enable it."

> +fi
> +
> +if test x$WANT_HAVE_QT = x1 && test x$HAVE_QT != x1
> +then
> +  AC_MSG_WARN([Unable to find the necessary Qt libraries; disabling Qt support.])
> +fi
> +
> +if test x$HAVE_QT = x1
> +then
> +
> +   # get flags:
> +   if test "x${PKG_CONFIG}" != "x" && ${PKG_CONFIG} --exists QtGui
> +   then
> +      QT4_CPPFLAGS="`${PKG_CONFIG} --cflags QtCore` `${PKG_CONFIG} --cflags QtGui`"
> +      QT4_LIBS="`${PKG_CONFIG} --libs-only-l QtCore` `${PKG_CONFIG} --libs-only-l QtGui`"
> +   fi

  Why not use PKG_CHECK_MODULES here?  Also, didn't you check QtCore
up above (in some of the code I snipped)?  Why are we checking it
again?

  Also, why do you test for QtGui here but QtDBus above?

> +   # default flags:
> +   if test "x${QT4_CPPFLAGS}" = x ; then
> +      QT4_CPPFLAGS="-DQT_SHARED -I/usr/include/qt4 -I/usr/include/qt4/QtCore -I/usr/include/qt4/QtGui"
> +   fi

  pkg-config should set these already, shouldn't it?

  If you really want to have fallback values, please add a configure
check that tries to compile a small Qt program with them to verify
up-front that they work.  (you can test this by manually breaking the
Qt check; just be sure to fix it again! :) )

> +   if test "x${QT4_LIBS}" = x ; then
> +      QT4_LIBS="-lQtGui -lQtCore"
> +   fi
> +fi
> +
> +if test x$HAVE_QT = x1
> +then
> +  AC_MSG_NOTICE([The Qt frontend will be built.])
> +fi
> +
> +AM_CONDITIONAL([BUILD_QT], [test x$HAVE_QT = x1])
> +if test x$HAVE_QT = x1
> +then
> +  AC_DEFINE([HAVE_QT], [], [Define if the Qt frontend is included in the build.])
> +fi
> +
> +if test x$HAVE_QT = x1
> +then
> +  CXXFLAGS="$CXXFLAGS $QT4_CPPFLAGS"
> +  LIBS="$LIBS $QT4_LIBS"

  Shouldn't you use "$QtCore_CPPFLAGS $QtGui_CPPFLAGS" and
"$QtCore_LIBS $QtGui_LIBS" to take advantage of the flags that
PKG_CHECK_MODULES already set?

> +fi

  (snip)

> diff --git a/src/main.cc b/src/main.cc
> index 03af0a8..a5b2fc7 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -47,6 +47,10 @@
>  #include <gtkmm.h>
>  #endif
>
> +#ifdef HAVE_QT
> +#include <QtCore/qglobal.h>
> +#endif

  Could you add "// For the Qt version" here?

  (snip)

> @@ -315,6 +334,9 @@ option opts[]={
>    {"gui", 0, &getopt_result, OPTION_GUI},
>  #endif
>    {"no-gui", 0, &getopt_result, OPTION_NO_GUI},
> +#ifdef HAVE_QT
> +  {"qt-gui", 0, &getopt_result, OPTION_QT_GUI},
> +#endif

  Maybe just "--qt" and "--no-qt" instead (long-term, --gui should
probably be deprecated in favor of --gtk).  Also, add a --no- variant
so it can be disabled if it's configured to be the default.

>    {"log-level", 1, &getopt_result, OPTION_LOG_LEVEL},
>    {"log-file", 1, &getopt_result, OPTION_LOG_FILE},
>    {"log-config-file", 1, &getopt_result, OPTION_LOG_CONFIG_FILE},
> @@ -656,6 +678,11 @@ int main(int argc, char *argv[])
>    bool use_new_gui = false;
>  #endif

> +#ifdef HAVE_QT
> +  // Use Qt frontend.
> +  bool use_qt_gui = false;
> +#endif

  Could you change "gui" to "use_gtk_gui" and "use_new_gui" to
"use_new_gtk_gui"?  I'd also like to change the configuration option
to control starting the GUI by default, but we can do that later
(Aptitude::Gui=none|gtk|qt, deprecate Aptitude::Start-Gui).

  Add a sanity-check here that exits with an error if the user tries
to use both the GTK+ and the Qt GUI at the same time.

> +  if(use_qt_gui)
> +    {
> +      if(aptitude::gui::qt::init(argc, argv))

  Please name that routine gui::qt::main().  "init" in the GTK+ code
is an experimental routine with a bad name.

  (snip)

> +++ b/src/qt/init.cc

  Maybe we could pick a better name?  I think maybe "qt-main.cc",
"qt.cc", or "main.cc" would be reasonable (with the routine being
renamed to main()).

  (snip)

  Daniel