Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve diagnostics with count of thumbs that can be regenerated #2200

Merged
merged 6 commits into from Jan 16, 2024
Merged

Improve diagnostics with count of thumbs that can be regenerated #2200

merged 6 commits into from Jan 16, 2024

Conversation

ildyria
Copy link
Member

@ildyria ildyria commented Jan 13, 2024 •
edited

@LudovicRousseau
Would you be able to test it on your install ?
https://patch-diff.githubusercontent.com/raw/LycheeOrg/Lychee/pull/2200.patch < here is the patch.

Locally I get

Info: Found 5 small that could be generated
       You can use `php artisan lychee:generate_thumbs small` to generate them.

@ildyria ildyria self-assigned this Jan 13, 2024
@ildyria ildyria requested a review from a team January 13, 2024 22:21
@ildyria ildyria added the Review: easy Easy review expected: probably just need a quick to go through. label Jan 13, 2024
@ildyria ildyria added this to the 5.0.4 milestone Jan 13, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 13, 2024 •
edited

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (b8ace78) 86.35% compared to head (7fe61e8) 86.03%.
Report is 4 commits behind head on master.

Additional details and impacted files

@LudovicRousseau
Copy link

LudovicRousseau commented Jan 14, 2024 •
edited

Application of the patch fails. I do not have a phpstan.neon file. So I skipped this file.
In diagnostics I now get:

        Info: Found 610 medium that could be generated
              You can use `php artisan lychee:generate_thumbs medium` to generate them.

I then run: php artisan lychee:generate_thumbs medium that creates some new medium images.
In diagnostics I then get:

        Info: Found 545 medium that could be generated
              You can use `php artisan lychee:generate_thumbs medium` to generate them.

I then run: php artisan lychee:generate_thumbs medium that creates some new medium images.
In diagnostics I then get:

        Info: Found 501 medium that could be generated
              You can use `php artisan lychee:generate_thumbs medium` to generate them.

etc...

I am now at Found 461 medium that could be generated.

Maybe the command could generate ALL the medium images?

Since I started the php artisan ... from my shell the files are created as my user rousseau instead of www-data.
In diagnostics I get:

Warning: /home/rousseau/Serveurs_web/images.xxxx.fr/Lychee/public/uploads/medium/8d has permissions 0775, but should have 2775
Error: /home/rousseau/Serveurs_web/images.xxxx.fr/Lychee/public/uploads/medium/8d is not writable by www-data
[...]

Solution: sudo chgrp -R www-data medium

I discover that the files in big/ are NOT in sub-folders. And in medium/ I see sub-directories like medium/fd/54/. I guess you changed the way to store files to increase performance. Very good!
Do you plan to also upgrade the way the files are stored in big/? That could be done in the "Maintenance" page.

@ildyria
Copy link
Member Author

ildyria commented Jan 14, 2024 •
edited

Actually there is no big anymore... It is original now. :)
What is in big is legacy uploads.

@ildyria
Copy link
Member Author

ildyria commented Jan 14, 2024 •
edited

@LudovicRousseau I improved the feedback for the thumb generation, note that you will still be blocked by the timeout (which is probably why it was generating ~50 images instead of generating the default 100 of them).

@LudovicRousseau
Copy link

Actually there is no big anymore... It is original now. :) What is in big is legacy uploads.

Ah OK.
Maybe I should re-upload my albums then.

@ildyria
Copy link
Member Author

ildyria commented Jan 14, 2024 •
edited

Ah OK. Maybe I should re-upload my albums then.

You don't need to, all new pictures will be place in original. The optimization is mostly if you are browsing the images in your file explorer.

kamil4
kamil4 reviewed Jan 14, 2024
View reviewed changes
app/Actions/Diagnostics/Pipes/Checks/SmallMediumExistsCheck.php
Comment on lines +71 to +72
->when($svHelpers->getMaxWidth(SizeVariantType::SMALL) !== 0, fn ($q1) => $q1->where('width', '>', $svHelpers->getMaxWidth(SizeVariantType::SMALL)))
->when($svHelpers->getMaxHeight(SizeVariantType::SMALL) !== 0, fn ($q2) => $q2->orWhere('height', '>', $svHelpers->getMaxHeight(SizeVariantType::SMALL)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if that orWhere in the second line will behave as expected when where is missing (i.e., if width in the first line is 0)? It won't generate a syntactically invalid SQL query or something?

(I know -- I should test; I'm just not there yet)

Same question with respect to the other three size variants below...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would believe that Laravel is smart enough to resolve that. :)

qwerty287
qwerty287 approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

untested

@ildyria
Copy link
Member Author

ildyria commented Jan 16, 2024

untested

I tested it locally and with @LudovicRousseau :)

@ildyria ildyria changed the title Improve diagnostics with number of thumbs that can be regenerated Improve diagnostics with count of thumbs that can be regenerated Jan 16, 2024
@ildyria ildyria merged commit 26ad321 into master Jan 16, 2024
33 checks passed
@ildyria ildyria deleted the improve-diagnostics-missing-small-medium branch January 16, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kamil4 kamil4 kamil4 left review comments

@qwerty287 qwerty287 qwerty287 approved these changes

Assignees

@ildyria ildyria

Labels
Review: easy Easy review expected: probably just need a quick to go through.
Projects
None yet
Milestone
5.0.4
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ildyria @LudovicRousseau @kamil4 @qwerty287