Discussion:
[KPhotoAlbum] Patch to display image counts (loaded/total) during image load
Robert Krawitz
2017-10-23 22:43:05 UTC
Permalink
This patch displays the count of images loaded/total images to be
loaded while processing new images. This may be a matter of taste,
but I hate progress bars of this type that don't tell you what's left
to do.
--
Robert Krawitz <***@alum.mit.edu>

*** MIT Engineers A Proud Tradition http://mitathletics.com ***
Member of the League for Programming Freedom -- http://ProgFree.org
Project lead for Gutenprint -- http://gimp-print.sourceforge.net

"Linux doesn't dictate how I work, I dictate how Linux works."
--Eric Crampton
Johannes Zarl-Zierl
2017-10-24 20:54:26 UTC
Permalink
Hi Robert,

Thanks for the suggestion! I've replaced the percentage with the <num>/<total>
directly on the progress bar, though.
I think the percentage does not add any value once we show the image count.

Cheers,
Johannes

Reference: https://cgit.kde.org/kphotoalbum.git/commit/?
id=2f9afd39b68b64eef253adf77cf7c24d7c8371cf
Post by Robert Krawitz
This patch displays the count of images loaded/total images to be
loaded while processing new images. This may be a matter of taste,
but I hate progress bars of this type that don't tell you what's left
to do.
Robert Krawitz
2017-10-24 21:50:21 UTC
Permalink
Post by Johannes Zarl-Zierl
Hi Robert,
Thanks for the suggestion! I've replaced the percentage with the <num>/<total> directly on the progress bar, though.
I think the percentage does not add any value once we show the image count.
Agreed.
Post by Johannes Zarl-Zierl
Reference: https://cgit.kde.org/kphotoalbum.git/commit/?
id=2f9afd39b68b64eef253adf77cf7c24d7c8371cf
Post by Robert Krawitz
This patch displays the count of images loaded/total images to be
loaded while processing new images. This may be a matter of taste,
but I hate progress bars of this type that don't tell you what's left
to do.
--
Robert Krawitz <***@alum.mit.edu>

*** MIT Engineers A Proud Tradition http://mitathletics.com ***
Member of the League for Programming Freedom -- http://ProgFree.org
Project lead for Gutenprint -- http://gimp-print.sourceforge.net

"Linux doesn't dictate how I work, I dictate how Linux works."
--Eric Crampton
Robert Krawitz
2017-11-20 00:08:04 UTC
Permalink
Hi,
Sorry to bother you directly, but I try to register to the KPhotoAlbum mailing list one week ago and I didn't get any confirmation message. I think the message I send to the mailing list were block because I am not registered.
I am using KPhotoAlbum since 2009. I really enjoy this software but I have been disappointed every time I try to share my galleries.
I wanted something that keeps the tag and allow people I send the picture to browse the pictures by tag.
In 2012, I produce a first website which did the job. It was based on MySQL and PHP, so no way to have a very simple deployment.
Now I just produce a new version, in full Javascript. It can run locally or on a website, just copy past the good file and you are done.
* a script to export the KPhotoAlbum database to a JSON file.
* a website
I am usign the Debian Stable version of KPhotoAlbum (Version 4.5) so you may have produce some improvement I didn't get. On my code, there are a lot of things to improve, but if you already have something better, I won't spend my time on it.
Anyway if you are interested, here is a live demo : http://poivron-robotique.fr/Demo_KPA_export/
My code is here : https://github.com/Keuronde/KPhotoAlbum_Export
Interesting idea, this.

One little suggestion would be that if you select multiple values in
one category that they should be or'ed rather than and'ed. If you
select two people, most likely you're interested in photos with either
person rather than only photos with both.

Also, why export it to JSON, rather than just use the XML that already
exists?
--
Robert Krawitz <***@alum.mit.edu>

*** MIT Engineers A Proud Tradition http://mitathletics.com ***
Member of the League for Programming Freedom -- http://ProgFree.org
Project lead for Gutenprint -- http://gimp-print.sourceforge.net

