Desktop: Add default filename for jex export #3034
Conversation
|
I'm not certain we should add this because it's often tricky to pick good default filenames for files. Also one feature of operating systems is that Save dialog will give you the last name you used - I believe that will work if, like now, we pass an empty string. If we hard-code it to something else though, we'll override every time the user's choice. Also passing a date could be an issue as it can contain reserved characters like "/". So I'm not sure, it could be a good change but it could have an unexpected impact on certain features. |
The date format should be set by the export function itself so there should never be any illegal characters. If the filename "mask" is "YYYYMMDD-HHMMSS-Joplin.jex" then any date will have that format (i.e. |
|
That kind of make sense for JEX files @dpoulton-github, but isn't the default filename applied to every export formats? Maybe @CalebJohn can confirm as he's more familiar with this part of the code than me. For example, if I export a note called "Shopping list" to PDF, I think it's better if it's called "Shopping List.pdf" rather than some ISO date format. Or perhaps the issue is that different export formats should have different default filenames? |
|
This applies to any export that wouldn't already have a default filename. So shopping list.pdf will still be created as normal, but any jex export will have this default name. |
|
Ok that makes sense, so let's do that then. There's an issue with the branch though - probably due to the recent merge: https://travis-ci.org/github/laurent22/joplin/jobs/686710047#L1096 Also you could add a test for that particular feature please? There's already a file tests/services_InteropService.js so I guess you could add the test there. |
|
@laurent22 from what I can tell all tests are testing modules from lib. But the InteropServiceHelper is in ElectronClient. Is there a good way to test this code? Or should I instead move this function into the InteropService file? |
There was a problem hiding this comment.
@CalebJohn, I don't think the pull request as it is would work as you've changed the function signature but didn't update the calls. However, the signature shouldn't change so if you could revert it that would be great. Let's skip testing as it's a bit complicated, for little gains.
| static async defaultFilename(noteId, fileExtension) { | ||
| if (!noteId) return ''; | ||
| const note = await Note.load(noteId); | ||
| static async defaultFilename(noteIds, fileExtension) { |
There was a problem hiding this comment.
Function signature shouldn't change (it accepts a note ID, not an array)
There was a problem hiding this comment.
Thanks for the catch, I guess thats something that has changed since I first made the pull request. I've fixed it now.
| static async defaultFilename(noteIds, fileExtension) { | ||
| if (!noteIds) { | ||
| const date = time.formatMsToLocal(new Date().getTime(), time.dateFormat()); | ||
| return friendlySafeFilename(`${date} - ${_('Joplin')}.${fileExtension}`); |
There was a problem hiding this comment.
I think just ${date}.${fileExtension} would be enough as the "Joplin" part is not relevant once the file has been exported (it's just a Markdown or PDF file like any other).
There was a problem hiding this comment.
Yes good point, I've updated it now.
|
Looks good, thanks @CalebJohn! |
Based on @dpoulton-github 's suggestion on on #3026