[Enhancement request] pyqt signlas in calibre source
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
calibre |
Fix Released
|
Undecided
|
Unassigned |
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.
capink (capink-eg) wrote : | #2 |
capink (capink-eg) wrote : | #3 |
capink (capink-eg) wrote (last edit ): | #4 |
changes in my-changes attachment
capink (capink-eg) wrote (last edit ): | #6 |
deleted.
description: | updated |
Kovid Goyal (kovid) wrote : Re: calibre bug 1937898 | #7 |
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) wrote : | #8 |
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) wrote : | #9 |
@capink: you should be able to do this using the same method as marked listeners. See db.view.
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) wrote : | #10 |
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) wrote : | #11 |
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) wrote (last edit ): | #12 |
@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) wrote : | #13 |
- Last Modified 0.3.zip Edit (28.0 KiB, application/zip)
Also here is the modified plugin if anyone is interested.
Charles Haley (cbhaley) wrote : | #14 |
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_
capink (capink-eg) wrote : | #15 |
@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.
capink (capink-eg) wrote : | #16 |
Charles Haley (cbhaley) wrote : | #17 |
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) wrote : | #18 |
OK, I got it. Would calling the method calling QTimer.singleShot() have the same effect?:
QTimer.
Charles Haley (cbhaley) wrote : | #19 |
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) wrote : | #20 |
Some comments:
1) Use weakref.
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.
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) wrote : | #21 |
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) wrote : | #22 |
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) wrote : | #23 |
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) wrote : | #24 |
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) wrote : | #25 |
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) wrote : | #26 |
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) wrote : Fixed in master | #27 |
Fixed in branch master. The fix will be in the next release. calibre is usually released every alternate Friday.
status fixreleased
Changed in calibre: | |
status: | New → Fix Released |
Kovid Goyal (kovid) wrote : | #28 |
I have implemented this, in your plugin you just need to call self.gui.
I have not tested it, I leave that to you.
Kovid Goyal (kovid) wrote : | #29 |
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) wrote : | #30 |
- my-changes Edit (1.4 KiB, text/plain)
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/
ac.
File "calibre_
self.
File "/home/
self.
File "/home/
db.
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.
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) wrote : Re: [Bug 1937898] Re: [Enhancement request] pyqt signlas in calibre source | #31 |
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/
> ac.initializati
> File "calibre_
> self.gui.
> File "/home/
> self.library_
> File "/home/
> db.new_
> 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.
> 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) wrote : | #32 |
- Last Modified 0.3.0.zip Edit (28.2 KiB, application/zip)
I am getting the following error:
Traceback (most recent call last):
File "/home/
ac.
File "calibre_
self.
File "/home/
self.
File "/home/
db.
File "/home/
return func(*args, **kwargs)
File "/home/
self.
File "/home/
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/
self.shutdown()
File "/home/
self.
File "/home/
self.db.close()
File "/home/
self.
File "/home/
return func(*args, **kwargs)
File "/home/
self.
File "/home/
self.join()
AttributeError: 'EventDispatcher' object has no attribute 'join'
I also attached the plugin I am using for testing.
Kovid Goyal (kovid) wrote : | #33 |
oops, should be fine now. Basic event delivery is working calling
gui.add_
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/
> ac.initializati
> File "calibre_
> self.gui.
> File "/home/
> self.library_
> File "/home/
> db.new_
> File "/home/
> return func(*args, **kwargs)
> File "/home/
> self.event_
> File "/home/
> 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/
> self.shutdown()
> File "/home/
> self.library_
> File "/home/
> self.db.close()
> File "/home/
> self.new_
> File "/home/
> return func(*args, **kwargs)
> File "/home/
> self.event_
> File "/home/
> 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:/
>
> --
> You received this bug notification because you are subscribed to
> calibre.
> https:/
>
> 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) wrote : | #34 |
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/
def library_
self.
self.
But I was not able to make it work. What should I do?
Kovid Goyal (kovid) wrote : | #35 |
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_
> self.gui.
> self.gui.
>
> But I was not able to make it work. What should I do?
I fixed it.
https:/
>
> --
> You received this bug notification because you are subscribed to
> calibre.
> https:/
>
> 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:/
>
>
--
_______
Dr. Kovid Goyal
https:/
https:/
_______
capink (capink-eg) wrote : | #36 |
It is working perfectly :) Thanks a million.
I uploaded a couple of screenshots that demonstrates what the plugin is trying to achieve with these signals.