[GUI Plugin] Reading List

[kovidgoyal]

In Qt applications you cannot use GUI objects in threads other than the main thread. You can use the Dispatcher helper class in calibre.gui2 to ensure that a python function call happens in the main thread.

[davidfor]

I keep meaning to report a bug. Whenever I do a "Move to list", I get an error. The details are:

calibre, version 0.8.33
ERROR: Unhandled exception: <b>NameError</b>:global name 'previous' is not defined

Traceback (most recent call last):
  File "calibre_plugins.reading_list.action", line 273, in _move_selected_to_list
  File "calibre_plugins.reading_list.action", line 449, in move_books_to_list
NameError: global name 'previous' is not defined

I don't know how long this has been hapening, but the above is with the version you posted earlier for JimmXinu.

After looking at the code, I fixed it by copying

previous = self.gui.library_view.currentIndex()

from one of the other methods to just after the parameter checking in move_books_to_list.

[kiwidude]

@davidfor - thanks for that, try the attached version. Probably something I broke when refactoring for this API stuff.

@JimmXinu - try the attached version. The relevant methods now have a display_warnings parameter which you can set to False.

Bearing in mind that these methods should still be called from a GUI thread and not a background thread, because adding/removing from a list can result in changes to the database if the user has rules setup to modify tags/custom columns based on list add/remove. Any database updates need to be done on the UI thread to be sure there is no conflict with the user currently editing data manually etc. So hence why plugins like metadata downloads force the operation back onto the GUI thread by prompting with a dialog after the background work is done. I suggest you follow the same pattern if the rest of your plugin is doing background work stuff.

I have also exposed for your convenience:
get_book_list(list_name)
get_list_names(exclude_auto=True)

Let me know if I broke anything else, I have only given this a quick test.

[JimmXinu]

Originally Posted by kiwidude

Bearing in mind that these methods should still be called from a GUI thread and not a background thread, because adding/removing from a list can result in changes to the database if the user has rules setup to modify tags/custom columns based on list add/remove. Any database updates need to be done on the UI thread to be sure there is no conflict with the user currently editing data manually etc. So hence why plugins like metadata downloads force the operation back onto the GUI thread by prompting with a dialog after the background work is done. I suggest you follow the same pattern if the rest of your plugin is doing background work stuff.

@kiwidude-I thought I might be doing that wrong. Thanks for the advice. I've been looking at Count Pages and now Reading List to borrow ideas from. Is there a better background processing plugin you'd recommend for me to study?

Originally Posted by kiwidude

I have also exposed for your convenience:
get_book_list(list_name)
get_list_names(exclude_auto=True)

@kiwidude-Thanks. But you forgot the self parameter on get_book_list & get_list_names.

[kiwidude]

@JimmXinu - I've uploaded a new version of the zip with the self parameter on the previous post.

As for the background stuff, it depends on how you do it. Basically it depends on whether you want to do it in-process (faster execution for small jobs) or out of process (the latter being a "job" the gets put in the calibre jobs queue, like downloading metadata or send to device).

Count Pages (& Modify ePub) use the job queue (out of process) approach, which is the most common way of doing it. Effectively it is scheduling an executable process to be launched for each "job". This has the additional advantage that if the code being called has memory leaks (which anything related to book conversions does for instance) you are not going to crash the main calibre process.

