Feature/member picture fixed ratio; fixes #1717 #336
Conversation
gagnieray
force-pushed
the
feature/member-picture-fixed-ratio
branch
from
55e09b4
to
8c85738
Compare
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## develop #336 +/- ##
=============================================
- Coverage 44.31% 44.20% -0.12%
- Complexity 5868 5896 +28
=============================================
Files 142 142
Lines 23816 23901 +85
=============================================
+ Hits 10554 10565 +11
- Misses 13262 13336 +74
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
The success message is not returned when a picture is changed with drag and drop : galette/galette/lib/Galette/Controllers/AjaxController.php Lines 143 to 149 in ce67c16 I don't see how to fix this. Maybe it is not the only case ? 🤔 |
|
I'm not sure I understand, but nothing is meant to be returned at this point: flash messages are stored until the next call. After an ajax call, calling the As far as I cna see in current Anyway, displaying a success message is maybe useless, if error messages are displayed correctly. In case of success, the new image will just appear, I guess that's enough :) As of the other places it's called, I'm not sure all works, but this seems OK in |
Yes that's it. Sorry I shouldn't have used the word "return".
OK. So I didn't break anything 😅 Fine !
All right thanks ! 😉 |
Looking at First, in such cases, Here, messages are displayed using Error messages are rendered using That's why, with drag'n drop, only error messages are displayed properly. I don't really know what would be the best way to fix that. A quick fix would be to use FYI, I also discovered an issue in |
Forget about this one. I messed things up on my install trying to understand messages 🙄 |
galette/lib/Galette/Core/Picture.php
Outdated
| // calculate image size according to ratio | ||
| if ($cur_width > $cur_height) { | ||
| $h = round($w / $ratio); | ||
| if (!isset($cropping)) { | ||
| if ($cur_width > $cur_height) { | ||
| $h = round($w / $ratio); | ||
| } else { | ||
| $w = round($h * $ratio); | ||
| } | ||
| // calculate image size applying the default max size on the smallest side of the image | ||
| } else { | ||
| $w = round($h * $ratio); | ||
| if ($cur_width > $cur_height) { | ||
| $w = round($h * $ratio); | ||
| } else { | ||
| $h = round($w / $ratio); | ||
| } | ||
| } |
There was a problem hiding this comment.
I made these changes because, depending on the ratio defined in the settings and the ratio of the original image, cropped images could be smaller than the max size allowed (and 200px max already makes images in the public members gallery a bit blurry depending on screen's size).
But I found a case where cropping fails : when the uploaded image largest side's size is 200px 😞
Any suggestion on how to resolve this case ?
Would it be allowed to simply increase protected $max_width and protected $max_height values ? 😅
|
I have not yet tested current PR, I must admit I'm a bit lost; it's been awhile since I worked on pictures ;) I'll take a look later today or maybe tomorrow trying to understand all changes on the PR; there are few things in current proposition that sounds a bit "strange" to me (for example, you calculate cropping after ratio ia already calculated, maybe the solution would be to keep ratio code as is, but after cropping size calculations? - this is just an idea quickly reading the code). As of the issue with cropping failure if the image size is the max allowed one; it won't be fixed if minimum size is increased; but just moved, no? As of the size itself... I'm not against a highest minimal size, but it's finally displayed ~200px actually (keeping in mind 1000px image makes no sense in all cases). We already have issues on images size at display. As far as I've seen on demo:
Aspect ratio is kept in all cases; but on trombi, this may cause already display issues on some images. Also, if source image is smaller than 200px, this is currently displayed with same sizes :( I see no other usages (in main) of image resizing, but it's also used in Auto and ObjectsLend plugins. |
There's no hurry 😉
Yes! Cropping must be done before resizing 👍
Another drawback of increasing those values would be a highest database size cost as members' pictures are also stored in the database.
This one is more an "optimization" issue than a wrong behavior, no ?
As members' pictures are only stored in 1 size, to prevent cases where images are "stretched", they should be stored with the biggest size they can be displayed with. Then, in case of cards display (as in the trombi page), to prevent the columns from being larger than the pictures max width, the grid could be constrained with a Otherwise, it's not my intention to go so far but, the perfect optimizations for responsive images would be to provide the same image in different sizes in the
OK. Then a check on minimum image size is required too.
If I've done things correctly, it shouldn't impact other usages 😄 |
After checking... No actually 🤣 |
Good catch. On another hand, we can continue to limit size (width, height and length) and keep storing full size images. Nowadays databases are most of the time OK (we won't store very huge images).
Yes, I know about "fluid images"; this is the idea some designers use to explain why there are 5Mio background images present on some websites :D This is indeed not a problem currently for Galette.
Yes, indeed; and I guess that was the case in 0.9.x :)
That seems a good solution for now.
Yes, maybe the maximum image size would be the original one (I've made this change on ObjectsLends plugin wen I took the ownership a few years ago), and we could use cache to store generated thumbnails. But I think that's currently too much for 1.0.0.
Yes :) |
gagnieray
force-pushed
the
feature/member-picture-fixed-ratio
branch
from
d13c99c
to
8fa5c66
Compare
Then yes I agree, storing only members' picture in db is useless.
For which purpose then ?
🤣
In the case of members' pictures, for some kind of "privacy by design" considerations, I think it's a good idea no to keep the source image and rather resize it to the minimum size possible. But the behavior you introduced in ObjectsLends could be usefull for dynamic fields. Regarding using cache to store thumbnails, I agree 😉
The last forced push on this PR is better 😄 |
Keeping in DB you mean? The main one: do not change any existing code, nor remove tables in database :p
Probably, yes... I guess the better solution would be to introduce dynamic images (so display, stroage, etc can be managed properly). What has been done on objectslend is not a good solution out of the box, it's based on main Picture, and I've added a file stored thumbnail that is not really well thinked at the beginning :( The idea behind is probably what would be done in core, but current code should really not be used :D
This is certainly too much work for now; especially if database storage is removed (no schema change in 1.0.0; this will wait next release)
I propose we finalize the original idea behind this pull request; so I can release a RC2 "quickly", and I'll propose a rewrite of the picture class with all changes for next release. |
|
PS: tests are failing because website is currently down, see https://bugs.galette.eu/issues/1721 |
|
I've just tested; its seems globally OK. I'm not against the feature, but I'm not sure this is something that will be widely used. I do not get the point of placing the new pref in the cards tab; it probably should be moved to parameters (even if there is no good place in facts). Also, I did not really see issue with 200px images. If the issue is a landscape 200x125 image results in original image just being uploaded; well, that makes sense to me; there is nothing to resize/crop. Honestly; we're talking about member photos that are maybe almost never used, and Galette is not a software image program, I guess this kind of "issue" is not significant. Well, if I did not miss anything... :) Also, a little con: there are many new translations for this feature, this should be avoid (unless really necessary) after a RC. But since I'm pretty sure this won't be widely used, lack of translation won't be a blocker (and I do not want to postpone after this for next release). |
That's why I made it optional 😉 In cases where members use "strange" ratios or small pictures, I think it is a simple and efficient solution to ensure consistancy on the trombi page and member cards.
Because it also applies to member cards.
As the objective of this feature is to ensure every picture will have exactly the same size and ratio, I definitely think it is an issue. I've added minimum image size requirement when cropping is enabled : 9db53f6
I understand.
I don't think so.
All right! I'll stop adding strings until the next release 👍 |
I registered on weblate so I can take care of the translations added by this PR 😁 |
lol, great ;) BTW, please do not extract strings in pull requests; I use to do that on develop only (to avoid conflicts). Not a problem, I'll drop the commit when I'll rebase before final merging. |
Maybe, yes... But this currently impacts default images, not just PDF cards rendering; when all other options on this tab only concerns PDF cards :)
👍 so I guess the issue is resolved? Expect the change proposal I made on too small text; this is OK for me. If you're happy with current core feature, I'll merge soon. |
I will move de fields in the parameters tab then 😄
Yes 😁
Thanks ! Wait for a last commit regarding changes in the preferences 😉 |
closes #1717 Add cropping options in settings parameters tab Add cropping focus selection on member form Add cropping on resizeImage() Minimum image dimensions required from cropping Restore and extend drag and drop picture feature Clean CSS and template
trasher
force-pushed
the
feature/member-picture-fixed-ratio
branch
from
c3ab1a4
to
656a271
Compare
|
I've squashed all commits in one, waiting for tests to turn green, and I'll merge. I'll extract new strings once merge will be done, but we'll have to wait for translations.... Weblate looks on official repository which is still down currently :( |
No description provided.