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

[Piotr Galiszewski]

2010/7/20 Daniel Burrows <dburrows at google.com>:
> 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.
>

OK

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

Ah. I see (looking at pkg_item). I mixed logic of candidate version
and visible version. It looks like I should rename candidate_version
to visible_version and use it as previous. Furthermore, pkt_item's
visible_version uses the the same code as my candidate_version. I have
only one more questions. If I read it correctly this visible_version
returns NULL in the same situations as my candidate_version.

In the second version of my patch (the first version without Qt
dependencies), I used proper logic for is_virtual and
candidate_version (should have been visible_version). I have changed
that code after first real testing of this code. My code crashes
frequently when is_virtual  was false and candidate_version returned
null. I based that code on pkg_tree and pkg_item, but probably I
missed some important checks

Should visible_version returns something in every situation
pkg.VersionList().end() returns true?

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

OK. I will use matching library as another filter for supporting
condition language. But I'd like to have current filter for basic
filtering

>  Daniel
>

-- 
Regards
Piotr Galiszewski