@emanuelet @Pad_Pors @erlend_sh Interesting! I just got back from a short holiday, so just digesting this now.
Here’s are some more ways the plugins differ
Image sizes in topic list
We are both saving the thumbnail to a topic_custom_field. However, I have also
Optimized the the thumbnails into two different sizes: 100x100 and 200x200 (for retina displays);
Saved the thumbnails into a custom JSON object (this was surprisingly difficult); and
Serialized the new topic_custom_field to the topic list.
These changes caused the issues that some have had with my plugin, partly because there are some type issues with JSON custom fields in Discourse.
Without similar logic your plugin is rendering the images full size in the topic list using the existing
image_url field. This puts a significant strain on resources when used at scale. If the topic list has a number of topics with featured images, all of those full-sized featured images are being loaded when the topic list is rendered.
This is one of the key issues to address with this kind of plugin.
Use of existing Discourse logic
You have created a few new ways of handling things. While there’s nothing wrong with creating new logic, there will definitely be some issues down the line (and currently) with maintaining it. For example, for the url handling logic you’ve added. Whenever possible it is better to use the existing Discourse logic that has already been battle-tested. Note that the hard-coded
SiteSetting.s3_upload_bucket + ".s3-ap-southeast-2.amazonaws.com/" will not work for most people.
New upload button in the composer
I’m a little confused as to what the thinking is here? The new upload button you’ve added seems to do the same thing as the existing upload button. Could you explain what it’s for a bit more?
Featured image appears above topic and does not appear in first post
This is an interesting feature. I can imagine that some people will not want it however. Might be best to add this as a setting, rather than by default.
Appearance of featured image cannot be set on a per-category basis
This ‘per category’ setting was a feature asked for by a number of people, so I changed the functionality to be set on a per-category basis.
@emanuelet All that said, well done! I learnt some new things reading your code.
So what should we do from here… if you’re up for it, we should combine efforts rather than work separately on the same feature. First, I’ll let you digest and respond to my thoughts. Maybe I’ve missed some things. Then we can talk about how to go about collaborating.