<div class="gmail_quote"> (sorry about dropping the list -- I'm still figuring out the quirks of this</div><div class="gmail_quote">mail interface)</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Thu, Jul 8, 2010 at 1:05 PM, Piotr Galiszewski <span dir="ltr"><<a href="mailto:piotr@galiszewski.pl" target="_blank">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@google.com" target="_blank">dburrows@google.com</a>>:<br>
<div>> On Thu, Jul 8, 2010 at 1:39 AM, Piotr Galiszewski <<a href="mailto:piotr@galiszewski.pl" target="_blank">piotr@galiszewski.pl</a>><br>
> wrote:Thanks for the informations. I am now rebasing and reworking my</div>
previous commits. I have planned to firstly write features, and later<br>
polish, cleanup and refactor code. As I see, this should change now,<br>
and I should try write code that is working and ready for merging? If<br>
so, I have to change my working plan a little, cause there is already<br>
small delay due to this code reworking. Please forgive me this delay<br>
;)<br></blockquote><div><br></div><div> Yeah, I know this will introduce some delays, at least for now. The thing</div><div>is, my experience is that deferring code quality often means you never</div><div>get around to it. And getting your code into good shape immediately has</div>
<div>some benefits:</div><div><br></div><div> - It's a lot easier in the long run to have code that's well-formatted, well</div><div> documented, etc, to start with, rather than going back and spending</div><div>
blocks of time doing nothing but documenting and formatting code (IMO).</div><div> - Making sure the design is sensible up-front can save you a *lot* of time</div><div> later; it's relatively cheap to change the design when you're first putting</div>
<div> it together, and a lot more expensive to do it after you've built a lot of</div><div> code.</div><div><br></div><div> Also, my experience with SoC (based on both times that I've been a</div><div>mentor) is that it goes better if your code is ready for submission from</div>
<div>early on. It ensures that we catch any differences of opinion about design,</div><div>syntax, etc, early on and not after you have three months worth of code</div><div>written. Plus, this is an educational program, and I think code reviews</div>
<div>serve both to help you write better code, and to give you some experience</div><div>with a more formal / professional style of development.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I have two other questions: Do you prefer smaller commits or one<br>
bigger, full feature commit for each branch? Also API docs should be<br>
written now, or it could wait a little. Also how precise they should<br>
be?</blockquote><div><br></div><div> My general preference (especially when I'm reviewing them) is for smaller</div><div>commits, balanced by my preference for commits that compile and work.</div><div>i.e., try to split your commits up into small chunks that will compile, pass</div>
<div>the unit tests, and run without major bugs (even if not all the features</div><div>work yet). So a good first (or maybe even second or third) stage for your</div><div>Qt frontend would be enough code to initialize Qt and display a blank window.</div>
<div><br></div><div> But despite what I said, feel free to use your common sense -- e.g., if it's</div><div>going to impose a lot of extra work to split a smallish commit into</div><div>two even smaller ones, don't bother.</div>
<div><br></div><div> As for API docs, please write them up-front. My experience since I adopted</div><div>this strategy has been that it's a really good way to flush out badly factored</div><div>code and other poor designs (such code is usually hard to document, or you</div>
<div>end up writing documentation that makes you feel so dirty and ashamed</div><div>that you go and fix it :) ). Also, it really helps when other people (ie, me) are</div><div>reviewing your code, so they know what was intended and what's a bug.</div>
<div><br></div><div> The API documentation should be written as if you're documenting a library,</div><div>because you are (every piece of aptitude is a library from the point of view</div><div>of the rest of the program). The most important thing to document is what</div>
<div>the meaning of each piece is and how it fits into the overall picture. Think</div><div>about what a reader who encountered your code without knowing what you</div><div>were doing would ask. e.g., consider the Filter class:</div>
<div><br></div><div><div> class Filter : public QObject</div><div> {</div><div> Q_OBJECT</div><div><br></div><div> QString _name;</div><div><br></div><div> public:</div><div> Filter(QString name) : _name(name) { }</div>
<div><br></div><div> const QString &name() { return _name; }</div><div><br></div><div> virtual bool acceptPackage(Package *package) { return true; }</div><div><br></div><div> signals:</div><div> void filterChanged();</div>
<div> };</div></div><div><br></div><div> Questions:<br> - What is a Filter?</div><div> - How does Filter relate to Package?</div><div> - Under what conditions is filterChanged emitted?</div><div><br></div>
<div>
Here's a stab at answering them, although I'm not sure that I got the right</div><div>answers:</div><div><br></div><div><font face="'courier new', monospace"> /** \brief A generic filter on packages for use with FiltersModel.<br>
</font></div><div><font face="'courier new', monospace"> *<br> * A Filter represents a predicate on Package objects, implemented</font></div><div><font face="'courier new', monospace"> * by acceptPackage(). Each filter has a name, which represents</font></div>
<div><font face="'courier new', monospace"> * something about the filter. I'm not sure exactly what. Maybe</font></div><div><font face="'courier new', monospace"> * it's intended to appear as a label when the user is selecting</font></div>
<div><font face="'courier new', monospace"> * filters?</font></div><div><font face="'courier new', monospace"> *<br> * Filters might change over time; if the logical value of a Filter</font></div>
<div><font face="'courier new', monospace"> * changes, it will emit filterChanged().</font></div><div><font face="'courier new', monospace"> *<br> * The main implementation of Filter is SearchFilter, which supports</font></div>
<div><font face="'courier new', monospace"> * simple string matches against several fields of a package.</font></div><div><font face="'courier new', monospace"> */</font></div>
<div><font face="'courier new', monospace"> class Filter : public QObject<br> {<br> Q_OBJECT</font></div><div><font face="'courier new', monospace"><br>
</font></div><div><font face="'courier new', monospace"> // \todo Shouldn't this be const?</font></div><div><font face="'courier new', monospace"> QString name;</font></div>
<div><font face="'courier new', monospace"><br></font></div><div><font face="'courier new', monospace"> public:</font></div><div><font face="'courier new', monospace"> /** \brief Create a new filter with the given name. */</font></div>
<div><font face="'courier new', monospace"> Filter(const QString &_name) : name(_name)<br> {<br> }<br><br></font></div><div><font face="'courier new', monospace"> /** \brief Retrieve the name of this filter. */<br>
const QString &get_name() const { return name; }</font></div><div><font face="'courier new', monospace"><br></font></div><div><font face="'courier new', monospace"> /** \brief Test whether the given package matches this filter</font></div>
<div><font face="'courier new', monospace"> * and return \b true if it does.</font></div><div><font face="'courier new', monospace"> *<br> * The default implementation of this method matches all packages.</font></div>
<div><font face="'courier new', monospace"> */</font></div><div><font face="'courier new', monospace"> virtual bool acceptPackage(const Package *package)<br>
{<br> return true;</font></div><div><font face="'courier new', monospace"> }</font></div><div><font face="'courier new', monospace"><br></font></div>
<div><font face="'courier new', monospace"> signals:</font></div><div><font face="'courier new', monospace"> /** \brief Emitted when this filter is changed to match a different</font></div>
<div><font face="'courier new', monospace"> * set of packages.</font></div><div><font face="'courier new', monospace"> */</font></div><div><font face="'courier new', monospace"> void filterChanged();</font></div>
<div><font face="'courier new', monospace"> };</font></div><div><font face="'courier new', monospace"><br></font></div><div><font class="Apple-style-span" face="arial, helvetica, sans-serif"> I have some more design notes on this particular piece of</font></div>
<div><font class="Apple-style-span" face="arial, helvetica, sans-serif">code, but I'm waiting to verify </font><font class="Apple-style-span" face="arial, helvetica, sans-serif">that it's OK for me to </font><span class="Apple-style-span" style="font-family: arial, helvetica, sans-serif; ">post some</span></div>
<div><span class="Apple-style-span" style="font-family: arial, helvetica, sans-serif; ">example code I wrote at work.</span></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
> So, if I understand correctly, you intend to allocate shared Package<br>
> objects and pass smart pointers to them? Would you mind using<br>
> boost::shared_ptr so we can have consistency across frontends? (the<br>
> backend objects that use this pattern already use that wrapper, and I'd<br>
> rather not add even more types of smart pointers if we can avoid it).<br>
><br>
<br>
</div>Not exactly, but yes. I will try to use boost::shared_ptr instead of<br>
it. Should it be done during reintroduction of Package class, or it<br>
can wait? Probably I will also create package_manager class for<br>
managing packages. Reading cache in model is not perfect solution and<br>
probably it will be problematic in the future<br></blockquote><div><br></div><div> Is package_manager some sort of a global storage pool of Packages,</div><div>then? Could you maybe call it package_pool instead to avoid naming</div>
<div>confusion?</div><div><br></div><div> I don't really understand the whole purpose of the Package object.</div><div>Is the goal here to save memory and CPU time by only copying package</div><div>information into QStrings once per package?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
>><br>
>> > - 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>
>> 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><br>
><br>
> That would kind of tie your hands. And all the workarounds are really<br>
> no better IMO.<br>
<br>
</div>It is a normal behavior of this classes, so we have to live with this<br>
;) Qt Model Framework is really nice and powerful solution and there<br>
are no better options for Qt.<br></blockquote><div><br></div><div> Yeah. Um, my language above was a little ambiguous, so I just wanted to</div><div>say clearly that I agree that you have to do things like this given how Qt works. </div>
<div><br></div><div> Daniel</div></div>