[Aptitude-devel] 002-qt-stubs review

[Piotr Galiszewski]

2010/7/14 Daniel Burrows <dburrows at debian.org>:
> On Wed, Jul 14, 2010 at 02:09:15AM +0200, Piotr Galiszewski <piotr at galiszewski.pl> was heard to say:
>> 2010/7/13 Daniel Burrows <dburrows at google.com>:
>> >  If Qt has no way of abstracting over slots, that would make this
>> > difficult.  In that case maybe you could just make a public signal for
>> > now?
>> >
>>
>> There is another problem. Qt moc requires that all declaration of
>> signals and slots have to be placed in header file.
>
>  That's kind of awkward...
>

It was false alarm. I added new rule to moc.mk file and now it is
working. It is in first patch in rebased series

>> Due to that, completely hiding implementation will not work and there
>> have to be another header. I can create tabs_manager_impl.h header,
>> but I do not know if it is acceptable solution. Another solutions are
>> not using normal class as it was written before, or something different
>> than Qt signals and slots mechanism. Maybe I am wrong, but my google
>> skill is to low to find better solution :/
>
>  I don't have a problem with using the sigc++ mechanisms more broadly,
> but I had the impression from you and Sune that this was strongly
> dispreferred.
>
>  If it's just a signal, could you leave it in the header despite the
> rest of the class being abstract?  I guess that means that other code
> could emit the signal, but other than that it should be OK for now...
>

As I wrote above it is working now. Probably there will be no problems
with this in the future. So another parts could also be written as
abstract interface. Which classes should use this approach? Every
widget?

>> Second problem is that I do not know for now, how to force one
>> packages_tab per window. Probably I will delay it for some later patch
>
>  Assuming you identify tab_widgets with windows, what about just a map
> storing the packages tabs that are active on each widget?
>
>    boost::unordered_map<tab_widget *, boost::unordered_set<packages_tab *> >
>      active_packages_tabs;

[snip]

Yesterday, I tried to use QMap/QHash with QList in exactly the same
way. Unfortunately QMap/QHash uses const references, so this wasn't
work. With boost it working. Thanks! ;) I have also changed other
QLists in this file to boost::unordered_set, so there is consistency
in the code

>
>  (this would be a really good candidate for unit-testing, with a
>   little refactoring to hide the widgets behind abstract interfaces
>   so that the unit test didn't have to use GUI objects directly ...
>   if such a thing is possible with the way Qt works, anyway)
>

Probably it will work ;)

Making this code multi windows aware needed more (sometimes tricked)
functions, but it should work without problems

I have also split patches a little, but in different way than you
suggested. I tried to make every patch compilable.

Thanks for the review and suggestions!

>  Daniel
>
-- 
Regards
Piotr Galiszewski