[4.2] Local adapter thumbnails #36552

dgrammatiko · 2022-01-03T20:02:26Z

Pull Request for Issue #36533 [Solves no 2 of the 3 reported problems, the thumbs ].

This is still a draft as it is missing the UI for regenerating the thumbs recursively (in the options of the local adapter)

Summary of Changes

Generate thumbs for the Media manager

Testing Instructions

Actual result BEFORE applying this Pull Request

Media manager loads a thumb

Expected result AFTER applying this Pull Request

Media manager Loads the full image

Documentation Changes Required

From a code point of view, the decision was to place the thumbs in a subfolder of the media folder called cache_mm
The reason that I didn't reuse the existing cache folder is simple: Joomla cleans that folder on each update and we WANT the thumbs to persist on updates (they are static files so PHP/JS/CSS/db changes are largely irrelevant).

plugins/filesystem/local/src/Adapter/LocalAdapter.php

Co-authored-by: Brian Teeman <[email protected]>

brianteeman · 2022-01-03T20:13:49Z

/me still waits for the day when there are no path issues in one of your pr

<div class="image-cropped" style="background-image: url("http://localhost/joomla-cms/C:/htdocs/joomla-cms/images/sampledata/parks/landscape/120px_pinnacles_western_australia.jpg?1641240738875");"></div>

dgrammatiko · 2022-01-03T20:19:45Z

/me still waits for the day when there are no path issues in one of your pr

FWIW this is not the subfolder issue but me modifying the path string in a UNIX way (obviously fails for fat/ntfs)

brianteeman · 2022-01-03T20:32:52Z

No its not a unixfs thing you are mixing up the JPATH_ROOT and the uri:root

I doubt it would work on any filesystem

JPATH_ROOT : C:\htdocs\joomla-cms
Uri::root() : http://localhost/joomla-cms/

dgrammatiko · 2022-01-03T21:19:50Z

@laoneo any thoughts here?

brianteeman · 2022-01-03T21:36:48Z

still no joy

laoneo · 2022-01-03T21:44:19Z

I did something similar in DPMedia. But made resized only 10 images per request. The other ones I do serve with with a placeholder image. On large image folders it takes too much resources on shared hosting and ended up with broken thumbnails.

brianteeman · 2022-01-03T22:51:40Z

the thumbnails are now created and used

think you need to check the code that creates the thumbnails as the dimensions are ok but the files are big - some are even bigger than the original

brianteeman · 2022-01-03T22:54:33Z

PS if we have thumbnails do we still need lazyload?

dgrammatiko · 2022-01-03T22:58:05Z

PS if we have thumbnails do we still need lazyload?

Yes, the thumbnails fix the bandwidth problem, the lazyloading fixes the UI responsiveness (eg view the file thumbs quicker).
Still the pagination or streaming is missing to fix the enormous lists (the actual number might not be that big on a cheap server). As I said this is a 3 fold problem...

Fedik · 2022-01-04T09:47:04Z

The reason that I didn't reuse the existing cache folder

Please use /media/cache/com_media as cache.
On update Joomla will clear a root /cache and should not clear /media/cache, otherwise it a bug to me.

Other thing: have to ensure a thumbnail exists (and create it if not) on request, not only while upload.
This way it will be easy to refresh whole cache in case of any failure.

dgrammatiko · 2022-01-04T10:13:51Z

Other thing: have to ensure a thumbnail exists (and create it if not) on request,

It's already like that

otherwise it a bug to me.

The last time I've checked, Joomla clears the media cache but I can change it no debate here

EDIT: @Fedik I switched the cache folder to 'media/cache/com_media/thumbs/' . $this->filePath where $this->filePath is the local adapter, eg images.
Someone needs to confirm that Joomla is not emptying the media/cache folder on updates otherwise this is completely wrong, an update should not invalidate the thumbnails...

dgrammatiko · 2022-01-16T21:34:04Z

Missing / before the filename.

Is that for the tests or on your local machine?

Signed-off-by: dgrammatiko <[email protected]>

