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

[Daniel Burrows]

On Sat, Jul 10, 2010 at 06:23:22PM +0200, Piotr Galiszewski <piotr at galiszewski.pl> was heard to say:
> 2010/7/10 Daniel Burrows <dburrows at debian.org>:
> >> +  # 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

  I don't think we need to avoid newer methods.  How about just
setting this to the earliest version that you *know* we require (I
assume we need at least 4.0, for instance), and we can tighten it as
we become aware of the need to do so.  All this does is move errors to
an earlier step of the compile process, so we might as well not put
unnecessary obstacles in front of users with older versions of Qt.

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

  Sounds good.

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

  Sorry, one more change request -- instead say "To get the Qt version
number".

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

  This change broke the indentation of some lines:

            case OPTION_GUI:
-             gui = true;
+        use_gtk_gui = true;
              break;
            case OPTION_NO_GUI:
-             gui = false;
+        use_gtk_gui = false;
              break;
 #else
            case OPTION_NO_GUI:
              // Recognize it as a NOP.
              break;
 #endif
+
            case OPTION_LOG_LEVEL:

  Also, there's an extra blank line at the end of that block.

  Indentation is also wrote for these case blocks (the first one is
correct):

            case OPTION_NEW_GUI:
#ifdef HAVE_GTK
              use_new_gtk_gui = true;
#endif
              break;

#ifdef HAVE_QT
      case OPTION_QT_GUI:
        use_qt_gui = true;
        break;

      case OPTION_NO_QT_GUI:
        use_qt_gui = false;
        break;
#endif


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

  Hmm, leave this alone for now.  The way the GUI is selected should
change (as I mentioned earlier) and that'll make this easier; also,
all the option handling should be fixed eventually to not suck so
much.

  A third reason is that I doubt we'll normally have both GUIs
compiled in at once.

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

  The current main GTK+ file is src/gtk/gui.cc, btw (also not a great
name).

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

  Sounds good.

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

  This gave me a little trouble the first time I tried it too.  You
need to use the "--onto" option if you're transplanting patches onto
an unrelated branch.  Here's the relevant diagram from the manual:

----- Cut here -----

       Here is how you would transplant a topic branch based on one
       branch to another, to pretend that you forked the topic
       branch from the latter branch, using rebase --onto.

       First let’s assume your topic is based on branch next. For
       example, a feature developed in topic depends on some
       functionality which is found in next.

               o---o---o---o---o  master
                    \
                     o---o---o---o---o  next
                                      \
                                       o---o---o  topic
       We want to make topic forked from branch master; for example,
       because the functionality on which topic depends was merged
       into the more stable master branch. We want our tree to look
       like this:

               o---o---o---o---o  master
                   |            \
                   |             o'--o'--o'  topic
                    \
                     o---o---o---o---o  next


       We can get this using the following command:

           git rebase --onto master next topic

----- Cut here -----

  The important thing to remember with this form of rebase is that
"next" is the most recent patch that *isn't* included in the rebasing.
I usually just run gitk and copy SHA values out of it when I'm doing
this.

  I think the same logic applies when you're using "--interactive",
but I've only really used that for in-place rebasing (rewriting the
most recent commits on a branch, but leaving them attached to the same
parent).

  Daniel