[KPhotoAlbum] Very experimental image-grouping patch
paul at xpg.dk
Wed Nov 28 09:35:10 CET 2007
2007/11/28, Jan Kundrát <jkt at gentoo.org>:
> 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.
Thank you :-)
> 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"
Actually, it was used in a previous version. I just forgot to remove it, thanks.
> 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
Sounds like a good idea.
> e) Parent-child relations are allowed to be nested, not sure that's what
> we want here
I've given this some thought and I'm pretty sure that nesting is not wanted.
> f) We use ClassName::foo() instead of ClassName::getFoo()
> g) I think we don't want to decrease displayed number of images in
> browser widget
I don't quite follow?
> h) It'd be nice if there was a tooltip on each parent showing its children
Indeed. Actually, my initial thought was to make a new interface for
children browsing, rather than using ThumbnailWidget. I went away from
that idea, as it would duplicate functionality. However, having the
children show up in a tooltip is a very good idea.
> i) No need for +class ImageInfoList; in DB/ImageInfo.h
Indeed not. The first instance of the patch had children as
ImageInfoList, rather than QStringList.
> 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?
The advantage by having the child attribute, is that we avoid having
to do name-lookup for the parent on each image-load. At least that was
my initial idea. But inconsistencies will mess things up badly, so I
agree that having only parent would be the best.
> 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
> overlayed icon?
Yep, that is what I had in mind.
> q) Add some actions/shortcuts to ImageView for cycling through the
Could you elaborate?
> r) Right now, it's possible to create a cycle which prevents images to
> be accessed at all (can be fixed by allowing only one level nesting)
Yep, I would say that multi-level nesting shouldn't be allowed.
> That said, it looks like great job.
Thank you very much for your comments. I'll fix the smaller issues as
soon as I get a chance, and create a new patch. Also, I'll try to
remove the child-stuff from XMLDB, as it's both ugly and, as you
mention, can lead to inconsistencies.
I hope more people will voice their opinion about this approach to
More information about the KPhotoAlbum