[Aptitude-devel] Review of eb4d4a

[Piotr Galiszewski]

2010/8/17 Daniel Burrows <dburrows at debian.org>:
>  Just some minor things here and then I think this is ready to merge.
>
>> +        /** \brief Method called after package pkg state has changed. */
>> +        void package_state_changed(std::vector<package_ptr> pkgs);
>
>  Pass by const reference.
>

OK

>> +
>> +        /** \brief Method called after reloading package cache. */
>> +        void handle_cache_reloaded();
>> +
>> +        /** \brief Method called after closing package cache. */
>> +        void handle_cache_closed();
>> +
>> +      public:
>> +        /** \brief Create a new package_model for the default package_pool. */
>> +        explicit packages_model_impl(QObject *parent = 0);
>> +
>> +        /** \brief Create a new package_model for the given package_pool. */
>> +        explicit packages_model_impl(package_pool *pkg_pool, QObject *parent = 0);
>
>  This should also be exposed in the header (forward-declare
> package_pool).
>

I am not sure what did you mean? I should create second
create_packages_model method with package_pool as parameter?

[snip]

>> +namespace aptitude
>> +{
>> +  namespace gui
>> +  {
>> +    namespace qt
>> +    {
>> +      const int aptitude_role = 1000;
>> +
>> +      const int package_role = aptitude_role + 0;
>> +      const int package_name_role = aptitude_role + 1;
>> +      const int package_short_description_role = aptitude_role + 2;
>> +      const int package_state_role = aptitude_role + 3;
>> +      const int package_installed_version_role = aptitude_role + 4;
>> +      const int package_candidate_version_role = aptitude_role + 5;
>> +      const int package_archive_role = aptitude_role + 6;
>> +
>> +      const int filter_role = aptitude_role + 7;
>
>  I wonder if it would be valuable to use a macro here?  Not a
> requirement, just an idle thought.
>
> #define DEFINE_APTITUDE_ROLE(id, name) const int name = aptitude_role + id;
>
>         DEFINE_APTITUDE_ROLE(0,  package_role);
>         DEFINE_APTITUDE_ROLE(1,  package_name_role);
>
>  ...etc.  The reason for doing this is just to make the list easier
> to scan (that's why I put two spaces after each comma, so if there are
> more than ten roles, everything still lines up).
>

OK, no problem :)

I have one question about rebasing this patches. Before doing this I
have to create required code snippets for google. We will be able to
submit them since 30th and new code is not allowed to be there. I am
not quite sure what will be the best option for this? Copy of files,
one big patch or patch for each commit? I need to make a decision and
any advice is much appreciated ;)
After creating this required archive I will normally rebase my patches

Thanks a lot for review and all your help and patience during summer.
"Unfortunately", you have to "live" with me for some more time ;)

>  Daniel
>
> _______________________________________________
> Aptitude-devel mailing list
> Aptitude-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
>
-- 
Regards
Piotr Galiszewski