[kiwidude]

Originally Posted by davidfor

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.

In terms of the moving between lists, the issue is "what" gets refreshed. Because depending on what the user is mentally thinking, it could be different. For instance, if they are thinking "I really want to have these books off of list A onto list B", then they may want to see list B after the action to see what list B now looks like. OTOH they may have had list A visible, and now want the refresh of list A done to prove the books are no longer on that list and prevent errors like you mention. And then there is the other twist where they may now be viewing a list at all, but they happen to know a book is on list A based on tags or whatever so they select/move it - in which case viewing a list is going to switch context completely which is likely undesired.

I was being lazy the other night when I just ripped out the list viewing as the "easy" answer. I think the best compromise I can do is that if the user is currently viewing the source list, then that source list gets refreshed. Otherwise no list gets viewed.

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:

I appreciate the input. As I have said far more than once here I know sod-all about Python (never used it before Calibre), and since my plugins are only a part-time exercise out of personal interest that have intermittent spurts of activity sometimes many months apart they tend to grow fairly organically. I hate Eclipse/Pydev with a passion for the utter lack of decent refactoring and testing tools I take for granted in the .NET world I do my day job in. So my care factor in making code changes for anything other than major functional reasons usually gets tempered by how many precious spare time hours I have to spend doing endless code&fix cycles and manually re-testing everything.

- 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.

I agree that separating would be nice, and there is definitely some copy/paste code that could be factored out. However it all comes at a "price" of lots more parameters you have to pass around the place and return from functions, since the gui updates need to have information about the books that were updated etc. There is also the complication of where gui warnings etc can occur part way through a function. Refactoring that is a royal *pita*. you end up with numerous "bitty" methods and huge parameter lists, creating future maintenance issues. It's a case of whether the hours of pain justify the effort .

As for the add/remove methods, well they do need to interact with the gui because the user could have tags/custom column rules applied to their list add/remove operations. In the case of an external plugin calling reading_list, they will have to take responsibility for doing those updates on screen. Otherwise what happens is the user sees no "effect" of doing the add/remove operation until they refresh their search or move up/down the row.

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

If I was writing a .NET application I would 100% agree with you. However from the countless hours of looking through the Calibre code, an exception raising based approach is just not that common. Now whether that is just Kovid's preference, or a "Python thing" I don't know. I'm sure as heck not going to try to start a revolution by trying to write my code differently to how the rest of Calibre works - writing these plugins is hard enough as it is