-
-
Notifications
You must be signed in to change notification settings - Fork 423
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
base: master
Are you sure you want to change the base?
Conversation
This also fixes #20. Currently there is no size/quality reduction, however it could be done using e.g. mozjpeg. |
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. |
d2ff89d
to
189ebbe
Compare
Option to disable tools added in #66 |
Co-authored-by: Ophir LOJKINE <[email protected]>
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; | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
img.setAttribute("width", 400*aspect); | ||
img.setAttribute("height", 400); |
There was a problem hiding this comment.
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.
@finnboeger |
I plan on adding general support to move and resize shapes which would include images as well. |
Doesn't the existing hand tool work to move images created with this patch? |
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 |
The demo website dont have image adding feature |
I would love to see this feature. Is this issue still in progress or has it become stale? |
I just haven't had the time to work on it yet. |
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. |
any updates here? |
Hi, I'd also really like this feature. Can anything be done to help the progress? |
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 |
type: "doc", | ||
data: dataURL, | ||
size: size, | ||
w: this.width * scale || 300, |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
is there still some interest in merging this PR? |
@lovasoa and @NyaomiDEV , it would be great to have the feature. I`m still missing this feature so much |
No description provided.