mybb icon indicating copy to clipboard operation
mybb copied to clipboard

Editor Paste / Drag-drop Image Handler

Open effone opened this issue 3 years ago • 9 comments

Resolves #2440, resolves #4078

Overrules #4080

effone avatar Apr 18 '21 12:04 effone

Questions:

  1. ~~CHMOD new upload directory?~~ DONE
  2. ~~Backend upload errors need to be displayed? (needs addition of language strings)~~ Silently skipped
  3. ~~Shall I update SCE to 3.0? Since the PR is already big?~~ DVZ waved head side-by-side
  4. ~~Rename the js file to avoid old template conflict?~~ Renamed. Open to discuss further
  5. ~~New settings not required to push through upgrade script? (I guess, no...)~~ sync_settings( ); will take care of it

@mybb/reviewers

effone avatar Apr 19 '21 20:04 effone

Nice, good job! Briefly tested! Would be nice to get it together with #4195 into 1.8.27 :) SCeditor needs some love!

@laird - could you test it please?

Eldenroot avatar Apr 25 '21 15:04 Eldenroot

Nice, good job! Briefly tested! Would be nice to get it together with #4195 into 1.8.27 :) SCeditor needs some love!

Agreed, @Eldenroot - it's excellent work, and it would be great if we could get this in to 1.8.27 at the last minute.

could you test it please?

Yes, for sure. I was away for a few days (oh, and you left out my surname in the mention - poor old Laird Popkin from Atlanta, Georgia must be feeling quite confused!) but have now tested this PR and have been in contact with @effone about it. There are a few minor niggles to sort out, but generally it seems to work as advertised, which will be a relief to so many of our users who have been caught out by this missing feature. My list of minor niggles is:

  1. There's a missing require_once of inc/functions_post.php in the new code in xmlhttp.php (confirmed by @effone, who I think is going to update the PR).
  2. The existence of the same problems with troublesome treatment of (ambiguity around) absolute versus relative paths for the new postembedpath setting as we tried to solve for the uploadspath setting in PR #4342 (not yet discussed with @effone). (ETA: Perhaps the easiest way to resolve these problems is simply to stipulate that the setting must be a relative path. This would ensure, too, re my comment below, that image files always can be served out of this directory).
  3. Errors being returned by the new code in xmlhttp.php as boolean true rather than as a human-readable string, and those errors not yet being displayed in any way in the browser - currently, the Javascript code has a comment "jGrowl it?", to which I think the answer is emphatically "Yes, definitely, let's do that".

I'm also not sure whether serving the image files directly out of the postembedpath directory (by default a sub-directory of uploads) is such a good idea, but maybe others can comment on that (not yet discussed with @effone).

@effone also made a good case to me privately for renaming jscripts/bbcodes_sceditor.js, the only unanswered question in his comment above, so perhaps we can quickly get that and the above sorted and then hopefully merge this very nice PR in time for 1.8.27.

lairdshaw avatar May 06 '21 15:05 lairdshaw

Would it be better to handle uploaded files as regular attachments? That would make it an interface improvement without introducing new kinds of uploads and locations watch out for. The editor field and attachment section could then be merged in the future.

With that behavior, we can just add server/client-side hooks to allows plugins to intercept such uploads (and insert external [img] links, etc.). This would make it easier to implement new platforms without modifying the core, and MyBB wouldn't include references to any specific platforms.

dvz avatar May 10 '21 22:05 dvz

As has been clarified before also, embeds are not attachments. They should not be treated as one and should not impact on allotted quota either. They should not be saved as .attach and they are not supposed to be bound as per admin's choice to get impacted to profile eligibility as attachment or free from restrictions if admin opts for an external API. My approach is justified as per my opinions.

Regarding other platforms it has been discussed with @euantorano and agreed upon that I can approach towards third party upload APIs

effone avatar May 11 '21 01:05 effone

Uploads path should be updated to use absolute path.

That will be requiring a proxy (see comment below)

Imgur and Imgbb API settings should be spliced.

Hmm. OK. But why it is problematic as what it is now? Admins gonna use either of the services anyway