"Linux doesn't dictate how I work, I dictate how Linux works."
--Eric Crampton
Johannes Zarl-Zierl
2017-11-21 21:21:38 UTC
Permalink
Hi,
I will try to use the reply-all function.
Can someone tell me if my email arrive in the mailing list ?
I've asked Jesper to manually subscribe you. He's quite a busy man, so please
bear with us if it takes some time.
I have create the issues on the github for the three improvement you
suggested. ( https://github.com/Keuronde/KPhotoAlbum_Export )
Cool.
Post by Robert Krawitz
Also, why export it to JSON, rather than just use the XML that already
exists?
My first question was to know if KPhotoAlbum had already something
like that for its export. So you confirm you don't have it yet ?
I don't know offhand. I know that there's an Android app that does
this, albeit by a very different mechanism.
No. The only export feature we have is for the .kim files (which are basically
a simpler version of an older index.xml format).

The Android app is something different - it connects to a running KPhotoAlbum
which acts as a server (using a binary protocol).
JSON seems simpler to manage for me, but I didn't realize that
javascript was obviously quite good at managing xml file. I will
have a look at it and try to get a version working directly with
KPhotoAlbum files.
Regarding the choice of JSON: seems reasonable to me.
Parsing a JSON format that was optimized for the task (and can be parsed
directly using JavaScript) is way easier and performs better(?) than using the
index.xml file format.

That's not to say that parsing the XML file format directly is not worthwhile
;-) But I don't think that would be a showstopper…
The only question is how stable the XML file format is (there are some
things I want to change for performance reasons). And note that there
are two XML file formats: the human-readable one and the compressed
one, which stores categories attached to images differently.
We generally try not to change the file format gratuitously.

I take it that you know about the file "documentation/database-layout.md" in
the git repo?

Cheers,
Johannes
Johannes Zarl-Zierl
2018-01-09 20:55:13 UTC
Permalink
Hello Samuel,

Just out of curiosity: how is your project coming along?

Btw. when looking at the workflow description on your github page, I thought
that maybe you could make things easier by using an export (.kim) file
instead.
On the plus side, KPA can automatically create the scaled images for you, and
the .kim file only contains the images that you exported.
Of course, the .kim file format is rather limited compared to the index.xml
file format (no positional tags, no image stacks, ...).

Cheers,
Johannes
Hi,
Post by Johannes Zarl-Zierl
Hi,
I will try to use the reply-all function.
Can someone tell me if my email arrive in the mailing list ?
I've asked Jesper to manually subscribe you. He's quite a busy man, so
please bear with us if it takes some time.
I just wanted to know if the subscribtion process was failed, in
progress or done.
Post by Johannes Zarl-Zierl
I have create the issues on the github for the three improvement you
suggested. ( https://github.com/Keuronde/KPhotoAlbum_Export )
Cool.
Post by Robert Krawitz
Also, why export it to JSON, rather than just use the XML that already
exists?
My first question was to know if KPhotoAlbum had already something
like that for its export. So you confirm you don't have it yet ?
I don't know offhand. I know that there's an Android app that does
this, albeit by a very different mechanism.
No. The only export feature we have is for the .kim files (which are
basically a simpler version of an older index.xml format).
The Android app is something different - it connects to a running
KPhotoAlbum which acts as a server (using a binary protocol).
Good, I am not inventing the wheel once more! Nice to know.
Post by Johannes Zarl-Zierl
JSON seems simpler to manage for me, but I didn't realize that
javascript was obviously quite good at managing xml file. I will
have a look at it and try to get a version working directly with
KPhotoAlbum files.
Regarding the choice of JSON: seems reasonable to me.
Parsing a JSON format that was optimized for the task (and can be parsed
directly using JavaScript) is way easier and performs better(?) than using
the index.xml file format.
That's not to say that parsing the XML file format directly is not
worthwhile ;-) But I don't think that would be a showstopper…
The only question is how stable the XML file format is (there are some
things I want to change for performance reasons). And note that there
are two XML file formats: the human-readable one and the compressed
one, which stores categories attached to images differently.
We generally try not to change the file format gratuitously.
I take it that you know about the file "documentation/database-layout.md"
in the git repo?
Well, no. I will have a look at it right now!
Post by Johannes Zarl-Zierl
Cheers,
Johannes
Regards,
Samuel
Samuel Kay
2018-01-09 21:12:00 UTC
Permalink
Hello Johannes,

My project is on standby, I have a gallery which looks good enough for me.
I don't think that my script would see any difference between the
index.xml and the .kim file.
I did create one personal gallery using the xml file given by the export
function, so it was probably the .kim file.

My next step would be to integrate that to KPhotoAlbum. I think it would
be nice to have that as the new default "html export" gallery.
I try to build KPhotoAlbum on Debian Jessie - I was to hard for me. Even
the cmake version is not compatible. I try, really, but I get stuck with
building "Meson" which is required at some building stage.
I also try to use a VM. I got several VM which did not start on my
computer (Fedora get stuck on the login screen for example).
Well now, I have a Debian Strech in a Working VM. And I try to get all
the dependencies. But it take me quite a long time and I haven't finish
yet.

