Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v1.16] Use of FileHandler #121

Closed
luke- opened this issue Aug 18, 2023 · 22 comments · Fixed by #134
Closed

[v1.16] Use of FileHandler #121

luke- opened this issue Aug 18, 2023 · 22 comments · Fixed by #134
Assignees

Comments

@luke-
Copy link
Contributor

luke- commented Aug 18, 2023

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
Copy link

spoorun commented Oct 23, 2023

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

@yurabakhtin
Copy link
Contributor

@luke- What I have researched:

  • The menu buttons "Upload", "Image", "Horizontal rule" are implemented as separate prosemirror plugins, e.g. https://github.com/humhub/humhub-prosemirror/tree/master/src/editor/core/plugins/upload
  • The creating/uploading new file from HumHub side uses own JS, e.g. to upload simple file the JS method file.upload https://github.com/humhub/humhub/blob/master/protected/humhub/modules/file/resources/js/humhub.file.js#L565 is called, and on the finish uploading a result is inserted into the preview block where we see what file was uploaded and have an icon to delete it.
  • Also other custom JS methods are loaded from external modules, like OnlyOffice uses onlyoffice.createSubmit.
  • If we will call the JS methods(without modification) from the RichText editor then result will be inserted into the preview area under form and not in the editor area as expected. I.e. the methods should be modified somehow, I cannot know how it will be easy.
  • I think it will be not easy to implement the integration, at least the integration will not be automatically for each module, i.e. for each module like OnlyOffice we should create the integration separately.

I see another approach:

  • As I understand main request is to allow instead of direct downloading we should allow to display the File Handler Dialog with button "View file", "Download", etc., if yes then:
  • we could add new icon(+) for each uploaded file like this:

insert-icon

  • on click it we will insert a tag like [test.docx](file-guid-view:5759ded6-e54c-4f01-b078-76f8cb1d09a8), this tag will be converted to link to open the File Handler Dialog,
  • please note currently file markdown tag has structure like [test.docx](file-guid:5759ded6-e54c-4f01-b078-76f8cb1d09a8), I suggest to introduce new different like file-guid-view or view-guid or some other, so such new special file tag will be converted to view file link with html like:
  • <a href="/file/view?guid=fed18c94-89c8-465f-b426-a4c637c8fa81" data-target="#globalModal">test.docx</a> instead of:
  • <a href="/file/file/download?guid=5759ded6-e54c-4f01-b078-76f8cb1d09a8">test.docx</a>

So what we have to do to implement this solution:

  • add the new (+) icon (from core side)
  • implement JS code to insert a file markdown tag with new flag like view-guid directly into the current cursor position of RichText editor. (I think the JS code can be implemented from HumHub core side, and it will works for all files uploaded or created by any external module.)
  • Parse new file tags with view-guid to link with url to /file/view?guid=. I think small changes will be required from Prosemirror render side.
  • Some changes might be needed in humhub\modules\content\widgets\richtext\converter\RichTextToHtmlConverter to parse the new tags, but from RichTextToEmailHtmlConverter we need to parse new tags as old because no need(it is impossible) to open the File Handler Dialog from email side.

@luke-
Copy link
Contributor Author

luke- commented Nov 21, 2023

@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:

  • 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)

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
Copy link
Contributor

@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 yurabakhtin linked a pull request Dec 15, 2023 that will close this issue
@yurabakhtin
Copy link
Contributor

@luke- I have started this in PR #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-
Copy link
Contributor Author

luke- commented Dec 15, 2023

@yurabakhtin Thanks, looks good.

@yurabakhtin
Copy link
Contributor

@luke- Currently I could implement only this way:

file-handler

I have 2 problems:

  • When the form is submitted from from Modal Window I don't know how to know it was called from RichText editor and not from normal file handler. For example, the "Save" button calls js function documenta.createSubmit(), i.e. it seems we need to have additional fix from each File Handler because I don't imagine how to do this only from the Prosemirror side.
  • [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.

@yurabakhtin
Copy link
Contributor

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

@yurabakhtin
Copy link
Contributor

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

Commit 8fc807b.

@luke-
Copy link
Contributor Author

luke- commented Jan 29, 2024

@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
Copy link
Contributor

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 humhub/humhub#6824 because the modal window uses URL /file/view, also these changes d5a961e 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-
Copy link
Contributor Author

luke- commented Feb 2, 2024

@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-
Copy link
Contributor Author

luke- commented Feb 2, 2024

We also need to rearrange the menu:
image

@yurabakhtin
Copy link
Contributor

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
Copy link
Contributor

We also need to rearrange the menu: image

@luke- Done in commit d696fd4, 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-
Copy link
Contributor Author

luke- commented Feb 6, 2024

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
Copy link
Contributor

@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-
Copy link
Contributor Author

luke- commented Feb 7, 2024

@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
Copy link
Contributor

@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-
Copy link
Contributor Author

luke- commented Feb 8, 2024

@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
Copy link
Contributor

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

@luke- luke- changed the title Use of FileHandler [v1.16] Use of FileHandler Feb 8, 2024
@luke- luke- closed this as completed Feb 8, 2024
@luke-
Copy link
Contributor Author

luke- commented Feb 8, 2024

@yurabakhtin Thanks it works as expected!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants