[Aptitude-devel] Review of b598238

[Daniel Burrows]

On Mon, Aug 2, 2010 at 11:03 AM, Piotr Galiszewski <piotr at galiszewski.pl> wrote:
> 2010/8/2 Daniel Burrows <dburrows at debian.org>:
>>> +     {
>>> +       switch(section)
>>> +       {
>>> +       case 0:
>>> +         return "";
>>> +       case 1:
>>> +         return "Package";
>>> +       case 2:
>>> +         return "Installed";
>>> +       case 3:
>>> +         return "Candidate";
>>> +       case 4:
>>> +         return "Archive";
>>> +       default:
>>> +         return "";
>>> +       }
>>
>>  What do these hardcoded numbers represent?  They appear to be
>> numeric column IDs counting from zero?  Could we make an enum for this?
>>
>
> True. Probably we cannot to this. It is a reimplementation of
> appropriate virtual method;
>
>>  Also, these should get _(), as should the string below.
>>
>
> I am not sure why getter is needed? This method is used internally. It
> will not be used in normal code

  Sorry, that was too much abbreviation.  I mean they need to be passed
to gettext:

  case 1:
    return _("Package");
  etc.

> This string is only for completeness and probably we will never see
> this text ;) Row is quite standard, but I can use something diffrent

  I guess "row" is fine for now.

> package_at_index will be used externally in one of the later patches.
> So declaration has to be in place. But as I said before it will be
> rewritten as an abstract interface in the future (to be more specific,
> as this patch needs some fixes, I will do this at the same time ;) )

  OK, no problem.

> Yes. I have a big problem how to document this functions. They are
> simple reimplementation of abstract methods, and they are used
> internally by other Qt classes (by view in this situation).

  I like having some documentation, though, because not everyone
who looks at this will be a Qt expert or have the Qt docs handy.

  Daniel