Skip to content
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

Plugin for saving the cover image to file #6813

Merged
merged 28 commits into from
Nov 10, 2020
Merged

Plugin for saving the cover image to file #6813

merged 28 commits into from
Nov 10, 2020

Conversation

zwim
Copy link
Contributor

@zwim zwim commented Oct 22, 2020

If the plugin is enabled the following actions can be selected:

  1. On opening of a document, the cover is saved to a file (format png).
    If the filename exists, a backup of the original is created.

  2. On closing of the document this file can be deleted and the original file can be restored.

  3. Certain documents can be excluded from image generation.

This plugin can be used to change the sleep-screen on Tolinos (filename: /sdcard/suspend_others.jpg) as long KOReader has an opened document. On close of KOReader the original sleep screen is restored.


This change is Reviewable

Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the principle (and quite a few people were requested that :)
Dunno if we should disable this plugin on some platforms like Kobo or Kindle where it would not make any sense as there are no other tool that could use such generated image?

(I think I'd also wouldn't mind if this was not a plugin but part of the core Screensaver code and menu.)

plugins/coverimage.koplugin/main.lua Outdated Show resolved Hide resolved
end
end

function CoverImage:onDocSettingsLoad(doc_settings, document)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it's the best event/timing (document is not yet fully opened I think), best to catch the ReaderReady event with onReaderReady):

-- Allow others to change settings based on external factors
-- Must be called after plugins are loaded & before setting are read.
self:handleEvent(Event:new("DocSettingsLoad", self.doc_settings, self.document))
-- we only read settings after all the widgets are initialized
self:handleEvent(Event:new("ReadSettings", self.doc_settings))
for _,v in ipairs(self.postInitCallback) do
v()
end
self.postInitCallback = nil
-- Now that document is loaded, store book metadata in settings
-- (so that filemanager can use it from sideCar file to display
-- Book information).
self.doc_settings:saveSetting("doc_props", self.document:getProps())
-- After initialisation notify that document is loaded and rendered
-- CREngine only reports correct page count after rendering is done
-- Need the same event for PDF document
self:handleEvent(Event:new("ReaderReady", self.doc_settings))

document is not passed and you need it, so either add it being passed above, but it's also immediately available to all plugins as self.ui.document

You might also need some checks like in:

function ReaderStatistics:isDocless()
    return self.ui == nil or self.ui.document == nil
end

so it does not fail and do nothing when in FileManager ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In onReaderReady I have added:

function CoverImage:onReaderReady(doc_settings)
   logger.dbg("CoverImage: onDocSettingsLoad")
   if self.enabled and self.ui and self.ui.document then 
        local lfs = require("libs/libkoreader-lfs")

The plugin is not shown in FileManager, because I have set is_doc_only = true,

plugins/coverimage.koplugin/main.lua Outdated Show resolved Hide resolved