Quy · 2022-01-16T21:40:06Z

Testing locally with a new install.

dgrammatiko · 2022-01-16T21:42:02Z

@Quy fetch the latest changes and try again (it's hard to debug something that's not replicable locally)

Signed-off-by: dgrammatiko <[email protected]>

Quy · 2022-01-16T21:49:32Z

Sorry no go. Thumbnails are generated. Just missing / before the filename in the thumbnail URL.

Signed-off-by: dgrammatiko <[email protected]>

Quy · 2022-01-16T22:28:13Z

It is ok in the main folder but not in subfolders. Missing / after banners folder.

http://localhost/Joomla_4.1.0-dev+pr.36552-Development-Full_Package/media/cache/com_media/thumbs/images/bannersosmbanner1.png?1642371757494

Signed-off-by: dgrammatiko <[email protected]>

plugins/filesystem/local/src/Adapter/LocalAdapter.php

Co-authored-by: Quy <[email protected]>

Signed-off-by: dgrammatiko <[email protected]>

Quy · 2022-01-17T18:39:17Z

plugins/filesystem/local/src/Adapter/LocalAdapter.php

+			if (!is_dir(dirname($thumbPaths['fs'])))
+			{
+				mkdir(dirname($thumbPaths['fs']), 0755, true);
+			}


Can this be removed since it is checked/created here?

joomla-cms/libraries/src/Image/Image.php

Line 345 in 4de5af0

if (!is_dir($thumbsFolder) && (!is_dir(\dirname($thumbsFolder)) || !@mkdir($thumbsFolder)))

Fedik · 2022-01-17T19:44:55Z

plugins/filesystem/local/src/Adapter/LocalAdapter.php

