Hi to everybody and especially to DanielADK (creator/maintainer of the plugin),
I just see that you published [extension by DanielADK] Modern formats
It's an interesting idea and I have some feedbacks.
1) directory naming
You should have fixed directory name (equivalent to its ID) for your plugin. Not "piwigo-modern-formats" or "modern_formats". If not, you will soon encounter conflicts with people having the plugin twice with the 2 names... This is why you can often find this code in Piwigo plugins :
if (basename(dirname(__FILE__)) != 'user_custom_fields')
{
add_event_handler('init', 'ucf_plugin_error');
function ucf_plugin_error()
{
global $page;
$page['errors'][] = 'User Custom Fields plugin folder name is incorrect, uninstall the plugin and rename it to "user_custom_fields"';
}
return;
}The directory name must be consistent with maintain class :
class user_custom_fields_maintain extends PluginMaintain
=> I strongly recommend you force the directory name to be "modern_formats" and have the class name "modern_formats_maintain" in maintain.class.php
2) Git
Than you for having a Git repository on Github. It makes things so much easier for us to review.
3) translations
You seem to have generated 19 languages at once? AI powered?
4) WebP as original or as preview/representative
You have chosen to convert the original file from JPG/PNG to WebP. I agree it may save a lot of space (especially with PNG files). I also find it a bit dangerous. Have you thought about generating a "pwg_representative" as WebP, instead of changing the original file? I'm just thinking out loud. OK it would keep the original PNG (thus its weight on disk) but all derivatives (multiple sizes) would be in WebP, making them lighter for browser display.
By the way, we're preparing a strong refresh for piwigo.org website, and all pictures will be WebP.
Offline
Thanks for the detailed review. Point by point:
1) Directory naming
Fixed in 1.2.0a. The folder is now enforced as "modern_formats" (= the plugin id): the maintenance class is "modern_formats_maintain", and main.inc.php refuses to run under any other folder name. I reproduced exactly the conflict you describe — with two folder names the plugin's (global) classes even throw a fatal "Cannot redeclare class". I've also renamed the GitHub repo to "modern_formats" so a plain "git clone" produces the correct folder, and the release ships a modern_formats.zip that extracts to the right place.
2) Git
Glad it helps the review.
3) Translations
Yes — the 19 extra languages are ai translated; I'm a solo dev and wanted initial coverage. I maintain en_UK and cs_CZ myself; the rest are best-effort and I'd genuinely welcome native corrections, or I'm happy to move them to Piwigo's translation platform if that's the preferred path.
4) WebP as original vs preview/representative
I did consider the representative route during design. For a displayable image, Piwigo generates derivatives from the original, not from a representative: in SrcImage (derivative.inc.php), when the original's extension is in picture_ext, rel_path = the original path (IS_ORIGINAL); representative_ext only becomes the derivative source for non-picture originals (video/PDF/RAW). So a WebP pwg_representative on a JPEG would be ignored and the derivatives would still be generated from the JPEG. Converting the original is the only native way to get WebP derivatives, which is why I went that way — keeping the source in a backup folder by default to address the destructiveness.
Offline
DanielADK wrote:
1) Directory naming
[...]
I've also renamed the GitHub repo to "modern_formats" so a plain "git clone" produces the correct folder, and the release ships a modern_formats.zip that extracts to the right place.
Not what I see on piwigo.org/ext :
ext-1084/rev-10541/piwigo-modern-formats_1.2.0.zip
ext-1084/rev-10542/piwigo-modern-formats_1.2.0a.zip
[... 3 minutes later...]
but then I test the "content" of the zip file:
$ unzip -t ext-1084/rev-10542/piwigo-modern-formats_1.2.0a.zip | head
Archive: ext-1084/rev-10542/piwigo-modern-formats_1.2.0a.zip
testing: modern_formats/ OK
testing: modern_formats/.gitattributes OK
testing: modern_formats/language/ OK
testing: modern_formats/language/el_GR/ OKI recommend you change the "Archive name" to "modern_formats_%.zip"
It will work if you install the plugin with the plugin manager in your Piwigo administration, because Piwigo renames the directory whatever the zip name.
3) Translations
Yes — the 19 extra languages are ai translated; I'm a solo dev and wanted initial coverage. I maintain en_UK and cs_CZ myself; the rest are best-effort and I'd genuinely welcome native corrections, or I'm happy to move them to Piwigo's translation platform if that's the preferred path.
It's nice to have the plugin directly in so many languages ! I've checked the French and I would correct almost nothing :-D
For me it's the first time a plugin developer does this. I like the fact that it's immediately available in my language. I also think adding your plugin translate.piwigo.org will give the possibility to fix strings by native speakers.
4) WebP as original vs preview/representative
[...] when the original's extension is in picture_ext, rel_path = the original path (IS_ORIGINAL); representative_ext only becomes the derivative source for non-picture originals (video/PDF/RAW).
OK, that's what I "feared". I wonder why we don't change Piwigo core : if piwigo_images.representative_ext is not null (in the database), then we use it to create derivatives. Then the derivatives would be WebP files. I need to understand why it was implemented that way.
By the way, what does the "keep a backup" option do?
Offline
Anyway, this plugin is a very good idea in my opinion, uses AI smartly and has a Github repository. Well done DanielADK :-D
Offline