Cheers,

Samuel
Post by Johannes Zarl-Zierl
Hello Samuel,
Just out of curiosity: how is your project coming along?
Btw. when looking at the workflow description on your github page, I thought
that maybe you could make things easier by using an export (.kim) file
instead.
On the plus side, KPA can automatically create the scaled images for you, and
the .kim file only contains the images that you exported.
Of course, the .kim file format is rather limited compared to the index.xml
file format (no positional tags, no image stacks, ...).
Cheers,
Johannes
Hi,
Post by Johannes Zarl-Zierl
Hi,
I will try to use the reply-all function.
Can someone tell me if my email arrive in the mailing list ?
I've asked Jesper to manually subscribe you. He's quite a busy man, so
please bear with us if it takes some time.
I just wanted to know if the subscribtion process was failed, in
progress or done.
Post by Johannes Zarl-Zierl
I have create the issues on the github for the three improvement you
suggested. ( https://github.com/Keuronde/KPhotoAlbum_Export )
Cool.
Post by Robert Krawitz
Also, why export it to JSON, rather than just use the XML that already
exists?
My first question was to know if KPhotoAlbum had already something
like that for its export. So you confirm you don't have it yet ?
I don't know offhand. I know that there's an Android app that does
this, albeit by a very different mechanism.
No. The only export feature we have is for the .kim files (which are
basically a simpler version of an older index.xml format).
The Android app is something different - it connects to a running
KPhotoAlbum which acts as a server (using a binary protocol).
Good, I am not inventing the wheel once more! Nice to know.
Post by Johannes Zarl-Zierl
JSON seems simpler to manage for me, but I didn't realize that
javascript was obviously quite good at managing xml file. I will
have a look at it and try to get a version working directly with
KPhotoAlbum files.
Regarding the choice of JSON: seems reasonable to me.
Parsing a JSON format that was optimized for the task (and can be parsed
directly using JavaScript) is way easier and performs better(?) than using
the index.xml file format.
That's not to say that parsing the XML file format directly is not
worthwhile ;-) But I don't think that would be a showstopper…
The only question is how stable the XML file format is (there are some
things I want to change for performance reasons). And note that there
are two XML file formats: the human-readable one and the compressed
one, which stores categories attached to images differently.
We generally try not to change the file format gratuitously.
I take it that you know about the file "documentation/database-layout.md"
in the git repo?
Well, no. I will have a look at it right now!
Post by Johannes Zarl-Zierl
Cheers,
Johannes
Regards,
Samuel
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum
Johannes Zarl-Zierl
2018-01-11 20:38:03 UTC
Permalink
Hello Samuel,
Post by Samuel Kay
My project is on standby, I have a gallery which looks good enough for me.
I don't think that my script would see any difference between the
index.xml and the .kim file.
I did create one personal gallery using the xml file given by the export
function, so it was probably the .kim file.
Good to know. Thanks for the update!
Post by Samuel Kay
My next step would be to integrate that to KPhotoAlbum. I think it would
be nice to have that as the new default "html export" gallery.
I thought you talked about inclusion as a script before? I'm open to
modernizing the HTML export, but running the pythong script from KPA directly
is bound to cause problems.

If you want to integrate the new JSON-based gallery into KPA as a replacement
for the current HTML export, we need to port the functionality of
parseXML_KPA.py into C++. On the plus side, that also removes the need for
parsing the .kim or index.xml file.

If you're still interested in going this way, I (and maybe Tobias?) can help
you.
Post by Samuel Kay
I try to build KPhotoAlbum on Debian Jessie - I was to hard for me. Even
the cmake version is not compatible. I try, really, but I get stuck with
building "Meson" which is required at some building stage.
I also try to use a VM. I got several VM which did not start on my
computer (Fedora get stuck on the login screen for example).
Well now, I have a Debian Strech in a Working VM. And I try to get all
the dependencies. But it take me quite a long time and I haven't finish
yet.
Debian Jessie is oldstable - I would not bother with that unless there is a
backport for the dependencies that you can use. I'm not even sure if the Qt
version on Jessie is recent enough for KPA.

On a recent Debian-based or openSUSE distro, the build instructions[1] should
work without problems.

Cheers,
Johannes


