-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix drop image rotate wrong #2322
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,82 @@ const STORAGE_FOLDER_PLACEHOLDER = ':storage' | |
| const DESTINATION_FOLDER = 'attachments' | ||
| const PATH_SEPARATORS = escapeStringRegexp(path.posix.sep) + escapeStringRegexp(path.win32.sep) | ||
|
|
||
| function getImage (file) { | ||
| return new Promise((resolve) => { | ||
| const reader = new FileReader() | ||
| const img = new Image() | ||
| img.onload = () => resolve(img) | ||
| reader.onload = e => { | ||
| img.src = e.target.result | ||
| } | ||
| reader.readAsDataURL(file) | ||
| }) | ||
| } | ||
|
|
||
| function getOrientation (file) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me it seems like this method does a lot of black magic.. what are all the constants about? I have no idea how this method works, what it does and what it returns..
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add comments later. |
||
| const getData = arrayBuffer => { | ||
| const view = new DataView(arrayBuffer) | ||
| if (view.getUint16(0, false) !== 0xFFD8) return -2 | ||
| const length = view.byteLength | ||
| let offset = 2 | ||
| while (offset < length) { | ||
| const marker = view.getUint16(offset, false) | ||
| offset += 2 | ||
| if (marker === 0xFFE1) { | ||
| if (view.getUint32(offset += 2, false) !== 0x45786966) { | ||
| return -1 | ||
| } | ||
| const little = view.getUint16(offset += 6, false) === 0x4949 | ||
| offset += view.getUint32(offset + 4, little) | ||
| const tags = view.getUint16(offset, little) | ||
| offset += 2 | ||
| for (let i = 0; i < tags; i++) { | ||
| if (view.getUint16(offset + (i * 12), little) === 0x0112) { | ||
| return view.getUint16(offset + (i * 12) + 8, little) | ||
| } | ||
| } | ||
| } else if ((marker & 0xFF00) !== 0xFF00) { | ||
| break | ||
| } else { | ||
| offset += view.getUint16(offset, false) | ||
| } | ||
| } | ||
| return -1 | ||
| } | ||
| return new Promise((resolve) => { | ||
| const reader = new FileReader() | ||
| reader.onload = event => resolve(getData(event.target.result)) | ||
| reader.readAsArrayBuffer(file.slice(0, 64 * 1024)) | ||
| }) | ||
| } | ||
|
|
||
| function fixRotate (file) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does it "fixes" the "wrong" rotation? I don't understand what might be wrong with it since i cannot reproduce the problem..
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just like this, https://stackoverflow.com/questions/10600613/ios-image-orientation-has-strange-behavior |
||
| return Promise.all([getImage(file), getOrientation(file)]) | ||
| .then(([img, orientation]) => { | ||
| const canvas = document.createElement('canvas') | ||
| const ctx = canvas.getContext('2d') | ||
| if (orientation > 4 && orientation < 9) { | ||
| canvas.width = img.height | ||
| canvas.height = img.width | ||
| } else { | ||
| canvas.width = img.width | ||
| canvas.height = img.height | ||
| } | ||
| switch (orientation) { | ||
| case 2: ctx.transform(-1, 0, 0, 1, img.width, 0); break | ||
| case 3: ctx.transform(-1, 0, 0, -1, img.width, img.height); break | ||
| case 4: ctx.transform(1, 0, 0, -1, 0, img.height); break | ||
| case 5: ctx.transform(0, 1, 1, 0, 0, 0); break | ||
| case 6: ctx.transform(0, 1, -1, 0, img.height, 0); break | ||
| case 7: ctx.transform(0, -1, -1, 0, img.height, img.width); break | ||
| case 8: ctx.transform(0, -1, 1, 0, 0, img.width); break | ||
| default: break | ||
| } | ||
| ctx.drawImage(img, 0, 0) | ||
| return canvas.toDataURL() | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * @description | ||
| * Copies a copy of an attachment to the storage folder specified by the given key and return the generated attachment name. | ||
|
|
@@ -38,26 +114,34 @@ function copyAttachment (sourceFilePath, storageKey, noteKey, useRandomName = tr | |
| } | ||
|
|
||
| try { | ||
| if (!fs.existsSync(sourceFilePath)) { | ||
| reject('source file does not exist') | ||
| const isBase64 = typeof sourceFilePath === 'object' && sourceFilePath.type === 'base64' | ||
| if (!fs.existsSync(sourceFilePath) && !isBase64) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you reject images that are not base64? e.g. i might drag&drop a pdf. Are all files always base64?!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only if the file not exists and the file is not a base64, reject.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, but i mean what happens if you drop a PDF for example. Is that still possible with your changes? Or will it be rejected because it is not bas64?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This patch does nothing for other file. It will work same as before
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. And why don't you use something like this library instead of implementing it yourself? (tbh: first google result) https://www.npmjs.com/package/fix-orientation ??
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has many dependencies, maybe we don't need to import so many code.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @ehhc. This feature looks good to have. But I think it should not be included in the code base of boostnote. It is almost impossible to maintain...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we make a npm module for this? And, I think we need some test for this too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll do it some days later. |
||
| return reject('source file does not exist') | ||
| } | ||
|
|
||
| const targetStorage = findStorage.findStorage(storageKey) | ||
|
|
||
| const inputFileStream = fs.createReadStream(sourceFilePath) | ||
| let destinationName | ||
| if (useRandomName) { | ||
| destinationName = `${uniqueSlug()}${path.extname(sourceFilePath)}` | ||
| destinationName = `${uniqueSlug()}${path.extname(sourceFilePath.sourceFilePath || sourceFilePath)}` | ||
| } else { | ||
| destinationName = path.basename(sourceFilePath) | ||
| destinationName = path.basename(sourceFilePath.sourceFilePath || sourceFilePath) | ||
| } | ||
| const destinationDir = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey) | ||
| createAttachmentDestinationFolder(targetStorage.path, noteKey) | ||
| const outputFile = fs.createWriteStream(path.join(destinationDir, destinationName)) | ||
| inputFileStream.pipe(outputFile) | ||
| inputFileStream.on('end', () => { | ||
| resolve(destinationName) | ||
| }) | ||
|
|
||
| if (isBase64) { | ||
| const base64Data = sourceFilePath.data.replace(/^data:image\/\w+;base64,/, '') | ||
| const dataBuffer = new Buffer(base64Data, 'base64') | ||
| outputFile.write(dataBuffer, () => { | ||
| resolve(destinationName) | ||
| }) | ||
| } else { | ||
| const inputFileStream = fs.createReadStream(sourceFilePath) | ||
| inputFileStream.pipe(outputFile) | ||
| inputFileStream.on('end', () => { | ||
| resolve(destinationName) | ||
| }) | ||
| } | ||
| } catch (e) { | ||
| return reject(e) | ||
| } | ||
|
|
@@ -137,10 +221,17 @@ function handleAttachmentDrop (codeEditor, storageKey, noteKey, dropEvent) { | |
| const filePath = file.path | ||
| const originalFileName = path.basename(filePath) | ||
| const fileType = file['type'] | ||
|
|
||
| copyAttachment(filePath, storageKey, noteKey).then((fileName) => { | ||
| const showPreview = fileType.startsWith('image') | ||
| const imageMd = generateAttachmentMarkdown(originalFileName, path.join(STORAGE_FOLDER_PLACEHOLDER, noteKey, fileName), showPreview) | ||
| const isImage = fileType.startsWith('image') | ||
| let promise | ||
| if (isImage) { | ||
| promise = fixRotate(file).then(base64data => { | ||
| return copyAttachment({type: 'base64', data: base64data, sourceFilePath: filePath}, storageKey, noteKey) | ||
| }) | ||
| } else { | ||
| promise = copyAttachment(filePath, storageKey, noteKey) | ||
| } | ||
| promise.then((fileName) => { | ||
| const imageMd = generateAttachmentMarkdown(originalFileName, path.join(STORAGE_FOLDER_PLACEHOLDER, noteKey, fileName), isImage) | ||
| codeEditor.insertAttachmentMd(imageMd) | ||
| }) | ||
| } | ||
|
|
||


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.
There is no documentation at all.
Nor are there any tests i see..
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.
Ok, I will add it later