OPDS Plugin: Ensure the default download filename is consistent across different platforms.#13709
Conversation
…latforms. The OPDS plugin correctly uses util.getSafeFilename() to remove any special characters from the filename for downloads. However this function works differently on different platforms, with some common characters like : and ? being allowed on some platforms but not others. In particular this may break Process Sync if it is configured to rely on file names. This change gives a better default behaviour by ensuring the default filename is the same across different platforms, while still letting the user change the filename to include special characters if the platform supports them.
| end | ||
| -- Ensure the default download filename is consistent across different platforms. In particular | ||
| -- this is needed for Progress Sync to work correctly if it is configured to rely on file names. | ||
| filename = filename:gsub('[\\,%/,:,%*,%?,%",%<,%>,%|]','_') |
There was a problem hiding this comment.
Given the explanation in the PR I was expecting something more along the lines of exporting the replaceAllInvalidChars function and updating wherever util.getSafeFilename() is used to use that instead. Because now you're first doing this and still doing util.getSafeFilename() again unnecessarily later. Or just to add an extra boolean to util.getSafeFilename() for example.
Lines 949 to 960 in acbde33
But that aside, I suppose this same concern would apply just about anywhere the function is used, wouldn't it?
|
My first thought was to just have util.getSafeFilename() always call replaceAllInvalidChars() regardless of the platform or filesystem. That would be simplest and most consistent. I didn't go with that because it would mean characters like : and ? can never be used in filenames even on platforms were they don't cause a problem. It's not unusual to have those characters in book titles so not being able to use them at all in file names could annoy people. The line I added is prior to the Save File dialog box and ensures the default filename is consistent. The later call toutil.getSafeFilename() happens after the user has possibly changed the filename, so isn't strictly unnecessary. I don't think the inconsistency is a concern when the user is actually choosing a filename. It's really just where KOReader chooses a default filename - certainly here in the OPDS plugin, possibly in Cloud Storage and Wallabag but I haven't looked at those. There may be other places too. Thinking about this more it may make most sense to go back to my first idea and just make the filename rules the same across all platforms. I'm open to suggestions on that. |
That's why it is the way it is, yes. I'd rather keep it that way, but I don't think I'd considered this scenario. For the moment can you update it to use |
|
Good suggestion. I've just pushed that change. |
Frenzie
left a comment
There was a problem hiding this comment.
Fine by me, but I'm waiting to merge for a bit to allow for more comments (if any).
|
@spfenwick Sorry, I forgot to merge. Would you mind double-checking it's correctly reconciled with #13946? |
…s different platforms (koreader#13709) * Ensure the default download filename is consistent across different platforms. The OPDS plugin correctly uses util.getSafeFilename() to remove any special characters from the filename for downloads. However this function works differently on different platforms, with some common characters like : and ? being allowed on some platforms but not others. In particular this may break Process Sync if it is configured to rely on file names. This change gives a better default behaviour by ensuring the default filename is the same across different platforms, while still letting the user change the filename to include special characters if the platform supports them. * Make util.replaceAllInvalidChars() global and call it rather than copying code.
The OPDS plugin uses util.getSafeFilename() to remove any special characters from the filename for downloads. Tthis function works differently on different platforms, with some characters that are common in book titles (like : and ?) being allowed on some platforms but replaced with _ on others. In particular this breaks Progress Sync if it is configured to rely on filenames and the book title contains any of those special characters.
If the user knows what the problem is it's easy enough to work around by changing the filename, but the default behaviour of silently failing to sync is not a good user experience.
This change gives a better default behaviour by ensuring the default filename when the file is downloaded via OPDS is the same across different platforms, while still letting the user change the filename to include special characters if the platform supports them.
This change is