I think this plugin should somehow mask image links, rather than disclose the full path like in [img]http://localhost/mybb/./uploads/postembeds/1_1620719162_blob.png[/img]

A considerable point from the privacy point of view.

For privacy purposes images should only be served if the user has permission to the post being seen. It isn't necessary to ship all these features in, but having a file to serve the images could make it easier for future improvements.

A good point, and that file can proxy the images as well, supporting absolute paths. But this will exhaust this PR further, anyway.

There are no image manipulation settings, like maximum dimensions, size, or resize features.

As said, this is the basic PR. Further possibilities are there to enhance it. Can be done in this PR as well, if agreed upon.

I think the feature should be disabled by default, or rather well reported in the release blog.

So you propose an additional setting for the behavior introduced? Well, that can be done as well. I guess @dvz was also asking for something similar...

effone avatar May 11 '21 11:05 effone

I think you need to add some hooks where possible. Also, I did try to disable the forum setting Yes, allow [img] code in posts (requires MyCode to be turned on) to se what happened, but apparently the editor stopped working. Is the following piece of code correct?

<script type="text/javascript">
var editorElm = "#message",
	sourceMode = 0,
	partialMode = 0,
	editorLang = (function ($) {
$.sceditor.locale["mybblang"] = {
	"Bold": "Bold",
	"Italic": "Italic",
	"Underline": "Underline",
	"Strikethrough": "Strikethrough",
	"Subscript": "Subscript",
	"Superscript": "Superscript",
	"Align left": "Align left",
	"Center": "Center",
	"Align right": "Align right",
	"Justify": "Justify",
	"Font Name": "Font Name",
	"Font Size": "Font Size",
	"Font Color": "Font Color",
	"Remove Formatting": "Remove Formatting",
	"Cut": "Cut",
	"Your browser does not allow the cut command. Please use the keyboard shortcut Ctrl/Cmd-X": "Your browser does not allow the cut command. Please use the keyboard shortcut Ctrl/Cmd-X",
	"Copy": "Copy",
	"Your browser does not allow the copy command. Please use the keyboard shortcut Ctrl/Cmd-C": "Your browser does not allow the copy command. Please use the keyboard shortcut Ctrl/Cmd-C",
	"Paste": "Paste",
	"Your browser does not allow the paste command. Please use the keyboard shortcut Ctrl/Cmd-V": "Your browser does not allow the paste command. Please use the keyboard shortcut Ctrl/Cmd-V",
	"Paste your text inside the following box:": "Paste your text inside the following box:",
	"PasteText": "Paste Text",
	"Numbered list": "Numbered list",
	"Bullet list": "Bullet list",
	"Undo": "Undo",
	"Redo": "Redo",
	"Rows:": "Rows:",
	"Cols:": "Cols:",
	"Insert a table": "Insert a table",
	"Insert a horizontal rule": "Insert a horizontal rule",
	"Code": "Code",
	"Width (optional):": "Width (optional):",
	"Height (optional):": "Height (optional):",
	"Insert an image": "Insert an image",
	"E-mail:": "E-mail:",
	"Insert an email": "Insert an email",
	"URL:": "URL:",
	"Insert a link": "Insert a link",
	"Unlink": "Unlink",
	"More": "More",
	"Insert an emoticon": "Insert an emoticon",
	"Video URL:": "Video URL:",
	"Video Type:": "Video Type:",
	"Insert": "Insert",
	"Insert a YouTube video": "Insert a YouTube video",
	"Insert current date": "Insert current date",
	"Insert current time": "Insert current time",
	"Print": "Print",
	"View source": "View source",
	"Description (optional):": "Description (optional):",
	"Enter the image URL:": "Enter the image URL:",
	"Enter the e-mail address:": "Enter the e-mail address:",
	"Enter the displayed text:": "Enter the displayed text:",
	"Enter URL:": "Enter URL:",
	"Enter the YouTube video URL or ID:": "Enter the YouTube video URL or ID:",
	"Insert a Quote": "Insert a Quote",
	"Invalid YouTube video": "Invalid YouTube video",
	"Dailymotion": "Dailymotion",
	"MetaCafe": "MetaCafe",
	"Mixer": "Mixer",
	"Vimeo": "Vimeo",
	"Youtube": "Youtube",
	"Facebook": "Facebook",
	"LiveLeak": "LiveLeak",
	"Insert a video": "Insert a video",
	"PHP": "PHP",
	"Maximize": "Maximize"
}})(jQuery);,
	postEmbed = { enabled: 1, host: "local", clientKey: "" },
	optEditor = {
		style: "http://localhost/mybb/jscripts/sceditor/styles/jquery.sceditor.mybb.css?ver=1823",
		rtl: 0,
		emoticonsEnabled: true,
		emoticons: {
			dropdown: { ":)": "http://localhost/mybb/images/smilies/smile.png",";)": "http://localhost/mybb/images/smilies/wink.png",":cool:": "http://localhost/mybb/images/smilies/cool.png",":D": "http://localhost/mybb/images/smilies/biggrin.png",":P": "http://localhost/mybb/images/smilies/tongue.png",":rolleyes:": "http://localhost/mybb/images/smilies/rolleyes.png",":shy:": "http://localhost/mybb/images/smilies/shy.png",":(": "http://localhost/mybb/images/smilies/sad.png",":at:": "http://localhost/mybb/images/smilies/at.png",":angel:": "http://localhost/mybb/images/smilies/angel.png",":@": "http://localhost/mybb/images/smilies/angry.png",":blush:": "http://localhost/mybb/images/smilies/blush.png",":s": "http://localhost/mybb/images/smilies/confused.png",":dodgy:": "http://localhost/mybb/images/smilies/dodgy.png",":exclamation:": "http://localhost/mybb/images/smilies/exclamation.png",":heart:": "http://localhost/mybb/images/smilies/heart.png",":huh:": "http://localhost/mybb/images/smilies/huh.png",":idea:": "http://localhost/mybb/images/smilies/lightbulb.png",":sleepy:": "http://localhost/mybb/images/smilies/sleepy.png",":-/": "http://localhost/mybb/images/smilies/undecided.png", }, // Emoticons to be included in the dropdown
			more: { ":cry:": "http://localhost/mybb/images/smilies/cry.png",":sick:": "http://localhost/mybb/images/smilies/sick.png",":arrow:": "http://localhost/mybb/images/smilies/arrow.png",":my:": "http://localhost/mybb/images/smilies/my.png", }, // Emoticons to be included in the more section
			hidden: {  } // Emoticons that are not shown in the dropdown but will still be converted. Can be used for things like aliases
		},
		toolbar: "bold,italic,underline,strike|left,center,right,justify|font,size,color,removeformat|horizontalrule,image,email,link,unlink|video,emoticon|bulletlist,orderedlist|code,php,quote|maximize,source"
	};
</script>
<script type="text/javascript" src="http://localhost/mybb/jscripts/editor.js?ver=1827"></script>

I think (jQuery);, is wrong.

Sama34 avatar May 11 '21 20:05 Sama34

Hmm. OK. But why it is problematic as what it is now? Admins gonna use either of the services anyway

We have followed this reasoning before, mainly to avoid confusion and keep both data fields stored in case the admin wants to toggle the service being used. It is not a big deal, if somebody else thinks it isn't worth it.

So you propose an additional setting for the behavior introduced?

I don't think @dvz did mean this in his comment above (which I also brought before), what I meant is that this feature being enabled by default could be problematic for admins when they update their boards, as some even disable the attachment system.

Also, although you seem to be using the core upload functions, I'm unsure your code will work with CDN settings, as at least I noticed you use $mybb->settings['bburl'] directly completely ignoring the possibility of $mybb->asset_url.

Sama34 avatar May 11 '21 20:05 Sama34

Just recalled, I think this should be shipped along the docs update required, if not for the feature description indeed for the new uploads path in the install and configuration section (permissions doc, etc).

Sama34 avatar May 11 '21 21:05 Sama34