[KPhotoAlbum] Very experimental image-grouping patch
jkt at gentoo.org
Wed Nov 28 01:11:25 CET 2007
Paul Fleischer wrote:
> As i wrote in a comment to Bug 152438, I have created a very
> experimental system for grouping images in KPhotoAlbum.
Wow, looks good.
Some comments that I have at a first glance:
a) There's no need to provide addChildImages() function "in case it
might be useful in future"
b) You've used another whitespace convention, you should've used 4
spaces per indent
c) Some constructions like DB::ImageDB::instance()->info( selected()
); aren't safe
d) If I read the code correctly, the "detach from parent" feature takes
effect only when real children are shown. It might make sense that when
you invoke this action and only parent is selected, it would make all
its children orphaned again
e) Parent-child relations are allowed to be nested, not sure that's what
we want here
f) We use ClassName::foo() instead of ClassName::getFoo()
g) I think we don't want to decrease displayed number of images in
h) It'd be nice if there was a tooltip on each parent showing its children
i) No need for +class ImageInfoList; in DB/ImageInfo.h
j) If thumbnail view is invoked by "show child images", you should
update status bar with some status string (and consider removing parent
image from the list, or at least marking it with different color than
k) Diff of kphotoalbumui.rc shows unrelated formatting chnges
l) Do we have to use "child" attribute in XML DB? Isn't "parent" enough?
What happens if these two are inconsistent?
m) if( !( (*it) == newParentName ) ) in
MainWindow::Window::slotUseAsParent() is ugly
n) No need for +class QPixmapCache; in Window.h
o) KPA should ask in similar way it asks for "really reorder thumbnails"
p) Perhaps change that red rectangle indicating parents with some
That said, it looks like great job.
cd /local/pub && more beer > /dev/mouth
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 252 bytes
Desc: OpenPGP digital signature
Url : /mailman/pipermail/kphotoalbum/attachments/20071128/88ecf5bc/attachment-0001.pgp
More information about the KPhotoAlbum