From b2d48d9a7f52ae0d37567eec57469ea2d9c901d3 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 1 Oct 2023 13:05:18 +0100 Subject: [PATCH] Images: Rolled out image memory handling to image actions - Moved thumnbail loading out of repo into ImageResizer. - Updated gallery and editor image handling to show errors where possible to indicate memory issues for resizing/thumbs. - Updated gallery to load image data in a per-image basis via edit form for more resiliant thumb/data fetching. Data was previously provided via gallery listing, which could be affected by failing generation of other images. - Updated image manager double click handling to be more pleasant and not flash away the edit form. - Updated editor handlers to use main URL when thumbs fail to load. --- .../Controllers/DrawioImageController.php | 19 ++++++-- .../Controllers/GalleryImageController.php | 18 ++++++-- app/Uploads/Controllers/ImageController.php | 26 ++++++++--- .../Controllers/ImageGalleryApiController.php | 7 ++- app/Uploads/Image.php | 2 +- app/Uploads/ImageRepo.php | 43 +++---------------- app/Uploads/ImageResizer.php | 43 +++++++++++++++---- lang/en/errors.php | 5 ++- resources/js/components/image-manager.js | 20 +++++---- resources/js/markdown/actions.js | 4 +- resources/js/wysiwyg/drop-paste-handling.js | 2 +- resources/js/wysiwyg/plugins-imagemanager.js | 2 +- resources/sass/_components.scss | 12 ++++++ .../pages/parts/image-manager-form.blade.php | 9 ++++ .../pages/parts/image-manager-list.blade.php | 8 +++- 15 files changed, 142 insertions(+), 78 deletions(-) diff --git a/app/Uploads/Controllers/DrawioImageController.php b/app/Uploads/Controllers/DrawioImageController.php index 49f0c1655..6293da4f7 100644 --- a/app/Uploads/Controllers/DrawioImageController.php +++ b/app/Uploads/Controllers/DrawioImageController.php @@ -5,6 +5,8 @@ namespace BookStack\Uploads\Controllers; use BookStack\Exceptions\ImageUploadException; use BookStack\Http\Controller; use BookStack\Uploads\ImageRepo; +use BookStack\Uploads\ImageResizer; +use BookStack\Util\OutOfMemoryHandler; use Exception; use Illuminate\Http\Request; @@ -19,7 +21,7 @@ class DrawioImageController extends Controller * Get a list of gallery images, in a list. * Can be paged and filtered by entity. */ - public function list(Request $request) + public function list(Request $request, ImageResizer $resizer) { $page = $request->get('page', 1); $searchTerm = $request->get('search', null); @@ -27,11 +29,20 @@ class DrawioImageController extends Controller $parentTypeFilter = $request->get('filter_type', null); $imgData = $this->imageRepo->getEntityFiltered('drawio', $parentTypeFilter, $page, 24, $uploadedToFilter, $searchTerm); - - return view('pages.parts.image-manager-list', [ + $viewData = [ + 'warning' => '', 'images' => $imgData['images'], 'hasMore' => $imgData['has_more'], - ]); + ]; + + new OutOfMemoryHandler(function () use ($viewData) { + $viewData['warning'] = trans('errors.image_gallery_thumbnail_memory_limit'); + return response()->view('pages.parts.image-manager-list', $viewData, 200); + }); + + $resizer->loadGalleryThumbnailsForMany($imgData['images']); + + return view('pages.parts.image-manager-list', $viewData); } /** diff --git a/app/Uploads/Controllers/GalleryImageController.php b/app/Uploads/Controllers/GalleryImageController.php index 0696ca62b..258f2bef6 100644 --- a/app/Uploads/Controllers/GalleryImageController.php +++ b/app/Uploads/Controllers/GalleryImageController.php @@ -5,6 +5,7 @@ namespace BookStack\Uploads\Controllers; use BookStack\Exceptions\ImageUploadException; use BookStack\Http\Controller; use BookStack\Uploads\ImageRepo; +use BookStack\Uploads\ImageResizer; use BookStack\Util\OutOfMemoryHandler; use Illuminate\Http\Request; use Illuminate\Support\Facades\App; @@ -22,7 +23,7 @@ class GalleryImageController extends Controller * Get a list of gallery images, in a list. * Can be paged and filtered by entity. */ - public function list(Request $request) + public function list(Request $request, ImageResizer $resizer) { $page = $request->get('page', 1); $searchTerm = $request->get('search', null); @@ -30,11 +31,20 @@ class GalleryImageController extends Controller $parentTypeFilter = $request->get('filter_type', null); $imgData = $this->imageRepo->getEntityFiltered('gallery', $parentTypeFilter, $page, 30, $uploadedToFilter, $searchTerm); - - return view('pages.parts.image-manager-list', [ + $viewData = [ + 'warning' => '', 'images' => $imgData['images'], 'hasMore' => $imgData['has_more'], - ]); + ]; + + new OutOfMemoryHandler(function () use ($viewData) { + $viewData['warning'] = trans('errors.image_gallery_thumbnail_memory_limit'); + return response()->view('pages.parts.image-manager-list', $viewData, 200); + }); + + $resizer->loadGalleryThumbnailsForMany($imgData['images']); + + return view('pages.parts.image-manager-list', $viewData); } /** diff --git a/app/Uploads/Controllers/ImageController.php b/app/Uploads/Controllers/ImageController.php index f92338bc8..c68ffdf6b 100644 --- a/app/Uploads/Controllers/ImageController.php +++ b/app/Uploads/Controllers/ImageController.php @@ -4,9 +4,11 @@ namespace BookStack\Uploads\Controllers; use BookStack\Exceptions\ImageUploadException; use BookStack\Exceptions\NotFoundException; +use BookStack\Exceptions\NotifyException; use BookStack\Http\Controller; use BookStack\Uploads\Image; use BookStack\Uploads\ImageRepo; +use BookStack\Uploads\ImageResizer; use BookStack\Uploads\ImageService; use BookStack\Util\OutOfMemoryHandler; use Exception; @@ -16,7 +18,8 @@ class ImageController extends Controller { public function __construct( protected ImageRepo $imageRepo, - protected ImageService $imageService + protected ImageService $imageService, + protected ImageResizer $imageResizer, ) { } @@ -98,12 +101,20 @@ class ImageController extends Controller $dependantPages = $this->imageRepo->getPagesUsingImage($image); } - $this->imageRepo->loadThumbs($image, false); - - return view('pages.parts.image-manager-form', [ + $viewData = [ 'image' => $image, 'dependantPages' => $dependantPages ?? null, - ]); + 'warning' => '', + ]; + + new OutOfMemoryHandler(function () use ($viewData) { + $viewData['warning'] = trans('errors.image_thumbnail_memory_limit'); + return response()->view('pages.parts.image-manager-form', $viewData); + }); + + $this->imageResizer->loadGalleryThumbnailsForImage($image, false); + + return view('pages.parts.image-manager-form', $viewData); } /** @@ -135,15 +146,16 @@ class ImageController extends Controller return $this->jsonError(trans('errors.image_thumbnail_memory_limit')); }); - $this->imageRepo->loadThumbs($image, true); + $this->imageResizer->loadGalleryThumbnailsForImage($image, true); return response(trans('components.image_rebuild_thumbs_success')); } /** * Check related page permission and ensure type is drawio or gallery. + * @throws NotifyException */ - protected function checkImagePermission(Image $image) + protected function checkImagePermission(Image $image): void { if ($image->type !== 'drawio' && $image->type !== 'gallery') { $this->showPermissionError(); diff --git a/app/Uploads/Controllers/ImageGalleryApiController.php b/app/Uploads/Controllers/ImageGalleryApiController.php index efdff5be4..ec96e4593 100644 --- a/app/Uploads/Controllers/ImageGalleryApiController.php +++ b/app/Uploads/Controllers/ImageGalleryApiController.php @@ -6,6 +6,7 @@ use BookStack\Entities\Models\Page; use BookStack\Http\ApiController; use BookStack\Uploads\Image; use BookStack\Uploads\ImageRepo; +use BookStack\Uploads\ImageResizer; use Illuminate\Http\Request; class ImageGalleryApiController extends ApiController @@ -15,7 +16,8 @@ class ImageGalleryApiController extends ApiController ]; public function __construct( - protected ImageRepo $imageRepo + protected ImageRepo $imageRepo, + protected ImageResizer $imageResizer, ) { } @@ -130,7 +132,7 @@ class ImageGalleryApiController extends ApiController */ protected function formatForSingleResponse(Image $image): array { - $this->imageRepo->loadThumbs($image, false); + $this->imageResizer->loadGalleryThumbnailsForImage($image, false); $data = $image->toArray(); $data['created_by'] = $image->createdBy; $data['updated_by'] = $image->updatedBy; @@ -138,6 +140,7 @@ class ImageGalleryApiController extends ApiController $escapedUrl = htmlentities($image->url); $escapedName = htmlentities($image->name); + if ($image->type === 'drawio') { $data['content']['html'] = "
id}\">
"; $data['content']['markdown'] = $data['content']['html']; diff --git a/app/Uploads/Image.php b/app/Uploads/Image.php index 1e42f414b..0a267a644 100644 --- a/app/Uploads/Image.php +++ b/app/Uploads/Image.php @@ -52,7 +52,7 @@ class Image extends Model */ public function getThumb(?int $width, ?int $height, bool $keepRatio = false): ?string { - return app()->make(ImageResizer::class)->resizeToThumbnailUrl($this, $width, $height, $keepRatio, false, true); + return app()->make(ImageResizer::class)->resizeToThumbnailUrl($this, $width, $height, $keepRatio, false); } /** diff --git a/app/Uploads/ImageRepo.php b/app/Uploads/ImageRepo.php index 4aa36bab9..0e312d883 100644 --- a/app/Uploads/ImageRepo.php +++ b/app/Uploads/ImageRepo.php @@ -30,19 +30,13 @@ class ImageRepo * Execute a paginated query, returning in a standard format. * Also runs the query through the restriction system. */ - private function returnPaginated(Builder $query, int $page = 1, int $pageSize = 24): array + protected function returnPaginated(Builder $query, int $page = 1, int $pageSize = 24): array { $images = $query->orderBy('created_at', 'desc')->skip($pageSize * ($page - 1))->take($pageSize + 1)->get(); - $hasMore = count($images) > $pageSize; - - $returnImages = $images->take($pageSize); - $returnImages->each(function (Image $image) { - $this->loadThumbs($image, false); - }); return [ - 'images' => $returnImages, - 'has_more' => $hasMore, + 'images' => $images->take($pageSize), + 'has_more' => count($images) > $pageSize, ]; } @@ -120,7 +114,7 @@ class ImageRepo $image = $this->imageService->saveNewFromUpload($uploadFile, $type, $uploadedTo, $resizeWidth, $resizeHeight, $keepRatio); if ($type !== 'system') { - $this->loadThumbs($image, true); + $this->imageResizer->loadGalleryThumbnailsForImage($image, true); } return $image; @@ -134,7 +128,7 @@ class ImageRepo public function saveNewFromData(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image { $image = $this->imageService->saveNew($imageName, $imageData, $type, $uploadedTo); - $this->loadThumbs($image, true); + $this->imageResizer->loadGalleryThumbnailsForImage($image, true); return $image; } @@ -161,7 +155,7 @@ class ImageRepo $image->fill($updateDetails); $image->updated_by = user()->id; $image->save(); - $this->loadThumbs($image, false); + $this->imageResizer->loadGalleryThumbnailsForImage($image, false); return $image; } @@ -182,7 +176,7 @@ class ImageRepo $image->save(); $this->imageService->replaceExistingFromUpload($image->path, $image->type, $file); - $this->loadThumbs($image, true); + $this->imageResizer->loadGalleryThumbnailsForImage($image, true); } /** @@ -214,29 +208,6 @@ class ImageRepo } } - /** - * Load thumbnails onto an image object. - */ - public function loadThumbs(Image $image, bool $shouldCreate): void - { - $image->setAttribute('thumbs', [ - 'gallery' => $this->getThumbnail($image, 150, 150, false, $shouldCreate), - 'display' => $this->getThumbnail($image, 1680, null, true, $shouldCreate), - ]); - } - - /** - * Get a thumbnail URL for the given image. - */ - protected function getThumbnail(Image $image, ?int $width, ?int $height, bool $keepRatio, bool $shouldCreate): ?string - { - try { - return $this->imageResizer->resizeToThumbnailUrl($image, $width, $height, $keepRatio, $shouldCreate); - } catch (Exception $exception) { - return null; - } - } - /** * Get the raw image data from an Image. */ diff --git a/app/Uploads/ImageResizer.php b/app/Uploads/ImageResizer.php index 5fe8a8954..0d090a94b 100644 --- a/app/Uploads/ImageResizer.php +++ b/app/Uploads/ImageResizer.php @@ -11,12 +11,42 @@ use Intervention\Image\ImageManager; class ImageResizer { + protected const THUMBNAIL_CACHE_TIME = 604_800; // 1 week + public function __construct( protected ImageManager $intervention, protected ImageStorage $storage, ) { } + /** + * Load gallery thumbnails for a set of images. + * @param iterable $images + */ + public function loadGalleryThumbnailsForMany(iterable $images, bool $shouldCreate = false): void + { + foreach ($images as $image) { + $this->loadGalleryThumbnailsForImage($image, $shouldCreate); + } + } + + /** + * Load gallery thumbnails into the given image instance. + */ + public function loadGalleryThumbnailsForImage(Image $image, bool $shouldCreate): void + { + $thumbs = ['gallery' => null, 'display' => null]; + + try { + $thumbs['gallery'] = $this->resizeToThumbnailUrl($image, 150, 150, false, $shouldCreate); + $thumbs['display'] = $this->resizeToThumbnailUrl($image, 1680, null, true, $shouldCreate); + } catch (Exception $exception) { + // Prevent thumbnail errors from stopping execution + } + + $image->setAttribute('thumbs', $thumbs); + } + /** * Get the thumbnail for an image. * If $keepRatio is true only the width will be used. @@ -29,8 +59,7 @@ class ImageResizer ?int $width, ?int $height, bool $keepRatio = false, - bool $shouldCreate = false, - bool $canCreate = false, + bool $shouldCreate = false ): ?string { // Do not resize GIF images where we're not cropping if ($keepRatio && $this->isGif($image)) { @@ -52,7 +81,7 @@ class ImageResizer // If thumbnail has already been generated, serve that and cache path $disk = $this->storage->getDisk($image->type); if (!$shouldCreate && $disk->exists($thumbFilePath)) { - Cache::put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); + Cache::put($thumbCacheKey, $thumbFilePath, static::THUMBNAIL_CACHE_TIME); return $this->storage->getPublicUrl($thumbFilePath); } @@ -61,19 +90,15 @@ class ImageResizer // Do not resize apng images where we're not cropping if ($keepRatio && $this->isApngData($image, $imageData)) { - Cache::put($thumbCacheKey, $image->path, 60 * 60 * 72); + Cache::put($thumbCacheKey, $image->path, static::THUMBNAIL_CACHE_TIME); return $this->storage->getPublicUrl($image->path); } - if (!$shouldCreate && !$canCreate) { - return null; - } - // If not in cache and thumbnail does not exist, generate thumb and cache path $thumbData = $this->resizeImageData($imageData, $width, $height, $keepRatio); $disk->put($thumbFilePath, $thumbData, true); - Cache::put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); + Cache::put($thumbCacheKey, $thumbFilePath, static::THUMBNAIL_CACHE_TIME); return $this->storage->getPublicUrl($thumbFilePath); } diff --git a/lang/en/errors.php b/lang/en/errors.php index 285817e47..8813cf90a 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -51,8 +51,9 @@ return [ 'image_upload_error' => 'An error occurred uploading the image', 'image_upload_type_error' => 'The image type being uploaded is invalid', 'image_upload_replace_type' => 'Image file replacements must be of the same type', - 'image_upload_memory_limit' => 'Failed to handle image upload and/or create thumbnails due to system resource limits', - 'image_thumbnail_memory_limit' => 'Failed to create image size variations due to system resource limits', + 'image_upload_memory_limit' => 'Failed to handle image upload and/or create thumbnails due to system resource limits.', + 'image_thumbnail_memory_limit' => 'Failed to create image size variations due to system resource limits.', + 'image_gallery_thumbnail_memory_limit' => 'Failed to create gallery thumbnails due to system resource limits.', 'drawing_data_not_found' => 'Drawing data could not be loaded. The drawing file might no longer exist or you may not have permission to access it.', // Attachments diff --git a/resources/js/components/image-manager.js b/resources/js/components/image-manager.js index bc0493a88..b6397b004 100644 --- a/resources/js/components/image-manager.js +++ b/resources/js/components/image-manager.js @@ -1,6 +1,4 @@ -import { - onChildEvent, onSelect, removeLoading, showLoading, -} from '../services/dom'; +import {onChildEvent, onSelect, removeLoading, showLoading,} from '../services/dom'; import {Component} from './component'; export class ImageManager extends Component { @@ -229,8 +227,8 @@ export class ImageManager extends Component { this.loadGallery(); } - onImageSelectEvent(event) { - const image = JSON.parse(event.detail.data); + async onImageSelectEvent(event) { + let image = JSON.parse(event.detail.data); const isDblClick = ((image && image.id === this.lastSelected.id) && Date.now() - this.lastSelectedTime < 400); const alreadySelected = event.target.classList.contains('selected'); @@ -238,12 +236,15 @@ export class ImageManager extends Component { el.classList.remove('selected'); }); - if (!alreadySelected) { + if (!alreadySelected && !isDblClick) { event.target.classList.add('selected'); - this.loadImageEditForm(image.id); - } else { + image = await this.loadImageEditForm(image.id); + } else if (!isDblClick) { this.resetEditForm(); + } else if (isDblClick) { + image = this.lastSelected; } + this.selectButton.classList.toggle('hidden', alreadySelected); if (isDblClick && this.callback) { @@ -265,6 +266,9 @@ export class ImageManager extends Component { this.formContainer.innerHTML = formHtml; this.formContainerPlaceholder.setAttribute('hidden', ''); window.$components.init(this.formContainer); + + const imageDataEl = this.formContainer.querySelector('#image-manager-form-image-data'); + return JSON.parse(imageDataEl.text); } runLoadMore() { diff --git a/resources/js/markdown/actions.js b/resources/js/markdown/actions.js index a7fde9322..4909a59d0 100644 --- a/resources/js/markdown/actions.js +++ b/resources/js/markdown/actions.js @@ -34,7 +34,7 @@ export class Actions { const imageManager = window.$components.first('image-manager'); imageManager.show(image => { - const imageUrl = image.thumbs.display || image.url; + const imageUrl = image.thumbs?.display || image.url; const selectedText = this.#getSelectionText(); const newText = `[![${selectedText || image.name}](${imageUrl})](${image.url})`; this.#replaceSelection(newText, newText.length); @@ -417,7 +417,7 @@ export class Actions { const newContent = `[![](${data.thumbs.display})](${data.url})`; this.#findAndReplaceContent(placeHolderText, newContent); } catch (err) { - window.$events.emit('error', this.editor.config.text.imageUploadError); + window.$events.error(err?.data?.message || this.editor.config.text.imageUploadError); this.#findAndReplaceContent(placeHolderText, ''); console.error(err); } diff --git a/resources/js/wysiwyg/drop-paste-handling.js b/resources/js/wysiwyg/drop-paste-handling.js index 33078cd1d..9668692c8 100644 --- a/resources/js/wysiwyg/drop-paste-handling.js +++ b/resources/js/wysiwyg/drop-paste-handling.js @@ -61,7 +61,7 @@ function paste(editor, options, event) { editor.dom.replace(newEl, id); }).catch(err => { editor.dom.remove(id); - window.$events.emit('error', options.translations.imageUploadErrorText); + window.$events.error(err?.data?.message || options.translations.imageUploadErrorText); console.error(err); }); }, 10); diff --git a/resources/js/wysiwyg/plugins-imagemanager.js b/resources/js/wysiwyg/plugins-imagemanager.js index 37b5bfafd..f1ea12050 100644 --- a/resources/js/wysiwyg/plugins-imagemanager.js +++ b/resources/js/wysiwyg/plugins-imagemanager.js @@ -11,7 +11,7 @@ function register(editor) { /** @type {ImageManager} * */ const imageManager = window.$components.first('image-manager'); imageManager.show(image => { - const imageUrl = image.thumbs.display || image.url; + const imageUrl = image.thumbs?.display || image.url; let html = ``; html += `${image.name}`; html += ''; diff --git a/resources/sass/_components.scss b/resources/sass/_components.scss index c1989c1f6..150f78e12 100644 --- a/resources/sass/_components.scss +++ b/resources/sass/_components.scss @@ -457,6 +457,18 @@ body.flexbox-support #entity-selector-wrap .popup-body .form-group { text-align: center; } +.image-manager-list .image-manager-list-warning { + grid-column: 1 / -1; + aspect-ratio: auto; +} + +.image-manager-warning { + @include lightDark(background, #FFF, #333); + color: var(--color-warning); + font-weight: bold; + border-inline: 3px solid var(--color-warning); +} + .image-manager-sidebar { width: 300px; margin: 0 auto; diff --git a/resources/views/pages/parts/image-manager-form.blade.php b/resources/views/pages/parts/image-manager-form.blade.php index 3a73bee7c..bd84e247d 100644 --- a/resources/views/pages/parts/image-manager-form.blade.php +++ b/resources/views/pages/parts/image-manager-form.blade.php @@ -8,8 +8,17 @@ option:dropzone:file-accept="image/*" class="image-manager-details"> + @if($warning ?? '') +
+
@icon('warning')
+
{{ $warning }}
+
+ @endif +
+ +
+
@icon('warning')
+
{{ $warning }}
+ +@endif @foreach($images as $index => $image)