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. >> 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 _______________________________________________ Aptitude-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