function CoverImage:addToMainMenu(menu_items)
menu_items.coverimage = {
-- sorting_hint = "device", -- maybe this plugin is better situated in device?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or in the Screensaver menu ? (Dunno if that's possible)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not find the screensaver menu on android (Tolino).
But I could put it in the Gear/Document menu.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no screensaver menu on android.

plugins/coverimage.koplugin/main.lua Outdated Show resolved Hide resolved
plugins/coverimage.koplugin/main.lua Outdated Show resolved Hide resolved
plugins/coverimage.koplugin/main.lua Outdated Show resolved Hide resolved
plugins/coverimage.koplugin/main.lua Outdated Show resolved Hide resolved
plugins/coverimage.koplugin/main.lua Show resolved Hide resolved
@elvvis
Copy link

elvvis commented Oct 22, 2020

The plugin will not work on the Tolino. You can select a custom image (in png format) as your screensaver, but if the content of this image is changed later, this will not affect the screensaver (the screensaver image will be converted to jpg format after the png image is selected).
The correct way is to save the book cover as jpg (for Tolino Epos 2 in the directory /data/data/de.telekom.epub/files/lastread.jpg), but for this the device must be rooted.

@zwim
Copy link
Contributor Author

zwim commented Oct 22, 2020

@elvvis: Are you sure? Have you tried it? I am using this plugin for two weeks now and it works on my Epos2.

The plugin will not work on the Tolino. You can select a custom image (in png format) as your screensaver, but if the content of this image is changed later, this will not affect the screensaver (the screensaver image will be converted to jpg format after the png image is selected).

You have to store the image under /sdcard/suspend_others.jpg (use jpg even if the image is png).

The correct way is to save the book cover as jpg (for Tolino Epos 2 in the directory /data/data/de.telekom.epub/files/lastread.jpg), but for this the device must be rooted.

You don't have to root your device for the screensaver!
With this plugin I have my "normal" screensaver image in /sdcard/suspend_others.jpg. If I open a document in KOReader the original image is renamed to /sdcard/suspend_others.jpg.bak and the actual cover is stored as /sdcard/suspend_others.jpg. So I have the book-cover as screensaver. When I end KOReader, the old image is deleted and the backup gets renamed to /sdcard/suspend_others.jpg.

@elvvis
Copy link

elvvis commented Oct 22, 2020

@elvvis: Are you sure? Have you tried it? I am using this plugin for two weeks now and it works on my Epos2.

I didn't know the trick to save the png-image as jpg, but it works great this way.
Fantastic that you wrote this plugin. 👍

if not lfs.attributes(self.cover_image_path ..".bak") then
os.rename(self.cover_image_path, self.cover_image_path .. ".bak" )
end
Device.screen.bb.writePNG(image, self.cover_image_path, false)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "Device.screen.bb.writePNG(image, self.cover_image_path, false)" you can use "image:writePNG(self.cover_image_path, false)", so writePNG does not need to be changed in base/ffi/blitbuffer.lua.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elvvis: As you can see this PR just adds two files. For this PR nothing else has to be changed. So I don't understand your comment about base/ffi/blitbuffer.lua

Copy link
Member

@NiLuJe NiLuJe Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCoverPageImage() returns a BB, writePNG is a BB method.

You can just do image:writePNG(self.cover_image_path) ;).

(i.e., Pulling the function out of the Device.screen.bb namespace is extremely roundabout)

Copy link
Contributor Author

@zwim zwim Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NiLuJe: Ahh, now I see :-)
It works

@poire-z
Copy link
Contributor

poire-z commented Oct 22, 2020

For the normal non-Android screensaver stuff, we have another option, Exclude this book's cover from screensaver - so when you don't like a cover for a book, you can ignore it and fallback to the other options.
You might want to duplicate it in your plugin (I actually use it quite often), and don't save the cover image on opening when it is set - or restore/save when toggling it.

if Device:supportsScreensaver() then
local ss_book_settings = {
text = _("Exclude this book's cover from screensaver"),
enabled_func = function()
return not (self.ui == nil or self.ui.document == nil)
and G_reader_settings:readSetting('screensaver_type') == "cover"
end,
checked_func = function()
return self.ui and self.ui.doc_settings and self.ui.doc_settings:readSetting("exclude_screensaver") == true
end,
callback = function()
if Screensaver:excluded() then
self.ui.doc_settings:saveSetting("exclude_screensaver", false)
else
self.ui.doc_settings:saveSetting("exclude_screensaver", true)
end
self.ui:saveSettings()
end,
added_by_readermenu_flag = true,
}

@zwim
Copy link
Contributor Author

zwim commented Oct 23, 2020

@poire-z: Good point, now I can suppress embarrassing images on screen :-)

Comment on lines 29 to 34
function CoverImage:excluded()
local lastfile = G_reader_settings:readSetting("lastfile")
local exclude_ss = false -- consider it not excluded if there's no docsetting
if DocSettings:hasSidecarFile(lastfile) then
local doc_settings = DocSettings:open(lastfile)
exclude_ss = doc_settings:readSetting("exclude_cover_image")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is re-opening/parsing the metatada.zzz.lua, and might be costly. In most (or all, not sure about onCloseDocument) your contexts, you already have self.ui.odc_settings or doc_settings provided, so you can just readSetting("exclude_cover_image") on it - without having to re-open/parse it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line 34:
exclude_ss = self.ui.doc_settings:readSetting("exclude_cover_image") brings ./luajit: plugins/coverimage.koplugin/main.lua:35: attempt to index field 'ui' (a nil value)