[1] https://userbase.kde.org/Building_KPhotoAlbum
Tobias Leupold
2018-01-11 21:15:38 UTC
Permalink
Post by Johannes Zarl-Zierl
If you're still interested in going this way, I (and maybe Tobias?) can help
you.
For sure, as far as my scanty C++ skills allow to ;-)
Samuel Kay
2018-01-18 20:43:59 UTC
Permalink
Hi,

I have finally update my main systems and have compile KPhotoAlbum.
I also have a look at the code of the HTMLExport function.

I will probably need some help with the C code. But now, I try to figure
out how cmake works.
I add a folder in themes to put my web files (not my script). Do I need
to manually edit the Cmakelists.txt file in "themes" and add one in my
folder ?

Samuel
Post by Tobias Leupold
Post by Johannes Zarl-Zierl
If you're still interested in going this way, I (and maybe Tobias?) can help
you.
For sure, as far as my scanty C++ skills allow to ;-)
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum
Johannes Zarl-Zierl
2018-01-18 21:14:06 UTC
Permalink
Hi,
Post by Samuel Kay
I add a folder in themes to put my web files (not my script). Do I need
to manually edit the Cmakelists.txt file in "themes" and add one in my
folder ?
Yes. Add a line "add_subdirectory(json_interactive)" (or whatever you called
your folder, and then add a CMakeLists.txt file to your folder, installing
your files.
It's probably easiest to take a look at the CMakeLists.txt from one of the
other theme folders and write yours accordingly.

Cheers,
Johannes
Samuel Kay
2018-01-21 21:25:43 UTC
Permalink
Hi,

I have compile KPhotoAlbum with some more files.
Now I am not far to start to code.
I have two question :

* How do I easily see the logs produced by KPhotoAlbum. (I have adjust
the log level to QtDebugMsg for kphotoalbum.HTMLGenerator in
HTMLGenerator/HTMLGenerator) Do you use a grep command?
* I don't understand from where the export function get its data. I
need the same input data. Where is the database on this line
Generator.cpp:80

ImportExport::Export exp( m_setup.imageList(), kimFileName( false ),

false, -1, ImportExport::ManualCopy,

destURL + QDir::separator() +
m_setup.outputDir(), true, &ok);

Otherwise, I will use the files in theme/*/kphotoalbum.theme to put some
more data, and tell if the JS database, the HTML index file or the HTML
image file should be generated (this part is nearly working).

Cheers,

Samuel Kay
Post by Johannes Zarl-Zierl
Hi,
Post by Samuel Kay
I add a folder in themes to put my web files (not my script). Do I need
to manually edit the Cmakelists.txt file in "themes" and add one in my
folder ?
Yes. Add a line "add_subdirectory(json_interactive)" (or whatever you called
your folder, and then add a CMakeLists.txt file to your folder, installing
your files.
It's probably easiest to take a look at the CMakeLists.txt from one of the
other theme folders and write yours accordingly.
Cheers,
Johannes
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum
Johannes Zarl-Zierl
2018-01-21 22:26:54 UTC
Permalink
Hello Samuel,
Post by Samuel Kay
* How do I easily see the logs produced by KPhotoAlbum. (I have adjust
the log level to QtDebugMsg for kphotoalbum.HTMLGenerator in
HTMLGenerator/HTMLGenerator) Do you use a grep command?
The easiest way is to set the environment variable QT_LOGGING_RULES:

export QT_LOGGING_RULES="kphotoalbum.HTMLGenerator=true"
kphotoalbum

Alternatively, you can also write a logging configuration file, but that's
usually not what you want. The details are explained in the api documentation
for QLoggingCategory[1]

For finding out the possible logging categories, take a look into the
Logging.cpp files in each source directory of kphotoalbum.

[1]: https://doc.qt.io/qt-5/qloggingcategory.html#configuring-categories
Post by Samuel Kay
* I don't understand from where the export function get its data. I
need the same input data. Where is the database on this line
Generator.cpp:80
ImportExport::Export exp( m_setup.imageList(), kimFileName( false ),
false, -1, ImportExport::ManualCopy,
destURL + QDir::separator() +
m_setup.outputDir(), true, &ok);
Otherwise, I will use the files in theme/*/kphotoalbum.theme to put some
more data, and tell if the JS database, the HTML index file or the HTML
image file should be generated (this part is nearly working).
The steps for the current HTML export are as follows:
1) The HTMLDialog is shown and fills the fields of a Setup object.
→ the dialog gets a list of selected images

2) Upon clicking "OK", the HTMLDialog calls Generator::generate with the setup
object and the image list

