[Enhancement request] pyqt signlas in calibre source

[capink]

Wonder if it is possible to add some new pyqt siganls to calibre. The siganls are in my-changes file attached to this bug report. I use the signals in plugin I have that enables the user to have his own custom last_modified column(s), since last_modified in calibre is reserved for internal use. The plugin can update one or more custom date column(s) whenever one or more of the following events happen:

- book metadata updated.
- cover modified.
- format added.
- format(s) removed.
- category item renamed.
- category item removed.
- book edited by calibre editor.

This allows the user more granularity in defining what a custom #last_modified column(s) means without disrupting the last_modified column of calibre.

N.B. There is also one extra enter_key_pressed signal in the attached file that I use with the Actions Chains plugin to define actions whenever the enter key is pressed. Please let me know if I should open a separate report for this.

[capink (capink-eg)]

I uploaded a couple of screenshots that demonstrates what the plugin is trying to achieve with these signals.

[capink (capink-eg)]

changes in my-changes attachment

[capink (capink-eg)]

deleted.

[Kovid Goyal (kovid)]

While this is fine general, db.cache cant import/depend on Qt. So you
would need a more generic mechanism, just plain python callback
functions. And there are many more places metadata is modified, for
instance the functions to rename tags/authors/etc.

[capink (capink-eg)]

Thanks for you reply. Regarding your point about functions to rename tags/authors/etc, the changes I posted did actually contain modifications in several places, but I am not sure whether they cover all the functionality:

- a modification to rename_items()
- a modification to remove_items()
- a modification to add_format()
- a modification to remove_formats()

I did not know about cache.db not supposed to import/depend on Qt. And I don't know how to do callbacks in a context of third party plugin. My initial thought was to add a method or a function to the plugin and have calibre call that instead of emitting signals, but this would depend on get_gui(). I am thinking if cache.db cannot import Qt, chances are it cannot import get_gui (or a functions that depends on it) as well.

Anyway, since my knowledge is not sufficient, I don't want to go around messing with calibre code.

[Charles Haley (cbhaley)]

@capink: you should be able to do this using the same method as marked listeners. See db.view.add_marked_listener() and the listener call loop in db.view.set_marked_ids().

My guess is that you would add something similar to db.cache. You could add a listener type for each signal, but I think it would be better to use a single listener method and pass a string telling the listener what 'event' has happened. The rest of the arguments passed to the listener would be conditioned by the event type.

Note that the weakref stuff is important because listeners (plugins) can come and go and we don't want random pointers preventing garbage collection. Also note that you will need to re-register the listeners whenever the library changes.

@kovid: if this is a bad idea let us know. :)

[Kovid Goyal (kovid)]

Basically, that's fine. You would also need to add the actual signal in
the GUI and registerde-register it when the library is changed. See
library_moved() in ui.py

[Charles Haley (cbhaley)]

Ahhh, you are suggesting that the GUI (ui.py) always register the callback, then emit a signal (or signals) when the callback is invoked. That is better than what I was thinking -- that the eventual clients register the callback -- because the mechanism seen by the clients (the plugins) is a bog-standard signal. The clients don't worry about library changes etc. because the GUI reregisters the callback.

This implementation is much closer to what @capink already did, with the signal class moved to ui.py.

[capink (capink-eg)]

@cbhaley Thanks for your feedback. I gave your idea a go (also went with Kovid's suggestion of a signal in ui.py) and so far it seems to be working.

The changes are attached here.

@Kovid I made the suggested changes but I am struggling with where to insert the new methods ... etc. Also, I added a line (in library_moved()) to re-register the listeners whenever the db changes. But do I have to also add a method to de-register? (remove_listeners), aren't the old references are garbage collected?

Also I don't really understand the FunctionDispatcher and I am not sure whether it is necessary here. I tried with and without it and works both ways.

Thanks again for both of you for you time and help.

[capink (capink-eg)]

Also here is the modified plugin if anyone is interested.

[Charles Haley (cbhaley)]

Three quick comments:
1) In cache.py you should wrap the call to a listener in a try/except block.
2) You should use FunctionDispatcher. It ensures that the call is made in the same thread in which the dispatcher was created. This becomes important if changes are made to the DB by a process in a different thread, possibly the command line and the web server.
3) I think you should force the signal connection to be QueuedConnection. The only way I know to do that is to provide your own signal connect method, something like connect_to_db_changed(some_function). The reason to use queued is to let the db finish whatever it is doing before the signal is emitted, for example releasing locks and committing transactions.

[capink (capink-eg)]