and
exclude_ss = DocSettings:readSetting("exclude_cover_image")
brings up ./luajit: frontend/docsettings.lua:170: attempt to index field 'data' (a nil value)

So i have combined the old method with self.ui.doc_settings

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the 2nd one:
exclude_ss = DocSettings:readSetting("exclude_cover_image")
DocSettings is a module/class, not an instance, so it has no data, nothing to readerSetting from :)

Comment on lines 29 to 33
function CoverImage:excluded()
local lastfile = G_reader_settings:readSetting("lastfile")
local exclude_ss = false -- consider it not excluded if there's no docsetting
if DocSettings:hasSidecarFile(lastfile) then
if self and self.ui then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to check if self (it always exists), I guess you meant if self.ui and self.ui.doc_settings then.

But I still don't like all that :)
It feels none of this is needed, and you don't need to care about lastfile:

  • in CoverImage:onCloseDocument(), you don't really need to know if excluded: if restore and there's a .bak: restore it.
  • in CoverImage:onReaderReady(doc_settings), you have the doc_settings, so just read it

If there's some edge case because you call these 2 methods when toggling stuff in the menu, may be you should have 2 other methods: CoverImage:backupPreviousAndCreateNewOne() and CoverImage:restorePreviousIfAny() (with better names :) to do the low level check and action - and onCloseDocument/OnReaderReady/your menu callbacks would do the check they can from what they have - and call the 2 lowlevel methods if stuff should be done.

Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks simpler and sane :)

