2010/7/20 Daniel Burrows <[email protected]>: > On Tue, Jul 20, 2010 at 9:36 AM, Piotr Galiszewski <[email protected]> > wrote: >> 2010/7/20 Daniel Burrows <[email protected]>: >>>> + 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 _______________________________________________ Aptitude-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