@cbhaley: Thanks. I will update to address all points. When you talk about using QueuedConnection you mean in the plugin code, right? why not just use something like self.gui.db_event.connect(self.process_event, type=Qt.QueuedConnection) instead of creating a new method?

[Charles Haley (cbhaley)]

Because I want to force a queued connection, not depend on the plugin writer to do it. I know you would do it but I don't have faith in others.

[capink (capink-eg)]

OK, I got it. Would calling the method calling QTimer.singleShot() have the same effect?:

QTimer.singleShot(0, partial(self.db_event.emit, event_name, event_args))

[Charles Haley (cbhaley)]

Yes. AFAIK that is what QueuedConnection does. The only problem I can imagine is event evaluation order, but QT would be crazy not to evaluate the events time-order FIFO.

[Kovid Goyal (kovid)]

Some comments:

1) Use weakref.WeakValueDictionary for the listeners

2) Yes, QTimer will work, but it is not the same as FunctionDispatcher.
FunctionDispatcher calls the function immediately if the thread is the
gui thread, QTimer will always call the function after an event loop
tick. In this case QTimer's semantics are preferable to
FunctionDispatcher, since that guarantees the lock is not held when the
function is called. But, for performance, rather than QTimer, it is
probably better to use QMetaObject.invokeMethod

However this discussion is academic because of the following:

For performance, I would like to reduce overhead in the case when
no plugin has registered for the events. This can be achieved by only
calling add_listener when a plugin actually calls the function to
connect to the db events. Thus we anyway need a function to connect
and we might as well use QueuedConnection there rather than
QTimer/QMetaObject.

Finally, taking a step back, the whole motivation for this is basically
to track times that events happened to a book. So why not just add that
to the database directly? Seems a lots less, umm wonky.
Something like an "events" table that tracks the event name,the book id and
the time the event occurred along with a free form event_data column.

To avoid creating a large performance overhead, most events will be
turned off by default, and an API provided to turn them on per library.

[Kovid Goyal (kovid)]

Regarding enter_key_pressed I implemented it slightly differently. It is now emitted when enter is pressed even in the cover grid.

[capink (capink-eg)]

Thanks a lot for adding the enter_key_pressed signal. I tested it and it is working, also much better that it can be activated in cover grid as well.

Your suggestion about an event table that is turned off by default, sounds like a cleaner and more robust solution, but it is way beyond what I can do.

[Kovid Goyal (kovid)]

OK I will look into adding an events table, when I have a little spare
time, am rather swamped at the moment.

[Charles Haley (cbhaley)]

I suspect that even with the event table some sort of signal will still be required so clients can know something happened and do required GUI/data updates.

[Kovid Goyal (kovid)]

Depends on the use case I think. If it is to update a custom column then
yes, if it is simply to display the information to the user on demand,
it can be synthesized from the table.

@capink: enlighten us as to your use case.

[capink (capink-eg)]

I need to update a custom column. In addition to the books_ids it also needs some event arguments e.g. the field name in set_field().

[Kovid Goyal (kovid)]

Fixed in branch master. The fix will be in the next release. calibre is usually released every alternate Friday.

 status fixreleased

[Kovid Goyal (kovid)]

I have implemented this, in your plugin you just need to call self.gui.add_db_listener(self.your_callback)

I have not tested it, I leave that to you.

[Kovid Goyal (kovid)]

Note that you will get events not just for the currently displayed db but also any dbs thatarebeing served in thebackground by the embedded content server.

[capink (capink-eg)]

Thanks a lot for adding this to calibre. Here are some notes:

- When I test this I get this error:

Traceback (most recent call last):
  File "/home/user/calibre/src/calibre/gui2/ui.py", line 401, in initialize
    ac.initialization_complete()
  File "calibre_plugins.last_modified.action", line 55, in initialization_complete
    self.gui.add_db_listener(self.process_event_in_db)
  File "/home/user/calibre/src/calibre/gui2/ui.py", line 205, in add_db_listener
    self.library_broker.start_listening_for_db_events()
  File "/home/user/calibre/src/calibre/srv/library_broker.py", line 225, in start_listening_for_db_events
    db.new_api.add_listener(self.on_db_event)
AttributeError: 'str' object has no attribute 'new_api'

- I guess I can discard events from other dbs by using:

if not db.library_id == self.gui.current_db.library_id:
    return

- Thanks for adding the extra events (book_created, books_removed). They will be very useful.

- At the risk of sounding a bit greedy, there is one event missing (book_edited), can you please add that as well. I added it in the attached my-changes file.

