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
Fix .doc export with LibreOffice (soffice) #3338
Conversation
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.
Wow, great catch, @ldidry !! Great work!! 👍
I've added some minor suggestions, let me now what you think.
src/node/utils/LibreOffice.js
Outdated
if (type === 'doc') { | ||
queue.push({ | ||
"srcFile": srcFile, | ||
"destFile": destFile.replace(/\.doc/, '.odt'), |
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 don't know if srcFile
could be something like "test.html.html"
, but in this case this replace
would not work as you're expecting. You can use the $
regex mark of EOL to fix that:
"test.html.html".replace(/\.html/, '.odt')
> "test.odt.html"
"test.html.html".replace(/\.html$/, '.odt')
> "test.html.odt"
src/node/utils/LibreOffice.js
Outdated
"destFile": destFile.replace(/\.doc/, '.odt'), | ||
"type": 'odt', | ||
"callback": function () { | ||
queue.push({"srcFile": srcFile.replace(/\.html/, '.odt'), "destFile": destFile, "type": type, "callback": callback}); |
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.
same here
src/node/utils/LibreOffice.js
Outdated
@@ -35,7 +35,18 @@ var queue = async.queue(doConvertTask, 1); | |||
* @param {Function} callback Standard callback function | |||
*/ | |||
exports.convertFile = function(srcFile, destFile, type, callback) { | |||
queue.push({"srcFile": srcFile, "destFile": destFile, "type": type, "callback": callback}); | |||
if (type === 'doc') { |
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.
as this is a very specific case, it would be good to have some comments explaining the reason we have this if/else
. Would you mind adding it here, @ldidry ? You can add a reference to this PR, it would make it easier for other devs to know why this code needed to be added.
When using LibreOffice to convert pads to doc, we got `Error: no export filter for /tmp/xxxx.doc` (tested with LO 5 and 6). Maybe it's a regression from LO. Anyway, converting HTML to odt, then to doc works. Thx to lpagliari for her review!
Thank you for your suggestions, they're indeed very good 🙂 |
Thanks, @ldidry ! |
When using LibreOffice to convert pads to doc, we got
Error: no export filter for /tmp/xxxx.doc
(tested with LO 5 and 6). Maybe it's a regression from LO. Anyway, converting HTML to odt, then to doc works.