humhub / humhub-prosemirror

Other
17 stars 8 forks source link

[v1.16] Use of FileHandler #121

Closed luke- closed 7 months ago

luke- commented 1 year ago

Create File Handler

image image

Open File Handler

Also in RichtText embedded files, should be open using the FileHandler Open Dialog, instead of immediately start the download.

image image

spoorun commented 10 months ago

That would be great, though 'insert rule' (horizontal line insert) would need a separate a new separate entry

yurabakhtin commented 10 months ago

@luke- What I have researched:

I see another approach:

insert-icon

So what we have to do to implement this solution:

luke- commented 9 months ago

@yurabakhtin Thanks for the research.

Hmm, I'm not a big fan of the solution, that we always need to upload the file to the file list first and then add it to the Markdown editor.

Some arguments against this:

Without digging deeper into the code, we need two parts:

  1. A ProseMirror Plugins which adds a dropdown with all available create file handlers. When click, the File Handler Modal is opend.
  2. ProseMirror needs to detect a newly created file e.g. via events done here: https://github.com/humhub/text-editor/blob/main/resources/js/humhub.text_editor.js#L51 - and should inject a Markdown link as suggested.

Do you think we can use the existing event? We may also have to introduce a new event here and refactor the FileHandler a little.

yurabakhtin commented 9 months ago

@luke-

Some arguments against this:

  • it requires an extra step
  • on large Markdown documents e.g. Wiki Page the file section below is not always visible
  • some Markdown Fields e.g. Calendar Description don't have an extra file section
  • Not sure on the long run we have in Posts these two file upload options (Button and Upload File in Markdown)

Ok, I agree with it.

Without digging deeper into the code, we need two parts:

  1. A ProseMirror Plugins which adds a dropdown with all available create file handlers. When click, the File Handler Modal is opend.
  2. ProseMirror needs to detect a newly created file e.g. via events done here: https://github.com/humhub/text-editor/blob/main/resources/js/humhub.text_editor.js#L51 - and should inject a Markdown link as suggested.

Do you think we can use the existing event? We may also have to introduce a new event here and refactor the FileHandler a little.

Yes, it seems w can try to use the JS event humhub:file:created, also I find another event humhub:file:created.cfiles. We need to investigate deeply how to handle a file after uploading and how we can insert it into the richtext editor. I guess if we will implement the inserting inline tag into richtext editor through new ProseMirror Plugin, then original event will continue inserts an uploaded file into the uploading area under form as it was before, i.e. result of the uploaded file will be displayed in two places.

yurabakhtin commented 9 months ago

@luke- I have started this in PR https://github.com/humhub/humhub-prosemirror/pull/134:

file-handlers

The links are just copied from the dropdown and action link.click() is called, i.e. the new added menu items work as original, we still need find a solution how to process result on event humhub:file:created.

luke- commented 9 months ago

@yurabakhtin Thanks, looks good.

yurabakhtin commented 8 months ago

@luke- Currently I could implement only this way:

file-handler

I have 2 problems:

yurabakhtin commented 7 months ago

From discussion: Set global var on click file handler and on Normal link and on RichTextToolbar link.

yurabakhtin commented 7 months ago

From discussion: Set global var on click file handler and on Normal link and on RichTextToolbar link.

Commit https://github.com/humhub/humhub-prosemirror/pull/134/commits/8fc807b2524d8e2fe922e753ae23340f57770cff.

luke- commented 7 months ago

@yurabakhtin Why do we need this?

I have 2 problems:

  • [Word.docx](file-guid:090a402a-50e1-464a-83fa-39250b261123) - we should create some additional option to know the markdown link will be converted to link for viewing instead of current/default downloading url, maybe like [Word.docx](file-guid:090a402a-50e1-464a-83fa-39250b261123 mode=view) but I am not sure how it is possible from markdown parser, if it is, then we should add the mode=view for each link which is inserted by new plugin, but we need firstly solve the 1st point above.

I would prefer file links to always trigger the file handler modal. Or are there situations where this should not be the case?

yurabakhtin commented 7 months ago

I would prefer file links to always trigger the file handler modal. Or are there situations where this should not be the case?

