[KPhotoAlbum] Very experimental image-grouping patch

[Paul Fleischer]

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

Oups :-)

> c) Some constructions like DB::ImageDB::instance()->info( selected()[0]
> ); aren't safe

So true.

> 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()

Oups :-)

> 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
> children)

Agreed.

> k) Diff of kphotoalbumui.rc shows unrelated formatting chnges

Oups :-)

> 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

Agreed.

> n) No need for +class QPixmapCache; in Window.h

Oups :-)

> o) KPA should ask in similar way it asks for "really reorder thumbnails"

Good idea.

> 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
> alternatives

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
image grouping.