Comment on lines 37 to 75
local has_bak = lfs.attributes(self.cover_image_path .. ".bak")
-- Delete image if backup exists, the backup could contain a user defined sleep image
if lfs.attributes(self.cover_image_path) and has_bak then
os.remove(self.cover_image_path)
end
-- Restore backup if it exists, so we can use the last image.
-- On Tolino a user defined /sdcard/suspend_others.jgp can be used as screensaver on system sleep.
if has_bak then
os.rename(self.cover_image_path .. ".bak", self.cover_image_path)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like all this can be wrap in a single if has_bak then.
(And may be have self.cover_image_path .. ".bak" in a local variable to avoid doing the concat twice.

About the last comment:

On Tolino a user defined /sdcard/suspend_others.jgp can be used as screensaver on system sleep

You mean one could chose to not have a main image (the one that becomes .bak)? In this case, shouldn't you also os.remove(self.cover_image_path) when has_bak is false ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, the has_bak on the remove is wrong.

callback = function()
self.enabled = not self.enabled
G_reader_settings:saveSetting("cover_image_enabled", self.enabled)
G_reader_settings:flush()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you feel the need to add this :flush() ?
It womm flush to disk settings.reader.lua - which shouldn't be needed to be done explicitely. :saveSettings() updates the table in memory, and it will always be flushed with the other stuff when needed (regularly, on switching documents, and always on KOReader exit).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your hint. I have used watch -n 0.5 grep cover_image settings.reader.lua to watch changes. With this I noticed, that no changes in settings.reader.lua showed up. On the next start of KOReader the old values where still there. Maybe this was caused by kill of KOReader-emulator (ctrl-c).
Now I have checked this all over again, and could NOT confirm this strange behavior.

You are right, the flush is not necessary.

Copy link
Member

@NiLuJe NiLuJe Oct 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a SIGQUIT SIGINT, so, yeah, a ^C will murder the state, don't do that ;).

We only save the state on graceful exits.

Comment on lines 38 to 65
if lfs.attributes(self.cover_image_path) then
os.remove(self.cover_image_path)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is always called by onCloseDocument() (if restore=true), if there is no coverimage, this might delete the default image (which would not have been renamed to .bak) ?

Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must have thought more than I did about all the logic and edge cases - but reading it from a distance, the logic feels a bit fuzzy and unclear. It feels there could be a cleaner and simpler logic to managing all that.

local CoverImage = WidgetContainer:new{
name = 'coverimage',
is_doc_only = true,
settings_id = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused (probably a left over of the copied base plugin)


function CoverImage:cleanUpImage()
if not self.ui.doc_settings:readSetting("exclude_cover_image") == true then
local lfs = require("libs/libkoreader-lfs")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this lfs at top with the other requires (it's always loaded, so costs nothing even if not used).
(The other one below can then be removed.)

Comment on lines 63 to 75
local image = self.ui.document:getCoverPageImage()
if image then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still an issue in onCloseDocument/cleanUpImage() if there were no image here: the one that is os.remove(self.cover_image_path) is actually the main fallback image.

self.enabled = not self.enabled
G_reader_settings:saveSetting("cover_image_enabled", self.enabled)
if self.enabled then
self.restore = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't one toggle "enabled" while still wanting "restore" to stay false?
I assume the combination of enabled=true and restore=false is when one wants to keep the last book cover as the screensaver when exiting KOReader, or exiting it while in Filebrowser ?

@zwim
Copy link
Contributor Author

zwim commented Oct 25, 2020

You are right, the logic is a bit complicated.
I will let it sit for a while, now.

@rola25
Copy link

rola25 commented Oct 25, 2020

Does this also work on Tolino firmware 13.2.1?

@zwim
Copy link
Contributor Author

zwim commented Oct 25, 2020

@rola25: Actually I have 14.0.0, so I can not test it on 13.2.1. But the trick with putting a screensaver image in /sdcard/suspend_others.jpg also works on 13.2.1.
So I think, the plugin should work.

On old firmware versions (IIRC 10.0.x) the filename in /sdcard/suspend.jpg (IIRC) was different.

If you try it, could you give me a feedback please?

@rola25
Copy link

rola25 commented Oct 25, 2020

I can try it. Should I just copy the coverimage.koplugin Folder to /data/data/org.kor..... or do I need anything else? Which Screensaver do I have to choose in the tolinio-settings? Just the tolino-face screensaver?

@zwim
Copy link
Contributor Author

zwim commented Oct 25, 2020

You just have to copy folder coverimage.koplugin in /data/data/.... You find the plugin settings, if you open Menu-Gear-Dokument, when you have a document opened. You won't see it in the filemanager.
You don't have to mess with the Tolino software.

@pazos
Copy link
Member

pazos commented Oct 25, 2020

On old firmware versions (IIRC 10.0.x) the filename in /sdcard/suspend.jpg (IIRC) was different.

Not sure about the file, but the path is highly specific. For example, in @rola25 logcat

I/KOReader( 5021): Application info {
I/KOReader( 5021):   VM heap: 128MB (normal)
I/KOReader( 5021):   Flags: user.
I/KOReader( 5021):   Paths {
I/KOReader( 5021):     Assets: /data/data/org.koreader.launcher/files
I/KOReader( 5021):     Library: /data/app-lib/org.koreader.launcher-2
I/KOReader( 5021):     Storage: /storage/sdcard1
I/KOReader( 5021):   }
I/KOReader( 5021): }

The proper way of handling that is to use android.getExternalStoragePath(), which will always return a valid path to primary storage. (/storage/sdcard1 in this case). Usually /sdcard is a symlink to that folder, but that's another topic.

@zwim
Copy link
Contributor Author

zwim commented Oct 25, 2020

@pazos: I let the user enter a filename for the image. So the user is responsible to enter a valid path. If the plugin is (ab)used to change the screensaver image, this has to be entered only once (and it is device-specific).

Maybe later, if someone requests, we could improve the dialog for the filename with a path-selector and then we would use `android.getExternalStoragePath()', or we use the filemanager to select the image ...

@pazos
Copy link
Member

pazos commented Oct 25, 2020

Yeah, I know and I'm fine with your proposal 👍

Maybe later, if someone requests, we could improve the dialog for the filename with a path-selector and then we would use `android.getExternalStoragePath()', or we use the filemanager to select the image ...

You can do a runtime check with Device:isValidPath(path) when the user enters the file path. On the emulator it will return always true. On android it will return true if the path starts with android.getExternalStoragePath()

The filechooser thingy is a bit overkill IMHO.

@rola25
Copy link

rola25 commented Oct 25, 2020

I tried it, but it didn't work. It creates the file on the right place but it didn't use it, so I made a logcat and it said:
I/PowerManagerService( 2477): Going to sleep by user request...
D/FrontLight( 2477): Light====>SYS. brightness orig 0 conv 0
I/Terry-FB( 2477): strSuspendImgFileName = /storage/sdcard1/suspend.jpg
I/Terry-FB( 2477): strEPubFolder + "/" + strDefaultImgFileName = /data/data/de.telekom.epub/files//suspend.jpg
I/Terry-FB( 2477): strLocaleImgFolder + "/" + strDefaultImgFileName = /system/usr/sleep/drawable-de-nodpi/suspend.jpg
I/Terry-FB( 2477): strEPubFolder + "/" + strChargeImgFileName = /data/data/de.telekom.epub/files//suspend_charging.jpg
I/Terry-FB( 2477): strLocaleImgFolder + "/" + strChargeImgFileName = /system/usr/sleep/drawable-de-nodpi/suspend_charging.jpg
I/Terry-FB( 2477): strLocaleImgFolder + "/" + strFullImgFileName = /system/usr/sleep/drawable-de-nodpi/suspend_batteryfull.jpg
I/DANIEL-FB( 2477): strCurrentImgFileName =============== suspend.jpg
I/FrameBufferEx( 2477): Disp File: /data/data/de.telekom.epub/files/suspend.jpg

So I tried to set it to /sdcard/suspend.jpg, but it also doesn't work ... I just see the tolino face

@rola25
Copy link

rola25 commented Oct 25, 2020

Perhaps because it has the wrong size?

@pazos
Copy link
Member

pazos commented Oct 25, 2020

I/Terry-FB( 2477): strSuspendImgFileName = /storage/sdcard1/suspend.jpg

Try that. If that doesn't work it could be that it needs a valid jpg and not a png (or the size, I dunno)

@zwim
Copy link
Contributor Author

zwim commented Oct 25, 2020

The right name of the file is /sdcard/suspend_others.jpg.
And the Tolino must not be connected via usb.
It can be a png named as jpg, size does not matter.

@rola25
Copy link

rola25 commented Oct 25, 2020

Aaah, it works now, I was connected via ADB+USB, that was the problem. I use /sdcard/suspend.jpg as Cover Image and that works when disconnected from USB

@zwim
Copy link
Contributor Author

zwim commented Oct 25, 2020

@rola25: Good to hear :-)
On my Tolino /sdard/suspend.jpg does not work and on yours it does. Could you please check if suspend_others.jpg works on your device, too?

@rola25
Copy link

rola25 commented Oct 25, 2020

I just tried it, and it doesn't work with suspend_others.jpg. I have a Toline Epos 1 with Firmware 13.2.1

@zwim
Copy link
Contributor Author

zwim commented Oct 25, 2020

@pazos: I have a logical much easier implementation. The only "drawback" is, that the user has to enter the file name of his default screensaver image, which is chosen, when the Cover Image is deleted onDocumentClose.

The UI would look like this
screenshot

@NiLuJe
Copy link
Member

NiLuJe commented Nov 9, 2020

It hasn't been merged yet, so, no?

@sndwichm4n
Copy link

Ok, I am no programmer. When is the merging?

@sndwichm4n
Copy link

I ask because pazos has approved and the plugin tool must be awesome. :)

@zwim
Copy link
Contributor Author

zwim commented Nov 9, 2020

I have added a short description of the usage of this plugin on
https://github.com/koreader/koreader/wiki/Android-tips-and-tricks

Copy link
Member

@Galunid Galunid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@pazos Is there any way to broadcast android Intent from lua? I took a look at Onyx SDK set cover and it doesn't seem too hard to add this, except for broadcasting the intent. The part of interest in the demo.

@pazos
Copy link
Member

pazos commented Nov 9, 2020

I have added a short description of the usage of this plugin on
https://github.com/koreader/koreader/wiki/Android-tips-and-tricks

Thanks for the wiki stuff @zwim. I think it is better to document it on a separate plugin page (The text and all the samples are ok) and leave Android-tips-and-tricks for platform code.

@pazos
Copy link
Member

pazos commented Nov 9, 2020

@pazos Is there any way to broadcast android Intent from lua? I took a look at Onyx SDK set cover and it doesn't seem too hard to add this, except for broadcasting the intent. The part of interest in the demo.

Yup. There are plenty of samples in the code: a dict lookup, open a link in the browser, start the EPD test activity ... all use startActivity. There's also a wrapper called startActivityIfSafe that queries the packageManager before calling startActivity, so it is only called if there's a package able to resolve the intent. It should be useful to prevent ActivityNotFound exceptions.

Still not sure about your intent with the intent :)

