[KPhotoAlbum] Latest patch

[Robert L Krawitz]

   From: "Jesper K. Pedersen" <blackie at blackie.dk>
   Date: Sun, 9 Jul 2006 13:03:00 +0200

   First of all, good work. Sorry for not responding earlier but my
   dad was hospitalized so I had to abrupt a holiday and rush home, so
   KPA suddenly got lower priority :-o

That's understandable :-(  Sorry to hear that, and I hope he's doing better.

   I have a few request I'd like you to look at before we finalize
   this, please read on.

   I've made so changes to the code in the meantime, so your patch
   doesn't commit cleanly, so please find attached your patch made
   against the current trunk.

OK, I'll take a look at it.

   | 1) A new option to permit smooth scaling vs. non-smoothed scaling
   |    (which is much, much faster, particularly with large images).  I
   |    think there's still an issue with image preloading sometimes taking
   |    priority over responding to a user command in at least some cases
   |    (so that sometimes there's a short pause that I don't think is
   |    strictly necessary), but image display is a *lot* faster if smooth
   |    scaling is turned off.

   Without having looked to much at the code yet, I assume this is only in the 
   viewer, and not for thumbnails, right?

Right now it applies everywhere; I could certainly change it.

   Being completely blind towards details uglyness (I really couldn't
   see that anything was wrong with the thumbnails back in the old
   days when I did not smoothscale them), can you tell me how much
   worse non smooth scale looks when viewing, and how much faster it
   is.

It's about 3x-4x faster, more so with smaller display sizes.  The
difference is visible if you know what to look for (it looks a little
bit like jpeg artifacts, although it isn't -- it generally looks
something like a grid of sharp detail).  How visible it is depends
upon the subject matter and the exact scaling required.

   | 2) A new viewer option to permit showing image size in the info box.
   good.

   | 3) A new viewer option to permit showing file name in the info box.

   With this turned on, we now get both vertical and horizontal
   scrollbars, which looks odd. It would be cool if you could look
   into why we get the vertical ones, they should not be needed,
   should they.  I think I'll make this non-shown by default, as I
   believe most KPA users won't need it.

That's fine.  I think the reason for the vertical scrollbar is that
the text display code tries to wrap the text to fit as best it can
within the display area.

   | 4) An option to allow specifying thumbnail size.

   Why would you want this? All you have to do is grab the thumbnail
   view with the middle mouse button and drag, to resize.

   If it simple is a matter of not having recognized this yet, and you
   agree that we do not need this options, then please remove this
   code from the patch.

In part, I hadn't recognized this.  There are also performance
implications; if you make the thumbnail size exactly match one of the
standard sizes, thumbnail loading will be faster.  I think it's a
useful option, but it's ultimately your call.

   | 5) A new command in the viewer to zoom pixel for pixel (if the image
   |    is larger than the viewer, of course :-( ) -- typing `=' will do
   |    this.

   great.

   | 6) Ability to scale to less than full screen.  This means that
   |    displaying pixel for pixel will always display pixel for pixel, in
   |    addition to the ability to zoom out beyond that point.

   And that was even an easy fix I see :-) It really makes me wonder
   why I ever put in that restriction.

   | 7) A new "standard size" setting, which can be one of three things:
   |
   |    * Full screen (the default, as today)
   |
   |    * Natural size (i. e. pixel for pixel)
   |
   |    * Natural size if possible (pixel for pixel if it would fit,
   |      otherwise full screen).
   |    In addition to being saved in the preferences, typing the `/' key
   |    at any time will display the image at "standard size".

   I don't really get it (tm), if I press the / on an image 100x120,
   shouldn't I see it this size, rather than scaled up? That doesn't
   seem to work for me.

It depends upon what you've set "standard size" to in the viewer
options.  The default setting is full screen, as it is today.  The
setting you're probably expecting is "Natural size if possible", which
is one of the options I added.

   If I choose to see things in standard view, will all subsequent
   images also be shown in standard view (I believe that would be the
   coolest).

Well, as it is they're always shown in whatever standard view the user
has chosen.  I think it might actually be cool to show all subsequent
images at the scaling last selected by the user.

   |    This isn't perfect.  In at least some cases it initially flashes
   |    the image at full size before rescaling it up or down.  Fixing this
   |    will require more extensive work.  In particular, doing most
   |    efficiently will require reorganizing the cache so that it actually
   |    keeps track of the image sizes rather than assuming that they're at
   |    the display size, and the cache code in general will require some
   |    work.  I probably won't have time to do this before I go back to
   |    work.

   Could you tell me what you do to see this flashing?

Set the standard size setting to Natural size, select several photos,
and view them.  The first one will be OK; subsequent ones won't.

   | 8) New options to skip forward and backward by 10, 100, and 1000
   |    images.  This was inspired by our 3500 photos from Alaska that
   |    we're trying to categorize.  We did some of them last night, and I
   |    want to restart in the middle (I know where we left off, but it's a
   |    pain to find it in the thumbnail view and then select everything
   |    from that point on).

   Again, this is only in the viewer, right?  If you have time, it
   would be cool if lots of the things in the viewers context menus
   could be put into submenus. At least on my laptop, the menu now is
   way to big.

Unfortunately, I don't know how to do this in Qt.  I agree it would be
useful.

   I at some point saw that "skip 1000 images backward" was disabled,
   but it seems random what menu item are disabled. I loaded three
   images, and I see all the skip entries enabled, except the 1000
   backwards.

I haven't see this.  I'm doing it from the keyboard, and I've never
had a problem.

   | 9) The display code checks if the stored size is -1 (i. e. KPhotoAlbum
   |    doesn't know what size it is), and fixes it up (and calls back to
   |    the viewer to update the image box).  This could happen in cases
   |    other than already-present thumbnails; for example, if you try to
   |    view images for which thumbnails aren't yet created.

   I trust you that this is correct.

Actually, this is code I'd like reviewed.

   | 10) A new option to specify whether or not to load RAW files if
   |    corresponding JPEG or TIFF files also exist.  Default is that it
   |    doesn't, which is a change from the current behavior.  This is
   |    useful with cameras that store both a raw image and a JPEG or
   |    TIFF.  Related to this, I've changed the new image finder code to
   |    pass the entire filename to canReadImage rather than just the file
   |    extension.  I believe this is the right thing to do because it
   |    allows the code to determine whether it actually can read the file
   |    or not.

   I've been tinkering with this issue for some time, without finding
   the time to do something real about it.  Lets just keep it in for
   now, but could you please make it off by default instead.

By "off" do you mean still load RAW images if there's a corresponding
JPEG image present?

   Finally, I loaded in a big image, and zoomed out of it till it was
   smaller than the screen, and suddenly my computer froze, could you
   please check if we end up creating huge pixmaps to show them really
   tiny, or what goes on here.

It looks like something like that's going on here.  I viewed a large
image and shrunk it, and each shrink step was slower than the
previous.  What's more, my KPhotoAlbum memory usage skyrocketed to
about 1.1 GB.

It looks like the immediate cause of the problem is in cropAndScale(),
specifically here (the kdWarning is debug code I put in):

        if ( _zStart != QPoint(0,0) || _zEnd != QPoint( _loadedImage.width(), _loadedImage.height() ) ) {
            _croppedAndScaledImg = _loadedImage.copy( _zStart.x(), _zStart.y(), _zEnd.x() - _zStart.x(), _zEnd.y() - _zStart.y() );
	    kdWarning() << "scaled image dimensions: "
			<< QPoint(_croppedAndScaledImg.width(),
				  _croppedAndScaledImg.height())
			<< endl;
        }

I'm not sure how best to fix this.  I'm trying a few ideas, but...