[Kovid Goyal (kovid)]

On Tue, Aug 10, 2021 at 11:04:05AM -0000, capink wrote:
> Thanks a lot for adding this to calibre. Here are some notes:
>
> - When I test this I get this error:
>
> Traceback (most recent call last):
> File "/home/user/calibre/src/calibre/gui2/ui.py", line 401, in initialize
> ac.initialization_complete()
> File "calibre_plugins.last_modified.action", line 55, in initialization_complete
> self.gui.add_db_listener(self.process_event_in_db)
> File "/home/user/calibre/src/calibre/gui2/ui.py", line 205, in add_db_listener
> self.library_broker.start_listening_for_db_events()
> File "/home/user/calibre/src/calibre/srv/library_broker.py", line 225, in start_listening_for_db_events
> db.new_api.add_listener(self.on_db_event)
> AttributeError: 'str' object has no attribute 'new_api'

this is now fixed.

>
> - I guess I can discard events from other dbs by using:
>
> if not db.library_id == self.gui.current_db.library_id:
> return
>

Simply compare the db objects no need to use ids.

>
> - Thanks for adding the extra events (book_created, books_removed). They will be very useful.
>
> - At the risk of sounding a bit greedy, there is one event missing
> (book_edited), can you please add that as well. I added it in the
> attached my-changes file.

Done.

[capink (capink-eg)]

I am getting the following error:

Traceback (most recent call last):
  File "/home/user/calibre/src/calibre/gui2/ui.py", line 401, in initialize
    ac.initialization_complete()
  File "calibre_plugins.last_modified.action", line 55, in initialization_complete
    self.gui.add_db_listener(self.process_event_in_db)
  File "/home/user/calibre/src/calibre/gui2/ui.py", line 205, in add_db_listener
    self.library_broker.start_listening_for_db_events()
  File "/home/user/calibre/src/calibre/srv/library_broker.py", line 225, in start_listening_for_db_events
    db.new_api.add_listener(self.on_db_event)
  File "/home/user/calibre/src/calibre/db/cache.py", line 77, in call_func_with_lock
    return func(*args, **kwargs)
  File "/home/user/calibre/src/calibre/db/cache.py", line 435, in add_listener
    self.event_dispatcher.add_listener(event_callback_function)
  File "/home/user/calibre/src/calibre/db/listeners.py", line 64, in add_listener
    self.start()
AttributeError: 'EventDispatcher' object has no attribute 'start'

Also When I quit calibre I get another error:

Traceback (most recent call last):
  File "/home/user/calibre/src/calibre/gui2/ui.py", line 1069, in quit
    self.shutdown()
  File "/home/user/calibre/src/calibre/gui2/ui.py", line 1166, in shutdown
    self.library_view.model().close()
  File "/home/user/calibre/src/calibre/gui2/library/models.py", line 373, in close
    self.db.close()
  File "/home/user/calibre/src/calibre/db/legacy.py", line 211, in close
    self.new_api.close()
  File "/home/user/calibre/src/calibre/db/cache.py", line 77, in call_func_with_lock
    return func(*args, **kwargs)
  File "/home/user/calibre/src/calibre/db/cache.py", line 2243, in close
    self.event_dispatcher.close()
  File "/home/user/calibre/src/calibre/db/listeners.py", line 78, in close
    self.join()
AttributeError: 'EventDispatcher' object has no attribute 'join'

I also attached the plugin I am using for testing.

[Kovid Goyal (kovid)]

oops, should be fine now. Basic event delivery is working calling
gui.add_listener(print)