@pazos
Copy link
Member

pazos commented Nov 9, 2020

@Galunid: we don't use sendBroadcast anywhere on the code. After a quick read of https://developer.android.com/reference/android/content/Context#sendBroadcast(android.content.Intent) it looks like sendBroadcast(intent) is the same than startActivity(intent). The former is used to communicate with Broadcast receivers and the later is to communicate with activities.

@pazos pazos added the Android label Nov 10, 2020
@pazos pazos merged commit 926e7dd into koreader:master Nov 10, 2020
@zwim zwim deleted the coverimage branch November 10, 2020 14:19
@ptrm
Copy link
Contributor

ptrm commented Nov 11, 2020

Is this plugin checking for android or disabling itself because of some deps furthers down the code, or is there a chance it might work for Pocketbooks (they also can have an image specified for screensaver)?

@Frenzie
Copy link
Member

Frenzie commented Nov 11, 2020

Is this plugin checking for android

Yes.

is there a chance it might work for Pocketbooks

Well… also yes? ;-P Depending on what PB requires, only minimal adjustments may be needed.

@zwim
Copy link
Contributor Author

zwim commented Nov 11, 2020

Is this plugin checking for android or disabling itself because of some deps furthers down the code, or is there a chance it might work for Pocketbooks (they also can have an image specified for screensaver)?