Extract ISBN has both the in-process and out of process approaches. If the user selects a very small number of books (can't remember the threshold I put in, might may be something like 2 or less), then it uses the in-process approach. More than that, it uses out of process (for memory leak prevention reasons).

[weirichk]

Kiwidude,

Thank you for developing this plugin. Either I am missing something or I have a feature request. Is there a way for the sorted list to be exported as a file so (i.e. like a catalog)? The Kindle Fire doesn't seem to support collections nor does it display the books in the same order as the list. Having the list exported (which Calibre could then upload to the KF) would allow me to access the list and see the order I want to read books.

If this already exists please point me in the direction. If not, can it be implemented?

Thank you,
Kurt

[kiwidude]

@weirichk - no it isn't something you are failing to find. It's an idea that has been tossed around a couple of times in this thread (mainly by me) but no real clamouring of demand to justify the effort.

You could perhaps view the list in Calibre and then produce a catalog using the existing catalog feature, though I haven't tried it. My guess would be that it might lose the reading list sort order.

I agree that ultimately it would be nice for the plugin to have an option for a list that when synced it sends a special book to your device which contains the equivalent of a catalog but in reading list order. Maybe I'll get to it one day... I don't know enough about the catalog code in Calibre at the moment though to know what if any I could reuse. And last time this idea got thrown around someone started talking about how they wanted the ability to customise the output etc - it all starts becoming a bit of a beast.

[JimmXinu]

Originally Posted by kiwidude

@JimmXinu - I've uploaded a new version of the zip with the self parameter on the previous post.

@kiwidude - I appreciate your help and guidance. I'm redoing the internals of my plugin to (hopefully) be more robust and 'correct'. Which does delay me from implementing the features I wanted a Reading List API for.

But I took a break to test all the RL methods you've exposed for me. So far I've found everything works as I expected except:

• remove_books_from_all_lists(book_ids) doesn't remove all books -- doesn't when called from Reading List either. Looks like a real bug.
• move_books_to_list(...) - unless refresh_screen=False, it sets the library display to marked:reading_list_<fromlist> afterwards. That may be the intended behavior, but it surprised me.
• edit_list(list_name) it's a corner case, but if called with a non-existent list name, it displays an empty list and errors on 'OK'. I won't object if you declare it undefined behavior.

Thanks again,
Jim

[kiwidude]

Hi Jim,

Remove from all lists works fine for me? I have two lists, I add a book to one or both of them, then I choose Remove from All lists and the lists are now empty (if that book was the only one on it) or reduced otherwise? If you are seeing something else can you give me some steps to replicate it?

The move books to list thing, yeah I was a bit conflicted about what to do with that. As depending on what you are doing it might be you want to see the source list, the dest list, or not have any change at all. I have removed that view_list line from what I will officially release.

The edit_list thing doesn't bother me, I wouldn't seriously expect anyone to invoke that method externally, and if they did it is a simple matter of making sure your list is valid. My menus get rebuilt every time they are displayed so it can't really happen in normal plugin usage.

[JimmXinu]

kiwidude,

I have a test library with 11 books in it. I have three lists, including Default. To start, all three lists are empty.

I select all 11 books, do 'Add to all lists'. Each list has 11 entries. With all 11 still selected (or reselected), I do 'Remove from all lists', now each list has 5 entries--the same five in each list.

I haven't analyzed the remove_books_from_all_lists() in depth, but using book_ids as a parameter and assigning to it in the for...list_names loop looks suspicious to me.

Jim

[kiwidude]

Hmmm, I havent tried replicating it again but looking at the code after what you described I can see the issue. Copy/paste wrong when I refactored stuff for you, darn it. New version attached above, thanks for finding it. Havent looked at why I couldnt replicate it, dumb luck I guess.

[JimmXinu]

It's working now. That was the last issue I had.

Many thanks for adding this API for me.

Jim

[davidfor]

Originally Posted by kiwidude

@davidfor - thanks for that, try the attached version. Probably something I broke when refactoring for this API stuff.

It works without the error. But...

The displayed list isn't being refreshed to show the changes. Looking at the code, the old version has "self.view_list(source_list_name)" at the end of the "if removed_ids:" block. The new version doesn't have this or anything similar in the equivalent block. I also noticed that the remove is not updating the list either. So, this might be deliberate. But, it can lead to a problem. If I try to remove or move the same book from the list, I get an error that the book isn't in the list.

Also, when testing the remove from list, I noticed there isn't a message put into the status area when a remove is done. The tags in the book details aren't being updated either. The same goes when adding a book to a list. The issue is in "_add_selected_to_list" and "_remove_selected_from_list". Both of these have "refresh_screen=True" as an argument. But, refresh_screen seems to be set to False. I can see where the menu items are created and I would think that what you have done is correct. Adding "True" as an extra parameter to the call to "partial" here fixes the problem. It appears the partial object created by this method is not respecting the default parameter values for the method when called. From the documentation, I can't decide if this is deliberate or a bug.

In any case, I suggest removing the "refresh_screen" from arguments of these methods. The methods are designed to be used only from the menus. As such, a value like this can be hard-coded or taken from a property for the instance. This is the sort of thing I define as a property of the instance so that I can easily turn it on or off for everywhere during my testing. When debugging, I might also add it to a menu or on the settings dialog so that I can easily toggle the value without rebuilding the code.

[kiwidude]

@davidfor - see post #323/#324 above for the discussion on move to list and why I removed the view_list line. You obviously use the move to list functionality yourself (unlike me), and as I don't believe it is an API call that Jim needs to use I'm open to suggestions if you want the previous behavior restored.

I'll take a look at the refresh_screen stuff tonight. I tried to make the changes for Jim around midnight that night just to get something out for him, and clearly didn't give it my normal TLC.

[davidfor]

Originally Posted by kiwidude

@davidfor - see post #323/#324 above for the discussion on move to list and why I removed the view_list line. You obviously use the move to list functionality yourself (unlike me), and as I don't believe it is an API call that Jim needs to use I'm open to suggestions if you want the previous behavior restored.

I could have sworn I read through those posts last night but didn't see anything that I can see the problem, but I think the lists need to be refreshed when these actions are taken (add, remove and move) from the menu. One problem is that if the list isn't updated, the user can't be sure the action happened, and if the user tries again, they will get an error.

I'll take a look at the refresh_screen stuff tonight. I tried to make the changes for Jim around midnight that night just to get something out for him, and clearly didn't give it my normal TLC.

OK, after reading all this code and discussing it a little here, I feel myself dropping into code review mode. Here are the suggestions I would be making if this was shown to me at work:

- Separate the database and GUI updates. This will help with exposing methods as part of the API as it means that no unexpected side-effects will happen. And, looking at it, the remove and add methods, probably shouldn't be interacting with the GUI in any way.

- For errors in the database/API methods, raise exceptions for the errors. These can be caught and handled by the calling code.

- Refactor the code that does the GUI updates. You can easily move this this to a separate method to be called from a lot of places. I would use the code from the bottom of "remove_books_from_list".

- Where do the GUI updates get done? In this, case, I would probably do them from the methods the menu call. These would check the input was OK, call the database updates, display any messages and update the GUI. But, I might also leave the input checking there and move the rest of it to another method. I would have to look at the code reuse scenarios properly to decide.

Those are my quick thoughts on this. On the other side of this, I do like your code. It is well laid out, easy to read, method and variable names are obvious and I can easily understand it.