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

[Piotr Galiszewski]

2010/7/21 Daniel Burrows <dburrows at debian.org>:
> On Tue, Jul 20, 2010 at 08:10:38PM +0200, Piotr Galiszewski <piotr at galiszewski.pl> was heard to say:
>> 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
>
>  You still have get_candidate_version, but it appears to return
> something other than the candidate version?  It should just read:
>
>    // ...
>    if(!candver.end())
>      candidate_ver = version::create(candver);
>    else
>      candidate_ver = version_ptr();
>    // ...
>

I use "candidate version" to display version in "candidate column" of
package list. Probably calling "candidate" is unfortunate. The
question is, what should be displayed in this column (lets forget
about candidate label for a moment). The newest version, the version
that is selected to installation, or the version that will be
installed after selecting install option under the package (candidate
version?)

>  Also, be aware that there may be some issues with caching the
> candidate version due to the way that version selection is handled
> (i.e., the candidate version of a package is *not* immutable).  I
> don't think you need to handle those in this iteration, though.
>
>> Should visible_version returns something in every situation
>> pkg.VersionList().end() returns true?
>
>  If by "true" you mean "false", then I think that seems correct.
>
>  Daniel
>
-- 
Regards
Piotr Galiszewski