All the plugin does, is to save the cover to a certain file (it must be in a place which is writeable for KOReader) and restore a fallback image file on close if wanted.

There is no Android mystery hidden in it.

As I don't have a pocketbook I can not test if it works there. But you can test it. Just disable lines 3-5 on plugins/coverimage.koplugin/main.lua and try it out.

I would be interested in the results.

@ptrm
Copy link
Contributor

ptrm commented Nov 11, 2020

The plugin itself works very well. However, the book cover location PB system feature requires the file to be in... BMP format 😁. Below are the cover file dumps before and after using the plugin. Png is not recognised and results in the generic "Touch HD 3" screen with features listed.
20201111_184512

On the other hand, trying to use a custom image as power down logo also ezports the image to tge system/logo dir as BMP, so without BMP support the pkugin seems unusable on PocketBooks 😁 On which I don't insist, but if it's a matter of a single small dependency, that would be great :)

That being said, Pocketbook has a cool feature with it's native reader to show last read page during boot, so you get to the most recent book quickly:
20201111_185140
This might be a handy feature on Android as a screensaver, too, with just a screenshot instead of a cover.

@NiLuJe
Copy link
Member

NiLuJe commented Nov 11, 2020

IIRC, we don't currently have anything that supports writing BMP files.

On the other hand, BMP is a pretty "dumb" format, c.f., stbiw, so someone motivated enough should be able to whip up something via FFI fairly quickly.

@Frenzie
Copy link
Member

Frenzie commented Nov 11, 2020

Maybe PB comes with a built-in utility like ImageMagick for such a conversion? Also, I don't know much about BMP files besides having used 'em in Microsoft Paintbrush/Paint when I was little. If they're mostly compatible with PNM for example we already have MuPDF.