On Tue, Aug 10, 2021 at 12:12:16PM -0000, capink wrote:
> I am getting the following error:
>
> Traceback (most recent call last):
> File "/home/user/calibre/src/calibre/gui2/ui.py", line 401, in initialize
> ac.initialization_complete()
> File "calibre_plugins.last_modified.action", line 55, in initialization_complete
> self.gui.add_db_listener(self.process_event_in_db)
> File "/home/user/calibre/src/calibre/gui2/ui.py", line 205, in add_db_listener
> self.library_broker.start_listening_for_db_events()
> File "/home/user/calibre/src/calibre/srv/library_broker.py", line 225, in start_listening_for_db_events
> db.new_api.add_listener(self.on_db_event)
> File "/home/user/calibre/src/calibre/db/cache.py", line 77, in call_func_with_lock
> return func(*args, **kwargs)
> File "/home/user/calibre/src/calibre/db/cache.py", line 435, in add_listener
> self.event_dispatcher.add_listener(event_callback_function)
> File "/home/user/calibre/src/calibre/db/listeners.py", line 64, in add_listener
> self.start()
> AttributeError: 'EventDispatcher' object has no attribute 'start'
>
> Also When I quit calibre I get another error:
>
> Traceback (most recent call last):
> File "/home/user/calibre/src/calibre/gui2/ui.py", line 1069, in quit
> self.shutdown()
> File "/home/user/calibre/src/calibre/gui2/ui.py", line 1166, in shutdown
> self.library_view.model().close()
> File "/home/user/calibre/src/calibre/gui2/library/models.py", line 373, in close
> self.db.close()
> File "/home/user/calibre/src/calibre/db/legacy.py", line 211, in close
> self.new_api.close()
> File "/home/user/calibre/src/calibre/db/cache.py", line 77, in call_func_with_lock
> return func(*args, **kwargs)
> File "/home/user/calibre/src/calibre/db/cache.py", line 2243, in close
> self.event_dispatcher.close()
> File "/home/user/calibre/src/calibre/db/listeners.py", line 78, in close
> self.join()
> AttributeError: 'EventDispatcher' object has no attribute 'join'
>
> I also attached the plugin I am using for testing.
>
> ** Attachment added: "Last Modified 0.3.0.zip"
> https://bugs.launchpad.net/calibre/+bug/1937898/+attachment/5517086/+files/Last%20Modified%200.3.0.zip
>
> --
> You received this bug notification because you are subscribed to
> calibre.
> https://bugs.launchpad.net/bugs/1937898
>
> Title:
> [Enhancement request] pyqt signlas in calibre source
>
> Status in calibre:
> Fix Released
>
> Bug description:
> Wonder if it is possible to add some new pyqt siganls to calibre. The
> siganls are in my-changes file attached to this bug report. I use the
> signals in plugin I have that enables the user to have his own custom
> last_modified column(s), since last_modified in calibre is reserved
> for internal use. The plugin can update one or more custom date
> column(s) whenever one or more of the following events happen:
>
> - book metadata updated.
> - cover modified.
> - format added.
> - format(s) removed.
> - category item renamed.
> - category item removed.
> - book edit...

[capink (capink-eg)]

Thanks a lot. The errors are gone and it is working now. But I am not able to make it work when I switch libraries. I tried de-registering/re-registering using this:

def library_changed(self, db):
    self.gui.remove_db_listener(self.process_event_in_db)
    self.gui.add_db_listener(self.process_event_in_db)

But I was not able to make it work. What should I do?

[Kovid Goyal (kovid)]

On Tue, Aug 10, 2021 at 01:39:04PM -0000, capink wrote:
> Thanks a lot. The errors are gone and it is working now. But I am not
> able to make it work when I switch libraries. I tried de-registering/re-
> registering using this:
>
> def library_changed(self, db):
> self.gui.remove_db_listener(self.process_event_in_db)
> self.gui.add_db_listener(self.process_event_in_db)
>
> But I was not able to make it work. What should I do?

I fixed it.
https://github.com/kovidgoyal/calibre/commit/e2a088a53b1c2a1edd9b561c1912fe6a476ffff9

>
> --
> You received this bug notification because you are subscribed to
> calibre.
> https://bugs.launchpad.net/bugs/1937898
>
> Title:
> [Enhancement request] pyqt signlas in calibre source
>
> Status in calibre:
> Fix Released
>
> Bug description:
> Wonder if it is possible to add some new pyqt siganls to calibre. The
> siganls are in my-changes file attached to this bug report. I use the
> signals in plugin I have that enables the user to have his own custom
> last_modified column(s), since last_modified in calibre is reserved
> for internal use. The plugin can update one or more custom date
> column(s) whenever one or more of the following events happen:
>
> - book metadata updated.
> - cover modified.
> - format added.
> - format(s) removed.
> - category item renamed.
> - category item removed.
> - book edited by calibre editor.
>
> This allows the user more granularity in defining what a custom
> #last_modified column(s) means without disrupting the last_modified
> column of calibre.
>
> N.B. There is also one extra enter_key_pressed signal in the attached
> file that I use with the Actions Chains plugin to define actions
> whenever the enter key is pressed. Please let me know if I should open
> a separate report for this.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/calibre/+bug/1937898/+subscriptions
>
>

--
_____________________________________

Dr. Kovid Goyal
https://www.kovidgoyal.net
https://calibre-ebook.com
_____________________________________

[capink (capink-eg)]

It is working perfectly :) Thanks a million.