[PATCH 0/2] Capture and display frame thumbnails in qapitrace

[Dan McCabe]

On 03/01/2012 07:34 PM, Zack Rusin wrote:
> On Thursday, March 01, 2012 12:04:26 PM José Fonseca wrote:
>> I'll defer to Zack comments on the gui implementation.
> fwiw, i think the idea is pretty good. i really don't want the gui to do this
> by default though. i think it might be better if we just show a larger
> thumbnail in the tooltip rather than 16x16 icon in the list, because the
> latter is unlikely to mean anything to anyone, but i guess we can figure that
> out later.
One of the optimizations I had in mind was having glretrace generate 
snapshots only for those frames that are visible in the GUI window (and 
that haven't been previously generated). Doing so should make everything 
more responsive. But I was saving that optimization until after the 
basic feature worked.
> besides that from the code perspective:
> a) the paint method isn't right, you should never do scaling in the paint
> method, generate thumbnails before. plus please leave a little some margin
> when drawing the thumbnail,
Will do.
> b) you should use camel case for all new code,
Will do.
> c) please do not pass constant object to methods by value. also for the
> qlist<qimage>  since we don't steal ownership of it i'd prefer if you passed it
> by constant reference instead of a pointer (the fact that you're leaking it
> right now is a good reason why :) ).
Will do. I think I understand your suggestion, but I'll get back to you 
if I need clarification.
> d) please don't abbreviate words (e.g. use snapshots instead of snaps)
Will do.
> e) for the changes to propagate the model needs to be notified. currently only
> calls are dynamic so we only do it for them. the way it's done is that
> ApiTrace object emits the changed(ApiTraceCall*) signal which is being
> captured by the ApiTraceModel::callChanged method which emits the
> QAbstractItemModell::dataChanged method which causes the view to update the
> given items. you should probably change the ApiTrace::changed to use
> ApiTraceEvent instead of ApiTraceCall and cast it appropriately when used.
I will investigate that suggestion further. I may get back to you if I 
need clarification.
>
> z

Thanks for the feedback.

cheers, danm