@zwim
Copy link
Contributor Author

zwim commented Nov 11, 2020

Couldn't we use the example of lodepng for creating BMP files:
https://raw.githubusercontent.com/lvandeve/lodepng/master/examples/example_png2bmp.cpp

If no one else stands up, I could look into it more deeply tomorrow.

@zwim
Copy link
Contributor Author

zwim commented Nov 12, 2020

I have done a quick test with https://github.com/nothings/stb and compiled a command line tool (Android, Emulator) to convert between bmp, jpg and png. For jpg even a compression (quality) setting is possible.

It seems to be usable on my Tolino.

usage: image-convert infile outfile format quality infile and outfile can be the same.

As I am not familiar to integrate a new library or C-functions to KOReader, I will definitively stop work here.
I hope someone wants to continue from here.

I could integrate the new function in the cover-image plugin.

/*
 * simple programm to convert files between bmp, jpg, png
 * Autor: @zwim
 * thanks to https://github.com/nothings/stb 
 *
 * Date: 11/12/2020
 * 
*/

#include <stdio.h>
#include <stdlib.h>
#include <strings.h>
 
#define STB_IMAGE_IMPLEMENTATION
#include "stb_image.h"
#define STB_IMAGE_WRITE_IMPLEMENTATION
#include "stb_image_write.h"
 
 int main(int argc, char *argv[]) {
    int width, height, channels;
    unsigned char *img;

    if ( argc < 5 ) {
        printf("usage: image-convert infile outfile format quality\n format can be: bmp, jpg, png\nquality is only valid for jpg: 1-100\n");
        exit(1);
    }

    img = stbi_load(argv[1], &width, &height, &channels, 0);

    if(img == NULL) {
        printf("Error in loading the image\n");
        exit(1);
    }
    printf("Loaded image with a width of %dpx, a height of %dpx and %d channels\n", width, height, channels);

    if ( strcasecmp(argv[3], "PNG" ) == 0 )
        stbi_write_png(argv[2], width, height, channels, img, width * channels);
    else if ( strcasecmp( argv[3], "JPG" ) == 0 )
        stbi_write_jpg(argv[2], width, height, channels, img, atoi(argv[4]) );
    else if ( strcasecmp( argv[3], "BMP" ) == 0 )
        stbi_write_bmp(argv[2], width, height, channels, img);

    stbi_image_free(img);    
    return 0; 
}

@Frenzie
Copy link
Member

Frenzie commented Nov 12, 2020

Couldn't we use the example of lodepng for creating BMP files:

I didn't realize the format was that simple, but I suppose it doesn't have compression. In that case I'd suggest simply popping in https://github.com/luapower/bmp/blob/25733a99f077bde8215d7d7939c96ea2371659bc/bmp.lua

@zwim
Copy link
Contributor Author

zwim commented Nov 12, 2020

simple, but I suppose it doesn't have compression.

In my younger days I was forced quite often to use bmp. It was a really dump format and had no compression. Don't know, if that was added later.

But I would prefer the stb solution, as we could also save images in the jpg format, which would save quite a bit of flash.
You can see a short demonstration (Android, Linux) on https://github.com/zwim/image-converter.

@Frenzie
Copy link
Member

Frenzie commented Nov 12, 2020

But I would prefer the stb solution, as we could also save images in the jpg format, which would save quite a bit of flash.

Er, if PB takes jpeg then we don't need any extra anything. ;-)

checked_func = function()
return lfs.attributes(self.cover_image_fallback_path, "mode") == "file"
end,
help_text = _("File to use when no cover is wanted (found ???) or book is excluded.\nLeave this blank to turn off the fallback image."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zwim Leftover draft of some sort, will you submit a PR to fix it?

Copy link
Contributor Author

@zwim zwim Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thank you.

Yes, we can delete the help_text, as the dialog shows instructions.
But there is a minor problem showing a wrong info message, when fallback_image is an empty string.

I do a PR a bit later, today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants