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

[Piotr Galiszewski]

Resending my answer due to the same reason :)

2010/7/10 Daniel Burrows <dburrows at debian.org>:
>  Ok, here goes.
>
>> 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.
>

done

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

Actually not, but I am not developing with this versions in my system.
Also this versions are available on testing and unstable, so I thought
that it is reasonable requirement. Of course I can try to not use
newer methods, and lower requirements

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

done

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

QtDBus check was a typo. fixed
- Pokaż cytowany tekst -

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

I did not check PKG_CHECK_MODULES macro. Now, I have removed all
manual PKG_CONFIG checking. I have also resigned from  fallback
values. It is hard to find good values, and probably it will be better
to add --with-qt-dir option in the future :)

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

done

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

done

>>    {"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).
>

done

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

I am not sure how to do this in proper way, cause use_gtk_gui is
enabled by default (it is a reason why check for use_qt_gui is above
gtk checks)
I do not see any other solution except adding new bool variables for
checking if the option was activated programatically or manually. Is
it an acceptable solution?

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

Yes, I have based on GTK code. Fixed

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

I chose qt_main.(cc|h), cause I was unable to find any file with - in name ;)

On the other hand, I have learned today, that using git rebase -i with
commit in first branch could be "a little" problematic for other
branches ;)

>  (snip)
>
>  Daniel
>
--
Regards
Piotr Galiszewski