Bug 245380

Created attachment 49432 [details]
this version needs to alter the libkdcraw rexpandebox widget with the other patch below.

Usability improvement. The RExpanderBox widget gains a toolbox mode, where only one included RExpander is shown at any given time.

Created attachment 49433 [details]
Adds an exclusive mode to RExpander.

this is a patch to libkdcraw (needs to recompile libkdcraw, kipi-plugins and digikam for effect). adds a toolbox mode to RExpanderBox widget.

Created attachment 49468 [details]
rexpanderbox based ui to bw filter, only digikam modified

I wasn't happy with my changes to libkdcraw and considered my changes to rexpanderbox too far reaching. So I thought this over and finally opted to limit my scope to digikam itself. I therefore created a subclass of rexpander named rexpanderexclusive (in libs/widgets/common) that belongs to the digikam namespace and basically adds a toolbox mode to the base widget. I added my subclass to the necessary CMakeLists. The RExpanderBoxExclusive class is just a toolbox-mode RExpanderBox by default, and can be reverted to a classic multi-pane behaviour at the flick of a switch with the provided setIsToolBox(bool) method.

Manuel,

"The dynamic curvesbox replaces the non editable histogram at the top of the toolbox"

No. the original histogram view is not static. It represent the modified image histogram. It used to check if change provide an under/over exposed picture. Please restore it.

Gilles Caulier

(In reply to comment #4)
> Manuel,
> 
> "The dynamic curvesbox replaces the non editable histogram at the top of the
> toolbox"
> 
> No. the original histogram view is not static. It represent the modified image
> histogram. It used to check if change provide an under/over exposed picture.

Yes, I understood this first time we talked. That's the reason why I added a method Curvebox::updatedData() to refresh the histogram in the background of the curve to reflect the modified image histogram, so you know if your actions are going to over/underexpose the modified image. The data I'm not showing is the original image static histogram now. 

> Please restore it.

No problem, I do it.

Manuel

Created attachment 49490 [details]
bwsepiafiltertool simple UI transformation from KTabWidget to RExpanderBox

following on cg request.

Cheers,

Manuel

Some feedback after to test :

1/ in iconview, with B&W preview icons, if you select one item to render preview, you is unable to compare preview together, because selected one is grayed by selection mode. If fact a selected item must be grayed around the thumbnail (text + border), but not the image as well.

2/ your expander box running in exclusive mode is very interesting. I will add it to libkdcraw later KDE 4.5.0 release (it will still back in digiKam core for a while, to not break binary compatibility). But there is a little problem there : if you unexpand current item, the vertical layout is broken, an a lots of space is added between all expander items. Sound like a stretch is not added to the bottom of layout to move all items to the top.

3/ i don't like to see the same icon for all expander item. please set different image for each one. It will set it more suitable for end users. look icons installed from digiKam core to your computer

Gilles Caulier

Created attachment 49552 [details]
corrected following cg inputs.

Thank you very much for the feedback, this is very encouraging.

> 1/ in iconview, with B&W preview icons, if you select one item to render
> preview, you is unable to compare preview together, because selected one is
> grayed by selection mode. If fact a selected item must be grayed around the
> thumbnail (text + border), but not the image as well.

Done. A combination of stylesheet and Qicon role got this one.
 
> 2/ your expander box running in exclusive mode is very interesting. I will add
> it to libkdcraw later KDE 4.5.0 release (it will still back in digiKam core for
> a while, to not break binary compatibility). But there is a little problem
> there : if you unexpand current item, the vertical layout is broken, an a lots
> of space is added between all expander items. Sound like a stretch is not added
> to the bottom of layout to move all items to the top.

Yes but the stretch conflict with the previewlist and I could find no meaningful way to address this concern because until this mode can be backported to libkdcraw, the layout of the parent RExpanderBox is opaque.

I mitigated the issue by hardcoding a larger initial size for previewlist. It's not as clean as I wish, but I don't know how to do better.

> 3/ i don't like to see the same icon for all expander item. please set
> different image for each one. It will set it more suitable for end users. look
> icons installed from digiKam core to your computer

Done. I'm not a graphic artist, so I chose some icons I thought more or less fitting, new icons would be a better solution, but it does look better.

Cheers, Manuel

Done. Excelent !

Just a little remark : in preview icon view, when you start tool, it do not select the last used settings. In fact, tool apply it, but nothing is selected to icon view.

For icons, i will review it later, after to have applied your patch on svn trunk (digiKam 1.4.0)

Also, for digiKam 2.0.0, we use a separate branch, where i will fork from trunk libkdcraw, and integrate your expander patch directly.

For information, code is there. Warnings, it's unstable for the moment, do not use in production. It include all GSoC 2010 works, as Face detection, Image versioning, and reverse geolocation : 

http://websvn.kde.org/branches/extragear/graphics/

I hope that you will interested to patch more digiKam in the future. Your implementation is very suitable, very well written, and easily reviewable.

Gilles Caulier

(In reply to comment #9)

> Just a little remark : in preview icon view, when you start tool, it do not
> select the last used settings. In fact, tool apply it, but nothing is selected
> to icon view.

This most strange, and I'm completely lost here, because on my working copy the behaviour is correct, and the preview icon is shown selected. (QT 4.5.3 & KDE 4.3.3 with updated kdegraphics.)

> For icons, i will review it later, after to have applied your patch on svn
> trunk (digiKam 1.4.0)

Thank you.

> Also, for digiKam 2.0.0, we use a separate branch, where i will fork from trunk
> libkdcraw, and integrate your expander patch directly.

That would be much more convenient. I tried to settle the geometry interactions between my derived instance and the displayed objects from the outside, but I realised I would need to worm a way through the QObject tree to get inside the RExpanderBox main layout. Very bad (and bug-prone) practice, I scrapped my attempts.
 
> For information, code is there. Warnings, it's unstable for the moment, do not
> use in production. It include all GSoC 2010 works, as Face detection, Image
> versioning, and reverse geolocation : 
> 
> http://websvn.kde.org/branches/extragear/graphics/

I'll sure peek a look as soon as I get back in mid-August.
 
> I hope that you will interested to patch more digiKam in the future. Your
> implementation is very suitable, very well written, and easily reviewable.

Well this is much undeserved praise, and I hope I can find new areas where polishing efforts are needed :-)

Cheers, Manuel

>> Just a little remark : in preview icon view, when you start tool, it do not
>> select the last used settings. In fact, tool apply it, but nothing is selected
>> to icon view.

>This most strange, and I'm completely lost here, because on my working copy the
>behaviour is correct, and the preview icon is shown selected. (QT 4.5.3 & KDE
>4.3.3 with updated kdegraphics.)

I will be more precise : i i don't apply yet B&W conversion using new setting layout (using preview as icon view), the first item from icon view is used to render preview effect on canvas (this is correct), but the icon view item is not selected.

I don't have tested the case of B&W tool have been already used before.

Gilles

(In reply to comment #11)

> I will be more precise : i i don't apply yet B&W conversion using new setting
> layout (using preview as icon view), the first item from icon view is used to
> render preview effect on canvas (this is correct), but the icon view item is
> not selected.

I understand. I can't look at it now, but I let you know as soon as I can.

Manuel

Manual,

I can confirm that preview icon selection is not restored between tool session (in all case)

Gilles

(In reply to comment #12)
> (In reply to comment #11)
> 
> I can confirm that preview icon selection is not restored between tool session
> (in all case)

That's most annoying. I can't duplicate it here. I strongly suspect a behaviour change in QListWidget in Icon Mode between Qt 4.5 and Qt 4.6. I need to setup an up-to-date new development box before I can tackle this one, at the moment, I'm blind.

Manuel

Here i use Qt 4.6.2 from Mandriva 2010.1 open source edition :

digiKam version 1.4.0 (rev.: 1152961)
Exiv2 can write to Jp2: Yes
Exiv2 can write to Jpeg: Yes
Exiv2 can write to Pgf: Yes
Exiv2 can write to Png: Yes
Exiv2 can write to Tiff: Yes
Exiv2 supports XMP metadata: Yes
LibCImg: 130
LibExiv2: 0.20
LibJPEG: 80
LibJasper: 1.900.1
LibKDE: 4.4.3 (KDE 4.4.3)
LibKExiv2: 1.2.0
LibKdcraw: 1.1.0
LibLCMS: 119
LibPGF: 6.09.44
LibPNG: 1.2.43
LibQt: 4.6.2
LibRaw: 0.10.0-Beta2
LibTIFF: LIBTIFF, Version 3.9.2 Copyright (c) 1988-1996 Sam Leffler Copyright (c) 1991-1996 Silicon Graphics, Inc.
Marble widget: 0.9.3
Parallelized demosaicing: Yes
Database backend: QSQLITE
LibGphoto2: 2.4.9
LibKipi: 1.2.0

(In reply to comment #15)
Looks like I'm not up to date on several counts :-)

digiKam version 1.4.0 (rev.: 1154595)
Dématriçage parallélisé: Oui
Exiv2 peut écrire dans un fichier JP2: Oui
Exiv2 peut écrire dans un fichier JPEG: Oui
Exiv2 peut écrire dans un fichier PGF: Oui
Exiv2 peut écrire dans un fichier PNG: Oui
Exiv2 peut écrire dans un fichier TIFF: Oui
Exiv2 prend en charge les métadonnées XMP: Oui
LibCImg: 130
LibExiv2: 0.20
LibJPEG: 62
LibJasper: 1.900.1
LibKDE: 4.3.3 (KDE 4.3.3)
LibKExiv2: 1.1.0
LibKdcraw: 1.1.0
LibLCMS: 118
LibPGF: 6.09.44
LibPNG: 1.2.37
LibQt: 4.5.3
LibRaw: 0.10.0-Beta2
LibTIFF: LIBTIFF, Version 3.8.2 Copyright (c) 1988-1996 Sam Leffler Copyright (c) 1991-1996 Silicon Graphics, Inc.
Élément graphique Marble: 0.8.1
LibGphoto2: 2.4.3
LibKipi: 1.1.0
Moteur de base de données: QSQLITE

Created attachment 49629 [details]
final bug squashing

> Manual,
>
> I can confirm that preview icon selection is not restored between tool session
> (in all case)
>
> Gilles

I finally made a quick and crude Qt 4.6.3 install, enough to confirm this bug.

It's a Qt 4.x bug. In 4.5, the calculated icons are not displayed properly over the text in case this text is translated until the QListWidget is reset(), and 4.6 is even further broken, because QListWidget::reset() doesn't restore the selection anymore like it used to previously.

The attached patch works around this (see utilities/imageeditor/widgets/previewlist.cpp in void PreviewList::slotProgressTimerDone() at line 325).

Cheers, Manuel

Just few remarks with rexpanderboxexclusive implementation :

- no need to include rexpanderboxexclusive.moc and rexpanderboxexclusive.h. .moc file is enough.
- with private members, when no d private container is used, add a "m_" prefix.
- when it's possible set value returned by methods as const (ex: bool isToolBox() const;)

For more details about coding style, take a look in digiKam HACKING file.

Gilles Caulier

SVN commit 1156631 by cgilles:

apply patch #49629 from Manuel Viet to improve ergonomics of BW converter
BUGS: 245380


 M  +1 -0      CMakeLists.txt  
 M  +6 -6      imageplugins/color/autocorrectiontool.cpp  
 M  +1 -1      imageplugins/color/autocorrectiontool.h  
 M  +50 -43    libs/dimg/filters/bw/bwsepiasettings.cpp  
 M  +1 -1      libs/dimg/filters/bw/bwsepiasettings.h  
 M  +1 -0      libs/widgets/common/CMakeLists.txt  
 M  +53 -28    utilities/imageeditor/widgets/previewlist.cpp  
 M  +12 -14    utilities/imageeditor/widgets/previewlist.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1156631

SVN commit 1156638 by cgilles:

contrast adjustement have been dropped from the layout. Restore it in gui...
CCBUGS: 245380


 M  +6 -7      bwsepiasettings.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1156638

Manuel,

We have a little problem with B&W converter settings and icon-view used to host preview. This widget is used into Batch Queue Manager, and it do not render any preview, because we process more than one image, not only just one as in editor.

Before, when we have used a simple listview, settings been fine. Now it's full of empty space. It's very annoying (:=)))

The solution will to set a dummy image in Batch Queue Manager (as a photo colors chart for ex.) 

Also, i think that space between icon is very large and can be optimized. Look space between icon item title and top of next icon on the bottom. What do you think about ?

Gilles Caulier

Created attachment 49666 [details]
BQM and B&W converter settings screen-shot

Created attachment 49681 [details]
first try to patch icon view in BQM

Manuel,

This patch is supposed to render a default image icon in icon view with B&W tool is assigned to BQM. But it crash. 

Please take a look. Perhaps i'm in a wrong way to fix it.

Gilles Caulier

Manuel,

Do you take a look to my patch ?

Gilles

SVN commit 1162180 by cgilles:

- B&W converter for BQM : Add preview using default image preview from Oxygen. Fix crash due to a non initialized pointer.
Please, always set a pointer to zero : in GDB, you will see an use of null pointer which is wrong of course. An unitialized pointer as a dumy 
which is impossible to identify as wrong quickly...
- Add a new kde notifier event when editor as saved a file. This is usefull when you review RAW files in icon view and you edit item in editor. 
During a save operation, which can take a while, you can go back to icon view to continue to sort items before to perform RAW import. The notifier 
event will bring you when editor is ready to load a new item.
CCBUGS: 245380


 M  +7 -0      digikam/digikam.notifyrc  
 M  +37 -33    libs/dimg/filters/bw/bwsepiasettings.cpp  
 M  +9 -0      showfoto/showfoto.cpp  
 U             utilities/imageeditor/editor/editorwindow.h  
 M  +9 -0      utilities/imageeditor/editor/imagewindow.cpp  
 M  +2 -1      utilities/imageeditor/widgets/previewlist.cpp  
 M  +2 -0      utilities/queuemanager/basetools/color/bwconvert.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1162180

Oups. I'm just back from 2 weeks of holidays without internet access and I discover your messages.

I'll look into this asap.

Cheers,

Manuel