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.

Sign up for GitHub

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

Jump to bottom

Insert images from local device (or link) #1900

Merged
merged 63 commits into from
Jan 3, 2022
Merged

Insert images from local device (or link) #1900

merged 63 commits into from
Jan 3, 2022

Conversation

julien-nc
Copy link
Member

@julien-nc julien-nc commented Oct 18, 2021 •
edited
Loading

Fixes #58

Here is a basic implementation of 2 new options to insert images:

  • Upload from local device
  • Download from http link

In both cases, the image is then stored in the /Text directory which is created if it does not exist.
An alternative mentioned in #58 (comment) would be to store uploaded images in a directory next to the markdown file.

Todo

  • Menu action for local file insertion
  • Handling file upload
  • Display uploaded image
  • Handle and display upload errors
  • Menu action for image link insertion
  • Handling link download/store
  • Display image from link
  • Handle and display link insertion errors
  • Change menu item naming
  • Change attachment folder name
  • Use the attachment folder when inserting images from Files
  • Handle .md file copy/move/delete with File events
  • Cleanup attachment folder when images are removed from markdown content

Gifcording 😁

link

@julien-nc julien-nc added enhancement New feature or request 2. developing labels Oct 18, 2021
@julien-nc julien-nc self-assigned this Oct 18, 2021
@julien-nc julien-nc force-pushed the enh/58/insert-local-images branch 2 times, most recently from b0e0099 to 93d7fa5 Compare October 20, 2021 13:38
@julien-nc julien-nc added this to the Nextcloud 23 milestone Oct 20, 2021
@julien-nc julien-nc mentioned this pull request Oct 21, 2021
Closed
This was referenced Oct 25, 2021
Merged
Merged
@blizzz blizzz mentioned this pull request Nov 3, 2021
Closed
18 tasks
@julien-nc julien-nc mentioned this pull request Nov 4, 2021
Merged
4 tasks
@skjnldsv skjnldsv mentioned this pull request Nov 8, 2021
Merged
23 tasks
mejo-
mejo- reviewed Nov 22, 2021
View reviewed changes
src/components/MenuBar.vue Outdated Show resolved Hide resolved
src/components/MenuBar.vue Show resolved Hide resolved
src/components/MenuBar.vue Outdated Show resolved Hide resolved
lib/Controller/ImageController.php Show resolved Hide resolved
@julien-nc julien-nc modified the milestones: Nextcloud 23, Nextcloud 24 Nov 22, 2021
@julien-nc
Copy link
Member Author

This now includes "attachment folder" changes. It's been rebased on master (so it must be used in NC 24).

The TODO list in this PR's description has been updated.

@julien-nc julien-nc requested a review from mejo- November 22, 2021 12:14
juliushaertl
juliushaertl reviewed Nov 22, 2021
View reviewed changes
lib/Service/ImageService.php Outdated
return null;
}
if ($imageFile instanceof File) {
// return $imageFile;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • To be discussed if we still need the original file for gif support for example

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been changed and will work if we want to support GIFs. If the mime type is not supported by the preview generator, the original file is returned now. This also fixes the svg display because there is no related preview provider by default.

@julien-nc
Copy link
Member Author

/compile amend

@mejo-
Copy link
Member

mejo- commented Jan 3, 2022

We can now merge or wait until a pairing session about Cypress and some more tests. As you wish.

I don't mind to merge now and add cypress tests later (if we don't forget about it 😬). But the failing psalm tests should be tackled first, no?

@julien-nc julien-nc force-pushed the enh/58/insert-local-images branch 2 times, most recently from 69b91e2 to 4879432 Compare January 3, 2022 10:23
@julien-nc
Copy link
Member Author

/compile amend

@nextcloud-command
fix some psalm false positives
dea3a59 
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
mejo-
mejo- approved these changes Jan 3, 2022
View reviewed changes
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice work 👌

@juliushaertl juliushaertl merged commit f33e0e5 into master Jan 3, 2022
@juliushaertl juliushaertl deleted the enh/58/insert-local-images branch January 3, 2022 10:58
This was referenced Jan 3, 2022
Closed
Closed
@juliushaertl
Copy link
Member

Filed #2049 for the cypress tests

@jancborchardt jancborchardt mentioned this pull request Jan 4, 2022
Closed
@tobiasKaminsky
Copy link
Member

Is this also ready for clients via our DirectEditingMobileInterface?

@julien-nc
Copy link
Member Author

@tobiasKaminsky All related endpoints are public and use the session edition token and optionally the public share token to authenticate.
So it is supposed to be ok but in reality it partially works. I've tried it on our Android app:

  • Inserting an image from a link ✔️
  • Uploading a local image ❌ The <input type=file> does not work when rendered in the Android app.
  • Serving the images ❌ 2 mistakes:
    • The share token presence detection is broken in
    • The ImageController does not get the user ID from the session

I'll make a PR to fix the 2 image serving issues.
I don't know about the local image upload issue. Do you think we should just hide this feature on mobile apps? Or is there a way to get a system file selector when clicking on such file input?

julien-nc pushed a commit that referenced this pull request Jan 7, 2022
refs #1900 fix image serving in direct editing, avoid passing empty s…
51c3365 
…hareToken, use the edition session to get the user ID

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
This was referenced Jan 7, 2022
Merged
Merged
julien-nc pushed a commit that referenced this pull request Jan 9, 2022
refs #1900 fix image serving in direct editing, avoid passing empty s…
99c58ed 
…hareToken, use the edition session to get the user ID

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@tobiasKaminsky
Copy link
Member

I don't know about the local image upload issue. Do you think we should just hide this feature on mobile apps? Or is there a way to get a system file selector when clicking on such file input?

This is possible, as we do it with Collabora.

In this regard, we have this nextcloud/server#16132, but want to wait for your side (move RichDocuments to directEditing)

julien-nc pushed a commit that referenced this pull request Jan 10, 2022
refs #1900 fix image serving in direct editing, avoid passing empty s…
466a6b5 
…hareToken, use the edition session to get the user ID

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
This was referenced Jan 13, 2022
Closed
Closed
Closed
@juliushaertl juliushaertl mentioned this pull request Mar 1, 2022
Closed
@vinicius73 vinicius73 mentioned this pull request Aug 17, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliushaertl juliushaertl juliushaertl approved these changes

@mejo- mejo- mejo- approved these changes

@azul azul Awaiting requested review from azul

Assignees

@julien-nc julien-nc

Labels
4. to release enhancement New feature or request
Projects
None yet
Milestone
Nextcloud 24
Development

Successfully merging this pull request may close these issues.

Ability to insert images from local device (or link)
4 participants
@julien-nc @juliushaertl @mejo- @tobiasKaminsky