The ideal place to add your code is IMO the Generator class or a new class
like it. You could add a new class based on the Generator class and call that
one from HTMLGenerator::slotOk() instead of the current Generator class.

That way we can easily add an option to choose between the old and the new
export code, and the new Generator is not pushed into the corset of the old
Generator.


What is interesting for integrating your work:
- Does the current html export dialog / the Setup class contain all the
information that you need?
- Are there any fields that you don't need?


I hope that I could clear things up for you. Further questions are always
welcome btw. ;-)

Cheers,
Johannes
Samuel Kay
2018-01-24 20:42:22 UTC
Permalink
Hi Johannes,

Thanks, I have the logging working in my console !
I also have found, thanks to your indications where are the data.
XMLhandler.cpp help me to get the structure of what is in FileNameList.

I had started to structure the code:
I put 3 config field in the KPhotoAlbum.theme file :
* NeedHTMLIndexFile
* NeedHTMLImageFile
* NeedJSDatabase

These are read by the HTMLDialog.cpp which write it to the setup into
these three Boolean :
* m_generateHTMLIndexFile
* m_generateHTMLImageFile
* m_generateJSDatabase

And I add a condition to the generation of the HTML file depending of
these Boolean.
Now, I am planning to code the function to generate the JSDatabase and
the heavy work should be done.
I probably won't use all the field in the setup, but since I use the
same setup, I don't think I would be pertinent to remove it.

Right now, I don't have further questions.

Cheers,

Samuel
Post by Johannes Zarl-Zierl
1) The HTMLDialog is shown and fills the fields of a Setup object.
→ the dialog gets a list of selected images
2) Upon clicking "OK", the HTMLDialog calls Generator::generate with the setup
object and the image list
The ideal place to add your code is IMO the Generator class or a new class
like it. You could add a new class based on the Generator class and call that
one from HTMLGenerator::slotOk() instead of the current Generator class.
That way we can easily add an option to choose between the old and the new
export code, and the new Generator is not pushed into the corset of the old
Generator.
- Does the current html export dialog / the Setup class contain all the
information that you need?
- Are there any fields that you don't need?
I hope that I could clear things up for you. Further questions are always
welcome btw. ;-)
Cheers,
Johannes
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum
Johannes Zarl-Zierl
2018-01-24 21:01:51 UTC
Permalink
Hi Samuel,
Post by Samuel Kay
Now, I am planning to code the function to generate the JSDatabase and
the heavy work should be done.
I probably won't use all the field in the setup, but since I use the
same setup, I don't think I would be pertinent to remove it.
Right now, I don't have further questions.
Cool. Thanks for the update!

Cheers, and happy hacking,
Johannes
Samuel Kay
2018-01-28 08:47:22 UTC
Permalink
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
Samuel Kay
2018-01-28 13:09:35 UTC
Permalink
_______________________________________________
KPhotoAlbum mailing list
***@mail.kdab.com
https://mail.kdab.com/mailman/listinfo/kphotoalbum
Johannes Zarl-Zierl
2018-01-30 20:25:34 UTC
Permalink
Hi Samuel,

This week turned out to be a little more busy than I had hoped for.
Additionally, I'm going to attend FOSDEM this weekend, so there won't be much
time this weekend either.

Please don't be alarmed if I don't come around to look at your patches before
the weekend after the next one.

Cheers, and thanks for the patch-set!
Johannes
Samuel Kay
2018-01-31 19:04:38 UTC
Permalink
Hi Johannes,

Thank you for these news.
I can wait few weeks, no problems.

Cheers,

Samuel
Post by Johannes Zarl-Zierl
Hi Samuel,
This week turned out to be a little more busy than I had hoped for.
Additionally, I'm going to attend FOSDEM this weekend, so there won't be much
time this weekend either.
Please don't be alarmed if I don't come around to look at your patches before
the weekend after the next one.
Cheers, and thanks for the patch-set!
Johannes
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum
Samuel Kay
2018-01-31 19:25:18 UTC
Permalink
Hi,

Since I did a git rebase, I haven't done a "cmake ..". Now I did, and I
have build error. Here is my last commit from the distant git :

commit b6ff0e9da74ec26b35f3c54e6e58321c528e140f
Author: l10n daemon script <***@kde.org>
Date: Tue Jan 23 03:17:06 2018 +0100

GIT_SILENT made messages (after extraction)

And here is my build error (yeah, french interface, I know...) :

:~/Projets/KPhotoAlbum/build$ make
[ 1%] Automatic moc for target kphotoalbum
[ 1%] Built target kphotoalbum_automoc
[ 1%] Updating version header.
-- Setting version information to v5.3-11-ge309c920-dirty...
[ 1%] Built target UpdateVersion
make[2]: *** Aucune règle pour fabriquer la cible «
*/usr/lib/x86_64-linux-gnu/libmarblewidget-qt5.so *», nécessaire pour
« kphotoalbum ». Arrêt.
CMakeFiles/Makefile2:68 : la recette pour la cible
« CMakeFiles/kphotoalbum.dir/all » a échouée
make[1]: *** [CMakeFiles/kphotoalbum.dir/all] Erreur 2
Makefile:138 : la recette pour la cible « all » a échouée
make: *** [all] Erreur 2


I do have a file */usr/lib/x86_64-linux-gnu/libmarblewidget-qt5.so.25*
but**no file**
*/usr/lib/x86_64-linux-gnu/libmarblewidget-qt5.so

*I create a symlink and it works. But I don't know what to do to fix that...

Regards,

Samuel
Johannes Zarl-Zierl
2018-01-31 21:31:53 UTC
Permalink
Post by Samuel Kay
I do have a file */usr/lib/x86_64-linux-gnu/libmarblewidget-qt5.so.25*
but**no file**
*/usr/lib/x86_64-linux-gnu/libmarblewidget-qt5.so
*I create a symlink and it works. But I don't know what to do to fix that...
You've installed libmarble, but not libmrble-dev,

@Tobias: is there already something of your marble branch in master?

HTH,
Johannes
Tobias Leupold
2018-02-01 12:03:18 UTC
Permalink
Post by Johannes Zarl-Zierl
@Tobias: is there already something of your marble branch in master?
No, it only lives in the marble branch. Probably some dependency of libkface?
I didn't merge any change to master yet!
Johannes Zarl-Zierl
2018-02-12 23:00:02 UTC
Permalink
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
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
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
Samuel Kay
2018-02-15 20:19:19 UTC
Permalink
Hi Johannes

I will try to correct most of the issues below.
but :
1b : I follow the instruction on the web site, I will try to group the
patchs but I have never that before. I may need some time to find the
good git commands
Otherwise, I think I will be able to provide you some cleaner patchs.

Thank you for having review my code.

Cheers,

Samuel
Post by Johannes Zarl-Zierl
Hi Samuel,
I did a first proper sweep over your patches. Apologies in advance for being
picky ;-)
(1) Patch format
These issues are more a formality, but make my life as reviewer a little
1a. Please use spaces for indentation.
1b. Group patches in a semantic way.
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.
[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
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
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
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum
Samuel Kay
2018-03-08 20:57:54 UTC
Permalink
Hi,

Here are my patches.
Sadly, I haven't find a way to correct :
* 1b. Since my code was written, I what to do to correct that
* 3a. I didn't find any occurrence. In your example, I wanted to
declare the variable outside the loop since they will be used in the
loop and after. Should I declare them in the loop ?
* 3b. I find some occurrences but not a lot. I hope I got all of them.

I manage 2 and 5 quite easily.

Cheers,

Samuel
Post by Samuel Kay
Hi Johannes
I will try to correct most of the issues below.
1b : I follow the instruction on the web site, I will try to group the
patchs but I have never that before. I may need some time to find the
good git commands
Otherwise, I think I will be able to provide you some cleaner patchs.
Thank you for having review my code.
Cheers,
Samuel
Post by Johannes Zarl-Zierl
Hi Samuel,
I did a first proper sweep over your patches. Apologies in advance for being
picky ;-)
(1) Patch format
These issues are more a formality, but make my life as reviewer a little
1a. Please use spaces for indentation.
1b. Group patches in a semantic way.
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.
[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
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
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
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum
Samuel Kay
2018-10-20 22:35:01 UTC
Permalink
_______________________________________________
KPhotoAlbum mailing list
***@mail.kdab.com
https://mail.kdab.com/mailman/listinfo/kphotoalbum
Johannes Zarl-Zierl
2018-10-22 19:54:40 UTC
Permalink
Hi Samuel,
A patch reorganizing the media in the HTML gallery
(actually, everything is in the root folder and you can't find
easily the index.html file)
A patch allowing to activate or deactivate the generation of
the files used in the gallery (the index files and the html
image files)
A patch allowing to create a JS database when exporting an
HTML gallery
A patch with my gallery using the JS database ?
i will also correct the points Johannes gives me to correct as
much as I can.
This sounds great!

Thanks,
Johannes

Loading...