@luke- Ok, but for this we need a small changes in core https://github.com/humhub/humhub/pull/6824 because the modal window uses URL /file/view, also these changes https://github.com/humhub/humhub-prosemirror/pull/134/commits/d5a961e8753eb500311329c577086125ba7c9a08 need in order to use the new mode view.

But please note the modal window with options/buttons of File Handler is opened only when at least one custom File Handler exists in system, but by default the modal window it not opened, instead we have a simple downloading and it depends on each File, it is from PHP side here https://github.com/humhub/humhub/blob/master/protected/humhub/modules/file/libs/FileHelper.php#L88-L89:

$fileHandlers = FileHandlerCollection::getByType([
    FileHandlerCollection::TYPE_VIEW,
    FileHandlerCollection::TYPE_EXPORT,
    FileHandlerCollection::TYPE_EDIT,
    FileHandlerCollection::TYPE_IMPORT
], $file); // <= here $file is instance of humhub\modules\file\models\File
if (count($fileHandlers) === 1 && $fileHandlers[0] instanceof DownloadFileHandler) {
    // Download link
} else {
    // Modal window with custom File Handler
}

For JS side we cannot detect this, so in all cases the modal window will be opened, and if no custom File Handler then it will looks like this:

modal-js

from PHP side the modal file view url is replaced with simple downloading url like this:

download-file

Do you agree this?

luke- commented 7 months ago

@yurabakhtin It is ok if a modal always opens within the richtext, even if only one download is possible.

In my tests, e.g. with the https://github.com/humhub/text-editor module, it did not add the created text file correctly in the rich text.

Otherwise it looks very good.

luke- commented 7 months ago

We also need to rearrange the menu: image

yurabakhtin commented 7 months ago

In my tests, e.g. with the https://github.com/humhub/text-editor module, it did not add the created text file correctly in the rich text.

@luke- For me it works correctly:

text-editor

yurabakhtin commented 7 months ago

We also need to rearrange the menu: image

@luke- Done in commit https://github.com/humhub/humhub-prosemirror/pull/134/commits/d696fd4a02ab4fec7df25f39628d77aa31e58eae, but please note there we don't use font-awesome icons, there svg path is used, so I could find this tool https://icomoon.io/app/#/select where I have found almost similar upload icon but it has a bit difference:

icons

luke- commented 7 months ago

In my tests, e.g. with the https://github.com/humhub/text-editor module, it did not add the created text file correctly in the rich text.

@luke- For me it works correctly:

@yurabakhtin Now it also works for me as described by you. However, the file appears twice.

image

yurabakhtin commented 7 months ago

@luke- I have removed the menu item "Image" yesterday as you requested on the screenshot, but what if user wants to add an image from some existing URL without uploading, maybe we should revert that?

image-menu-item

luke- commented 7 months ago

@luke- I have removed the menu item "Image" yesterday as you requested on the screenshot, but what if user wants to add an image from some existing URL without uploading, maybe we should revert that?

@yurabakhtin Thanks for the hint.

We have discussed it again internally and decided to leave out the "picture" icon. In case of need, users can add the image via the Markdown Source Mode.

The line icon will also be removed also for now.

Please just comment out these icons

yurabakhtin commented 7 months ago

@luke- Ok I have removed the horizontal line icon:

icons-richtext


Now it also works for me as described by you. However, the file appears twice.

image

I don't find how to implement this yet, because we can handle this by event humhub:file:created:

I.e. the same event is used, I tried to use and evt.preventDefault() and evt.stopPropagation() and evt.stopImmediatePropagation() but it didn't help, maybe later I will can find a solution.

luke- commented 7 months ago

@yurabakhtin Ok thanks. Let me know when I can help. Maybe Serh kann also help.

Didn't we introduced some global var to determine which triggered the file upload? e.g. isProsemirrorFileHandlerActive? Could we check this here? https://github.com/humhub/humhub/blob/master/protected/humhub/modules/file/resources/js/humhub.file.js#L533

yurabakhtin commented 7 months ago

@luke- Yes, it works, thanks for the advice.

luke- commented 7 months ago

@yurabakhtin Thanks it works as expected!