Ckeditor5: Review contents of this package

[Reinmar]

The main src/ dir contains now multiple plugins (only one is useful – ImageUpload) and some utils.

I'd propose something like:

src/ ui/ utils/ all the file* stuff imageupoad/ imageuploadcommand.js imageuploadengine.js imageuploadprogress.js imageupload.js imageuploadbutton.js

Also, I lost a bit the confidence that image upload should be together with upload. It surprised me here (I forgot about this decision in the meantime). I think it's weird now. The more I look at this the more it looks bad because of all this unnecessary coupling and confusion. Instead of increasing discoverability, we might've actually lowered it.

[Reinmar]

After we clean this package it also needs a better description cause the current "Upload feature." is rather confusing (and incorrect).

[Reinmar]

Also, I lost a bit the confidence that image upload should be together with upload. It surprised me here (I forgot about this decision in the meantime). I think it's weird now.

I got this feeling again now, when refactoring all the commands.

Also, ImageUploadCommand is troubling me. First of all, it is not only about upload – it also inserts the image so the name is confusing. But most importantly – I'm not entirely sure whether it should be a command at all.

I've been thinking about some more general InsertImage command which could accept various arguments – a File like now and e.g. image data directly (i.e. src and alt) because it's now unclear how someone could programmatically insert an image. However, a master command would create references to the entire upload package so it'd be really heavy.

So perhaps we could have more granular methods – InsertImageByFile, InsertImage (by data), etc.

Or maybe I'm overcomplicating this and we could have UploadImage (by file) and InsertImage (by data).

[pjasiun]

The point of having it as a command is that one may disable it, for instance, if there is no connection with the server.

[pjasiun]

As for keeping image plugins here: it was because of the idea that whenever one will want to handle upload, he may want to enable it for all types of content that can be uploaded: images, media, files. If they are in the one place it's simpler to find all needed plugins. However, I also found it strange recently.

[Reinmar]

Also, ImageUploadCommand is troubling me. First of all, it is not only about upload – it also inserts the image so the name is confusing. But most importantly – I'm not entirely sure whether it should be a command at all.

I had this feeling again now, while getting back to this feature. There's something wrong with this command. It does too much and is named incorrectly.

[Reinmar]

~I've just realised that the image upload feature is actually implemented by... ImageUploadProgress which makes little sense. Progress should be added by one plugin and handling for dropped and pasted images by another plugin.~

OK, I got confused by the number of these plugins. The uploading capabilities are implemented by ImageUplodEngine, while progress is added in ImageUploadProgress. I got confused because ImageUpload requires ImageUploadProgress and because ImageUploadProgress doesn't really need ImageUploadEngine.

So, these coupling is unnecessary. ImageUploadProgress should be optional and should not unnecessarily depend on ImageUploadEngine. Then, ImageUpload will need to require both of these plugins which will make it clear what does what.

Compare this to inheritance chains. Requiring one plugin in another one is a bit like saying that one plugin inherits from the other one. The less often we do that, the better.

Strongly related to https://github.com/ckeditor/ckeditor5/issues/425#issuecomment-333191475.

[Reinmar]

Another thing is that ImageUpload requires ImageUploadButton. Why? These are independent features.

[Reinmar]

Blocked by https://github.com/ckeditor/ckeditor5/issues/488.

[Reinmar]

And Adapter should be called UploadAdapter (see https://stackoverflow.com/questions/46765197/how-to-enable-image-upload-support-in-ckeditor-5/46773627#46773627 why). Don't forget to update my answer after the rename is done.

[Reinmar]

One more detail to fix:

    /**
     * Performs image loading. Image is read from the disk and temporary data is displayed, after uploading process
     * is complete we replace temporary data with target image from the server.
     *
     * @protected
     * @param {module:upload/filerepository~FileLoader} loader
     * @param {module:engine/model/element~Element} imageElement
     */
    load( loader, imageElement ) {

So is it protected or not?

[jodator]

So to sum up:

1. Almost everybody agrees that generic ImageUpload* classes location inside Upload thing is at least confusing - so the question is do we do something with this? I can see two options:

   a) leave as is (but group files as @Reinmar proposed earlier)
   b) extract ckeditor5-imageupload as independent plugin
   c) move ImageUpload* to image

   a) vs b) - one is less discoverable but have less plugins the latter better defines features/plugins roles and is more discoverable

   b) vs c) - I can see a situation that one would like to have InsertImage without UploadImage - ie inserting an image by its URI (look below).

2. Plugins coupling, naming and requires() of each other.

   The requires() chain should be flat like this:

   ImageUpload  ----+--->   ImageUploadEditing          | was ImageUploadEngine
               |
               +--->   ImageUploadProgressEditing  | was   ImageUploadProgressEngine
               |
               +--->   ImageUploadUI               | was ImageUploadButton
   

3. Some classes requires a bit better description as what they are meant for.

4. The ImageUploadCommand require some work as it:

   • the name does not reflect that it: uploads image and then inserts it
   • is required for toolbar button states handlng (ie disable when no server conn)

   Maybe creating insertImage command makes sense - it might be used by ImageUpload feature after successful upload - also might be helpful for creating an InsertImage feature that will insert image by given file URI.

5. Minor refactoring:

   • Rename Adapter to UploadAdapter
     

     - make load() method private (unless someone can tell my why this one is protected)

Things that I've found odd:

• The insertImage toolbar name vs uploadImage used for command, feature name and other plugins - shouldn't this be uploadImage? (corresponds to point 4).
• The FileRepository file have inner FileLoader class - shouldn't there be one class one file rule? I get that it is "internal" class of FileRepository and probably isn't useful as import elsewhere but.... I don't like it ()

[Reinmar]

@jodator, I'm afraid that you didn't notice that this is about moving stuff from ckeditor5-upload to ckeditor5-image. Could you update your comment?

[jodator]

@Reinmar yeah... there were so many ImageUpload there that I've fixated on that. I've updated first point to reflect current state and my feelings on UploadImage vs InsertImage.

[jodator]

So after F2F talks we're going to:

1. Use option c)
2. decouple and rename Plugins
3. Add minimal descriptions to classes
4. leave ImageUploadCommand as is but the toolbar button will be renamed to uploadImage
5. as in summary.

[Reinmar]

leave ImageUploadCommand as is but the toolbar button will be renamed to uploadImage

To explain this choice – we may want to implement insertImage button one day. It would allow multiple methods of inserting images – from the file system, by URL, etc. Just like we designed in https://github.com/ckeditor/ckeditor5-image/issues/16.

So, to enable that, let's focus the current button on uploading. This way, we'll be able to later add a more comprehensive feature without touching this one.

[jodator]

@Reinmar as we're renaming Adapter to UploadAdapter in FileRepository should we change also:

• FileRepository#createAdapter() to createUploadAdapter().
• filerepository-no-adapter error to filerepository-no-upload-adapter.

ps.: It makes a sense for me as adapter alone in above might be not precise enough (one might think of FileAdapter.

[Reinmar]

Makes sense to me. WDYT @pjasiun?

[pjasiun]

Yep, even recently someone asks what "adapter" is. Using "upload adapter" will make it cleaner.