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

Added document tool to upload images #42

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

finnboeger
Copy link
Contributor

@finnboeger finnboeger commented Apr 26, 2020

No description provided.

@finnboeger
Copy link
Contributor Author

This also fixes #20. Currently there is no size/quality reduction, however it could be done using e.g. mozjpeg.

@lovasoa
Copy link
Owner

lovasoa commented Apr 27, 2020

The size/quality reduction could be done prior to sending on the client side with a canvas, with the server only validating that the document does not exceed the maximum size.

@lovasoa lovasoa force-pushed the master branch 2 times, most recently from d2ff89d to 189ebbe Compare April 27, 2020 22:56
server/sockets.js Outdated Show resolved Hide resolved
server/sockets.js Outdated Show resolved Hide resolved
@finnboeger
Copy link
Contributor Author

Option to disable tools added in #66

@finnboeger finnboeger marked this pull request as ready for review May 4, 2020 17:48
Co-authored-by: Ophir LOJKINE <[email protected]>
Comment on lines +101 to +121
if (message.data.type === "doc") {
boardData = await getBoard(boardName);

if (boardData.existingDocuments >= config.MAX_DOCUMENT_COUNT) {
console.warn("Received too many documents");
return;
}

if (message.data.data.length > config.MAX_DOCUMENT_SIZE) {
console.warn("Received too large file");
return;
}

boardData.existingDocuments += 1;
} else if (message.data.type === "delete") {
boardData = await getBoard(boardName);

if (boardData.board[message.data.id].type === "doc") {
boardData.existingDocuments -= 1;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably be handled in the BoardData class

Copy link
Owner

Choose a reason for hiding this comment

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

Also, we should also verify the provided URL, to check that it's a data URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we would still broadcast the large files to all users in that case

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, you are right, I hadn't thought about that. I still think the board-specific code should happen in BoardData, but we should probably set the httpBufferSize option of socket.io in addition, to avoid having to handle and dispatch large messages.

ctx.canvas.width = image.width * scale;
ctx.canvas.height = image.height * scale;
ctx.drawImage(image, 0, 0, image.width * scale, image.height * scale);
var dataURL = ctx.canvas.toDataURL("image/webp", 0.7);
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably use jpeg by default here... We can make it configurable, though.

https://caniuse.com/#feat=webp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jpeg does not support transparency, which would require us to check what kind of image was uploaded and if it made use of the alpha channel.
If it is a png with transparency we would not be able to compress it other than by shrinking the dimensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would suggest using a webp polyfill, e.g. https://github.com/chase-moskal/webp-hero

Copy link
Owner

Choose a reason for hiding this comment

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

Let's maybe start with JPEG, and then add support for webp, with a polyfill, in a second pull request ? webp-hero won't work as-is with data urls...

Comment on lines +88 to +89
img.setAttribute("width", 400*aspect);
img.setAttribute("height", 400);
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably have a maximum and a minimum image size, and scale only images that do not fit.

@fklement
Copy link

@finnboeger
I just checked out your PR. Pretty nice so far.
Do you plan on adding support for moving the imported image?

@finnboeger
Copy link
Contributor Author

I plan on adding general support to move and resize shapes which would include images as well.
However the earliest I expect to have time to continue working on this would be in close to a month.

@lovasoa
Copy link
Owner

lovasoa commented Jul 21, 2020

Doesn't the existing hand tool work to move images created with this patch?

@finnboeger
Copy link
Contributor Author

I am not up to speed regarding the tools that have been added since my last commit so I can't comment on that right now

@TechRova
Copy link

This also fixes #20. Currently there is no size/quality reduction, however it could be done using e.g. mozjpeg.

The demo website dont have image adding feature

@stranljip
Copy link

I would love to see this feature. Is this issue still in progress or has it become stale?

@lovasoa
Copy link
Owner

lovasoa commented Oct 1, 2020

I just haven't had the time to work on it yet.

@lordneeko
Copy link

In addition, the ability to "paste" into the window and have it show up within the canvas would be desirable. Perhaps that is a follow-on PR.

@Y0ngg4n
Copy link

Y0ngg4n commented May 18, 2021

any updates here?

@keraldi
Copy link

keraldi commented Dec 14, 2021

Hi, I'd also really like this feature. Can anything be done to help the progress?

@gigabyteservice
Copy link

gigabyteservice commented Mar 3, 2022

this is producing an error with downloading functionality/tool

@mrducky4
Copy link

mrducky4 commented Apr 2, 2022

this is producing an error with downloading functionality/tool

@gigabyteservice I saw a problem with this, the downloaded SVG would not open if the board included an uploaded image. I fixed it by adding the attribute xmlns:xlink="http://www.w3.org/1999/xlink" to the <svg id="canvas"> element in board.html.

type: "doc",
data: dataURL,
size: size,
w: this.width * scale || 300,
Copy link

Choose a reason for hiding this comment

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

This uses the newly reduced value for scale. I think it was meant to use the previous value, which is the value used with drawImage()

boardData.existingDocuments = 0;
for (id in boardData.board) {
boardData.validate(boardData.board[id]);
if (boardData.board[id].type === "doc") existingDocuments += 1;
Copy link

Choose a reason for hiding this comment

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

Should this be boardData.existingDocuments?

@@ -26,6 +26,12 @@ module.exports = {
/** Maximum value for any x or y on the board */
MAX_BOARD_SIZE: parseInt(process.env['WBO_MAX_BOARD_SIZE']) || 65536,

/** Maximum size of uploaded documents default 1MB */
MAX_DOCUMENT_SIZE: parseInt(process.env['WBO_MAX_DOCUMENT_SIZE']) || 1048576,
Copy link

Choose a reason for hiding this comment

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

Large images failed for me, causing a transport error. The socket.io default maxHttpBufferSize is 1e6, so I changed this to 1e6 - 500.

@NyaomiDEV
Copy link

is there still some interest in merging this PR?

@holema
Copy link
Contributor

holema commented Oct 25, 2022

@lovasoa and @NyaomiDEV , it would be great to have the feature. I`m still missing this feature so much

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

Successfully merging this pull request may close these issues.