<div class="gmail_quote">On Thu, Jul 8, 2010 at 1:39 AM, Piotr Galiszewski <span dir="ltr"><<a href="mailto:piotr@galiszewski.pl">piotr@galiszewski.pl</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
2010/7/8 Daniel Burrows <<a href="mailto:dburrows@algebraicthunk.net">dburrows@algebraicthunk.net</a>>:<br>
<div class="im">> * Actually, please organize your repository as I noted above for at</div><div><div class="h5">
> least the first set of patches, so I can thoroughly vet your earlier<br>
> patches in isolation. (you should be able to just tag branches with<br>
> judicious use of "git branch")<br>
><br>
<br>
</div></div>OK. It will be my first task today. I will try to incorporate a the<br>
same time your other comments. The repository format I have chosen is<br>
a result of my bad habits from svn and Kadu repository, where is<br>
single master repository (topics branches are only for bigger<br>
reworking of the code)<br></blockquote><div><br></div><div> No problem -- I only came across this idea while I was trying to improve</div><div>my git-fu a couple months ago. You can see my WIP feature branches</div><div>
in my personal repository here:</div><div><br></div><div>git://<a href="http://git.debian.org/~dburrows/aptitude.git">git.debian.org/~dburrows/aptitude.git</a></div><div><br></div><div> or on the Web:</div><div><br></div>
<div><a href="http://git.debian.org/?p=users/dburrows/aptitude.git;a=summary">http://git.debian.org/?p=users/dburrows/aptitude.git;a=summary</a></div><div> </div><div> Unlike the branches I publish in the canonical repository, the feature</div>
<div>branches are frequently rebased, amended, rewritten, etc, and I delete</div><div>them when they're safely merged. i.e., don't try to base your work off of</div><div>them.</div><div><br></div><div> We could use a longer-lived branch for Qt work, but I'd prefer to just</div>
<div>merge it into the master branch in small chunks, ASAP.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">
> * In the patch that adds the configure tests:<br>
><br>
> - Please change AC_ARG_WITH to instead use AC_ARG_ENABLE. This<br>
> matches how GTK+ is enabled, and is more correct (--with- normally<br>
> lets you specify a particular installation of the library, and<br>
> we don't support that).<br>
><br>
> - Please support --disable-qt (or --without-qt). You should be<br>
> able to directly adapt the code that adds --enable-gtk and<br>
> --disable-gtk.<br>
><br>
<br>
</div>OK. I will change that. When writing this rules I had a dilemma which<br>
option will be better. But earlier I had written that there will be<br>
--with configuration switch, so I had stick with this.<br></blockquote><div><br></div><div><div> Just to be clear, my biggest reason for wanting to use AC_ARG_ENABLE</div><div>is that --with-* conventionally lets you say where the library is. I'd be</div>
<div>even happier if you supported that and used AC_ARG_WITH -- but I don't</div><div>know if it's worth the trouble at this stage.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">
> * Pointers/memory management:<br>
><br>
> - I see lots of bare pointers here. Could you give me a quick<br>
> summary of how memory management works in the Qt frontend?<br>
> aptitude strongly prefers smart pointers wherever it's feasible<br>
> to use them -- I guess widgets are probably managed by Qt itself,<br>
> but the Package object looks to me like a candidate for some<br>
> sort of smart pointer wrapper.<br>
><br>
<br>
</div>The GUI code will be managed just fine by the Qt machinery. The<br>
problem is with Filter and Package classes (to be fair Package class<br>
is really ugly now ;) ). I was planning to split this classes to e.g<br>
Package and PackageShared classes. All data will be available in<br>
PackageShared class which will be held in QSharedDataPointer object in<br>
Package classes. The data will be automatically destroyed when the<br>
last Package class will be deleted. Package class will be lightweight<br>
so they objects could be passes by value not by references. It works<br>
quite nice in Kadu messanger (Actually Kadu's solution is even more<br>
complicated, so I will need to simplify this. It is all documented<br>
here: <a href="http://www.kadu.net/~vogel/doc/html/group___storage.html" target="_blank">http://www.kadu.net/~vogel/doc/html/group___storage.html</a>)<br></blockquote><div><br></div><div> So, if I understand correctly, you intend to allocate shared Package</div>
<div>objects and pass smart pointers to them? Would you mind using</div><div>boost::shared_ptr so we can have consistency across frontends? (the</div><div>backend objects that use this pattern already use that wrapper, and I'd</div>
<div>rather not add even more types of smart pointers if we can avoid it).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">
> - Why are all of PackageDelegate's methods const when they modify<br>
> its state? Something seems wrong here. Is there a better way<br>
> to do this?<br>
><br>
<br>
</div>I have only reimplemented functions from QAbstractItemDelegate. They<br>
all are const there:<br>
<a href="http://doc.trolltech.com/4.6/qabstractitemdelegate.html" target="_blank">http://doc.trolltech.com/4.6/qabstractitemdelegate.html</a></blockquote><div><br></div><div> That would kind of tie your hands. And all the workarounds are really</div>
<div>no better IMO.</div></div><br><div> Daniel</div>