From cf44502cdf2dc43b5e0a75ef1028bff3bb0bb600 Mon Sep 17 00:00:00 2001 From: Keegan George Date: Tue, 22 Oct 2024 22:53:09 +0900 Subject: [PATCH] DEV: Improvements to auto grid images (#29317) This PR is a follow-up to https://github.com/discourse/discourse/commit/ea1473e53260e8a458f8118b89e7b07b5cc50507. When we initially added the experimental feature for automatically adding `[grid]` to images, we add the [grid] surrounding images after all the uploads have been completed. This can lead to confusion when `[grid]` is delayed to be added in the composer, as users may try to add grid manually leading to breakage. This also leads to issues with Discourse AI's automatic image caption feature. **In this PR**: we simply move the logic to be added when the images are uploaded and processing. This way, `[grid]` surrounding images is added immediately. We also apply a fix for an edge-case to prevent images from being wrapped in `[grid]` when they are already inside `[grid]` tags. --- .../discourse/app/lib/uppy/composer-upload.js | 78 +++++++++++++------ spec/system/composer_uploads_spec.rb | 38 +++++++++ 2 files changed, 92 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/uppy/composer-upload.js b/app/assets/javascripts/discourse/app/lib/uppy/composer-upload.js index af2e78a4c33..41dfc74e029 100644 --- a/app/assets/javascripts/discourse/app/lib/uppy/composer-upload.js +++ b/app/assets/javascripts/discourse/app/lib/uppy/composer-upload.js @@ -13,7 +13,6 @@ import { displayErrorForBulkUpload, displayErrorForUpload, getUploadMarkdown, - IMAGE_MARKDOWN_REGEX, isImage, validateUploadedFile, } from "discourse/lib/uploads"; @@ -256,6 +255,10 @@ export default class UppyComposerUpload { if (this.composerModel.privateMessage) { file.meta.for_private_message = true; } + + if (isImage(file.name)) { + this.#consecutiveImages.push(file.name); + } }); }); @@ -296,7 +299,6 @@ export default class UppyComposerUpload { if (this.isDestroying || this.isDestroyed) { return; } - const upload = this.#inProgressUploads.find( (upl) => upl.id === file.id ); @@ -352,6 +354,14 @@ export default class UppyComposerUpload { file.name ); }); + + const MIN_IMAGES_TO_AUTO_GRID = 3; + if ( + this.siteSettings.experimental_auto_grid_images && + this.#consecutiveImages?.length >= MIN_IMAGES_TO_AUTO_GRID + ) { + this.#autoGridImages(); + } }); }); @@ -367,11 +377,6 @@ export default class UppyComposerUpload { getUploadMarkdown(upload) ); - // Track consecutive images for surrounding with [grid] later: - if (isImage(upload.url)) { - this.#consecutiveImages.push(markdown); - } - cacheShortUploadUrl(upload.short_url, upload); new ComposerVideoThumbnailUppy(getOwner(this)).generateVideoThumbnail( @@ -398,13 +403,6 @@ export default class UppyComposerUpload { `${this.composerEventPrefix}:all-uploads-complete` ); - const MIN_IMAGES_TO_AUTO_GRID = 3; - if ( - this.siteSettings.experimental_auto_grid_images && - this.#consecutiveImages?.length >= MIN_IMAGES_TO_AUTO_GRID - ) { - this.#autoGridImages(); - } this.#displayBufferedErrors(); this.#reset(); } @@ -735,20 +733,51 @@ export default class UppyComposerUpload { #autoGridImages() { const reply = this.composerModel.get("reply"); const imagesToWrapGrid = new Set(this.#consecutiveImages); - const matches = reply.match(IMAGE_MARKDOWN_REGEX) || []; + const uploadingText = I18n.t("uploading_filename", { + filename: "%placeholder%", + }); + const uploadingTextMatch = uploadingText.match(/^.*(?=: %placeholder%…)/); + + if (!uploadingTextMatch || !uploadingTextMatch[0]) { + return; + } + + const uploadingImagePattern = new RegExp( + "\\[" + uploadingTextMatch[0].trim() + ": ([^\\]]+?)\\.\\w+…\\]\\(\\)", + "g" + ); + + const matches = reply.match(uploadingImagePattern) || []; const foundImages = []; - matches.forEach((fullImageMarkdown) => { - fullImageMarkdown = fullImageMarkdown.trim(); - // Check if the matched image markdown is in the imagesToWrapGrid - if (imagesToWrapGrid.has(fullImageMarkdown)) { - foundImages.push(fullImageMarkdown); - imagesToWrapGrid.delete(fullImageMarkdown); + const existingGridPattern = /\[grid\]([\s\S]*?)\[\/grid\]/g; + const gridMatches = reply.match(existingGridPattern); - // Check if we've found all the images - if (imagesToWrapGrid.size === 0) { - return; + matches.forEach((imagePlaceholder) => { + imagePlaceholder = imagePlaceholder.trim(); + + const filenamePattern = new RegExp( + "\\[" + uploadingTextMatch[0].trim() + ": ([^\\]]+?)\\…\\]\\(\\)" + ); + + const filenameMatch = imagePlaceholder.match(filenamePattern); + + if (filenameMatch && filenameMatch[1]) { + const filename = filenameMatch[1]; + + const isWithinGrid = gridMatches?.some((gridContent) => + gridContent.includes(imagePlaceholder) + ); + + if (!isWithinGrid && imagesToWrapGrid.has(filename)) { + foundImages.push(imagePlaceholder); + imagesToWrapGrid.delete(filename); + + // Check if we've found all the images + if (imagesToWrapGrid.size === 0) { + return; + } } } }); @@ -778,6 +807,7 @@ export default class UppyComposerUpload { } } } + // Clear found images for the next consecutive images: this.#consecutiveImages.length = 0; foundImages.length = 0; diff --git a/spec/system/composer_uploads_spec.rb b/spec/system/composer_uploads_spec.rb index c3d19d7a099..bbee973b819 100644 --- a/spec/system/composer_uploads_spec.rb +++ b/spec/system/composer_uploads_spec.rb @@ -244,5 +244,43 @@ describe "Uploading files in the composer", type: :system do %r{!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\)}m, ) end + + it "does not automatically wrap images in [grid] tags when uploading inside an existing [grid]" do + visit "/new-topic" + expect(composer).to be_opened + + composer.fill_content("[grid]\n\n[/grid]") + composer.move_cursor_after("[grid]\n") + + file_path_1 = file_from_fixtures("logo.png", "images").path + file_path_2 = file_from_fixtures("logo.jpg", "images").path + file_path_3 = file_from_fixtures("downsized.png", "images").path + attach_file([file_path_1, file_path_2, file_path_3]) do + composer.click_toolbar_button("upload") + end + expect(composer).to have_no_in_progress_uploads + expect(composer.composer_input.value).to match( + %r{\[grid\].*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*?\[/grid\]}m, + ) + end + + it "automatically wraps images in [grid] when site language is different" do + SiteSetting.default_locale = "es" + + visit "/new-topic" + expect(composer).to be_opened + + file_path_1 = file_from_fixtures("logo.png", "images").path + file_path_2 = file_from_fixtures("logo.jpg", "images").path + file_path_3 = file_from_fixtures("downsized.png", "images").path + attach_file([file_path_1, file_path_2, file_path_3]) do + composer.click_toolbar_button("upload") + end + + expect(composer).to have_no_in_progress_uploads + expect(composer.composer_input.value).to match( + %r{\[grid\].*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*?\[/grid\]}m, + ) + end end end