[Aptitude-devel] Review: 1ec363ac (001.1-package^)

[Daniel Burrows]

On Tue, Jul 20, 2010 at 9:36 AM, Piotr Galiszewski <piotr at galiszewski.pl> wrote:
> 2010/7/20 Daniel Burrows <dburrows at debian.org>:
>>> +      package::package(const pkgCache::PkgIterator &_pkg)
>>> +     : pkg(_pkg), is_virtual(false), short_desc_fallback(_("virtual"))
>>
>>  I wonder if it would be better to default-construct
>> short_desc_fallback (i.e., don't list it) and then assign it from
>> _("virtual") later, to avoid looking up _("virtual") for non-virtual
>> packages.  Or if you really want to avoid the default-construction,
>> use a ternary operator to avoid it.
>>
>
> I tried to make this variable const. I will fix it and initialize this
> variable later

  I don't think there's a big payoff for constness here, since the
whole class is based on using mutable members to cache.

>>  Is that logic correct?  I think you should only set is_virtual if
>> there are no versions at all (i.e., pkg.VersionList().end() is true).
>>
>
> True. I had a problem with this. How should things work for such
> packages. Searching and filtering for packages uses candidate version
> now (through package getters). I am not quite sure if it is good
> thing. Maybe I should add new method: get_version and use it instead
> of get_candidate_version. get_version will return candidate version if
> available, otherwise the newest version of package. All checks have to
> be fixed in this situation

  At the least, you shouldn't use is_virtual to mean something
different from the rest of the codebase; that will lead to confusion.

  Other frontends have the concept of the "visible version" of
a package; I'd suggest using that.  is_virtual will then be true iff
there is no visible version -- although I might just check whether
the visible version is NULL instead.

>>  Rendering logic in such a generic module raises a red flag for me.
>>
>>  I suggest just caching the parsed description, and letting consumers
>> of this class render it.  Or, alternatively, just always re-parsing
>> the description and returning a new parse each time.
>>
>
> I am little scared of performance in this case, as caching was the
> main purpose for writing this classes. Displaying this description is
> not a problem, because it is not frequently task. So rendering could
> be done in gui. Bigger problem is searching by description. In this
> case probably It will be better to give an access to unparsed
> description (maybe new getter?)

  I think that it's better to expose the raw description directly, yes.
Actually, if you use the matching library, you might not need that; it
will just handle the description internally.

  Daniel