+		{
+			$dir = JPATH_ROOT . '/media/cache/com_media/thumbs/' . $this->filePath;
+
+			if (!is_dir($dir))


You need it only when you writing a thumbs, it does not need in constructor

Fedik · 2022-01-17T19:46:06Z

plugins/filesystem/local/src/Adapter/LocalAdapter.php

+
+			if (!is_dir(dirname($thumbPaths['fs'])))
+			{
+				mkdir(dirname($thumbPaths['fs']), 0755, true);


Please replace all use of mkdir() to Folder::create(), this will create a folder with all subfolders recursively.

dgrammatiko · 2022-01-17T20:13:41Z

@Quy @Fedik and everyone else involved here: first of all thanks for all the code reviews, feedback and testing but we're too late for 4.1...
This needs to be postponed to 4.2 or if the maintainers feel that it could go on a patch on a 4.1.x
That said I won't be the one driving this feature so feel free to fork the work done here and patch anything still missing, buggy, etc

Thanks! ☮

thumbs up

47af679

joomla-cms-bot added the PR-4.1-dev label Jan 3, 2022

brianteeman reviewed Jan 3, 2022

View reviewed changes

plugins/filesystem/local/src/Adapter/LocalAdapter.php Outdated Show resolved Hide resolved

dgrammatiko and others added 2 commits January 3, 2022 21:06

Update plugins/filesystem/local/src/Adapter/LocalAdapter.php

a91a482

Co-authored-by: Brian Teeman <[email protected]>

CS

32e830b

dgrammatiko marked this pull request as draft January 3, 2022 20:10

dgrammatiko added 2 commits January 3, 2022 21:22

Fix windows FS

61b7c09

Deal with the thumbs on delete/save

ab78242

dgrammatiko added 4 commits January 3, 2022 21:50

oopsy

4e7595e

cs

6fa87d4

Cs

4d809d3

go

419f3c4

dgrammatiko added 3 commits January 3, 2022 23:13

meh

86b12aa

meh

f625e4b

meh

0c7c523

Expose the thumbs option

c301e4b

joomla-cms-bot added the Language Change This is for Translators label Jan 4, 2022

Use the media/cache folder

071ebc0

dgrammatiko marked this pull request as ready for review January 4, 2022 19:24

don't

af9df1b

Signed-off-by: dgrammatiko <[email protected]>

CS

eec4a93

Signed-off-by: dgrammatiko <[email protected]>

paths again

a69a231

Signed-off-by: dgrammatiko <[email protected]>

dgrammatiko added 3 commits January 16, 2022 23:34

maybe

e1f768b

Signed-off-by: dgrammatiko <[email protected]>

maybe not

d54e485

Signed-off-by: dgrammatiko <[email protected]>

mute failed test

a349fce

Signed-off-by: dgrammatiko <[email protected]>

joomla-cms-bot added the Unit/System Tests label Jan 16, 2022

dgrammatiko added 2 commits January 17, 2022 00:01

maybe realpath is not the right approach here

beb1a40

Signed-off-by: dgrammatiko <[email protected]>

and this

da45a4a

Signed-off-by: dgrammatiko <[email protected]>

Quy reviewed Jan 17, 2022

View reviewed changes

plugins/filesystem/local/src/Adapter/LocalAdapter.php Outdated Show resolved Hide resolved

dgrammatiko and others added 2 commits January 17, 2022 01:22

Update plugins/filesystem/local/src/Adapter/LocalAdapter.php

43c068b

Co-authored-by: Quy <[email protected]>

simplified

4de5af0

Signed-off-by: dgrammatiko <[email protected]>

Quy reviewed Jan 17, 2022

View reviewed changes

Fedik reviewed Jan 17, 2022

View reviewed changes

dgrammatiko changed the base branch from 4.1-dev to 4.2-dev January 24, 2022 10:41

Quy added PR-4.2-dev and removed PR-4.1-dev labels Jan 24, 2022

richard67 changed the title ~~[4.1] Local adapter thumbnails~~ [4.2] Local adapter thumbnails Feb 1, 2022

Merge branch '4.2-dev' into 4.1-devlocal-adapter-thumbs

77eddd5

dgrammatiko closed this Feb 3, 2022

dgrammatiko deleted the 4.1-devlocal-adapter-thumbs branch April 18, 2022 09:27

dgrammatiko mentioned this pull request Sep 21, 2022

[J4] Media Manager UX/Performance Improvements #36533

Open

obuisard mentioned this pull request Sep 22, 2022

[4.3] Local adapter thumbnails (PR 36552 clone) #38805

Merged

4 tasks

Uh oh!

[4.2] Local adapter thumbnails #36552

[4.2] Local adapter thumbnails #36552

Uh oh!

Conversation

dgrammatiko commented Jan 3, 2022

This is still a draft as it is missing the UI for regenerating the thumbs recursively (in the options of the local adapter)

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Uh oh!

Uh oh!

brianteeman commented Jan 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgrammatiko commented Jan 3, 2022

Uh oh!

brianteeman commented Jan 3, 2022

Uh oh!

dgrammatiko commented Jan 3, 2022

Uh oh!

brianteeman commented Jan 3, 2022

Uh oh!

laoneo commented Jan 3, 2022

Uh oh!

brianteeman commented Jan 3, 2022

Uh oh!

brianteeman commented Jan 3, 2022

Uh oh!

dgrammatiko commented Jan 3, 2022

Uh oh!

Fedik commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgrammatiko commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgrammatiko commented Jan 16, 2022

Uh oh!

Quy commented Jan 16, 2022

Uh oh!

dgrammatiko commented Jan 16, 2022

Uh oh!

Quy commented Jan 16, 2022

Uh oh!

Quy commented Jan 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Quy Jan 17, 2022

Choose a reason for hiding this comment

Uh oh!

Fedik Jan 17, 2022

Choose a reason for hiding this comment

Uh oh!

Fedik Jan 17, 2022

Choose a reason for hiding this comment

Uh oh!

dgrammatiko commented Jan 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

brianteeman commented Jan 3, 2022 •

edited

Loading

Fedik commented Jan 4, 2022 •

edited

Loading

dgrammatiko commented Jan 4, 2022 •

edited

Loading

Quy commented Jan 16, 2022 •

edited

Loading