[KPhotoAlbum] KPhotoAlbum - mailing list and export suggestion with code

Johannes Zarl-Zierl johannes at zarl-zierl.at
Mon Feb 12 23:00:02 GMT 2018


Hi Samuel,

I did a first proper sweep over your patches. Apologies in advance for being 
picky ;-)

Here are some hints for improvement:

(1) Patch format
These issues are more a formality, but make my life as reviewer a little 
easier:
1a. Please use spaces for indentation.

1b. Group patches in a semantic way.
E.g.:
Patch 1: Add theme files for js_export
Patch 2: Implement js database export
etc.


(2) Licenses
Since you include a third-party JavaScript library, you should also add their 
license file to the theme (and copy it when exporting).


(3) Coding conventions
3a. Please declare variables "as late as possible" (usually on first use). 
This improves readability.
seen here:
[Patch 2 -> "QString Images_data, Relations, Categories;"]

3b. Prefer "if (!condition)" over "if (condition == false)"


(4) TODOs
You introduced a few TODOs/question comments in the new code. I didn't have 
time yet to consider them in context. I'll comment on those when I've had the 
time…


(5) boolean values in kphotoalbum.theme file
There's already a boolean option "Default". For consistency's sake, please use 
"true" and "false" for the new keys as well.


So far, that's all I could find.

Cheers and thanks!
  Johannes






On Sonntag, 28. Jänner 2018 14:09:35 CET Samuel Kay wrote:
> Hi,
> 
> Once more, here is the patch, after having done "git rebase", an after
> having corrected two issues :* Special char were not handle correctly* Only
> the categories selected by the user are displayed.So, the export should be
> working OK.
> 
> In KPhotoalbum, go to :* *File* => *HTML Export*.In the *Layout* tab, select
> the "*Dynamic JS*" theme.
> 
> For now, in the "Image Sizes" field, only the highest resolution will be
> used, but all selected resolution will generated.
> 
> Things that I may do :* I still want to put the photos in separated folder*
> Use the description an the title set by the user* Manage Video
> 
> Cheers,
> 
> Samuel
> 
> 
> Samuel Kay a écrit :
> 
> 
> Hi,
> 
> I have a working version of the JS export. There are a few things I want to
> improve : * Actually, KPhotoAlbum change the file extension of the resize
> image (ie: JPG => jpg). I don't understand why. But with last patch, the JS
> export is using the name given by KPhotoAlbum (not the original file name).
> * I don't use the categories that are selected by the user and I display
> every category in the search panel. * The output give a lot of file in one
> folder. User should search the index file. I think it would be nice to put
> the photos in separate folder (depending of their size) and other generate
> html file in another folder.
> 
> But right now, here are my patch!
> 
> Cheers,
> 
> Samuel




More information about the Kphotoalbum mailing list