search

chenyanming / calibredb.el

Emacs calibre client - An Ebook Management Solution in Emacs.
GNU General Public License v3.0
323 stars 17 forks source link

Fixing typos, simplifying some functions, question on implementing hash tables to fix speed issue #45

Closed c1-g closed 3 years ago

c1-g commented 3 years ago

I fixed some of the typo ("detial" to "detail") mistakes but, I believe there're still more out there.

Reduced the code of calibredb-candidates and calibredb-query-to-alist to be more readable. (See their commit respectively.)

Also, I don't think alists are suitable for this package at all. Since the info node (elisp) Plists and Alists stated that alists are quite slow for lookups, which, is what we are currently using them for and, their only advantage compared to plists is that it can be faster to add association on the front of an alist, a feature that we don't use. So either we replace them with plists for better speed but , plists will still experience the same speed issue when working with large number of books, or a more future-proof alternative, (elisp) Hash Tables, which are extremely fast when working with large number of books and are quite similar to alists but, this will take time and a lot of testing to be done. I can work on a PR but I still want your input on it.

chenyanming commented 3 years ago

Thanks you help simplify the codes, better error handling and a lot more on calibredb.el! I will merge that soon.

chenyanming commented 3 years ago

I agree that changing alist to hash table would speed up the query, but I am not sure how much benefits we can get, and it also needs a lot of time to refactor.

calibredb.el is slow on the first time to query metadata.db and turn that into alist, entry by entry (I am not sure if it speeds up by changing it into hash table, because we still need to turn every entry into a small hash table). Besides, the real time seaching is slow (Since I filter the results by comparing the real time query strings with every entry, I am still not sure how fast hash table can go).

I may not have time to do it. If you can help, it would be great!

By the way, it would help me if you work base on develop branch.

By the way, I am working on opds branch and already made a lot of changes. I would merge to develop soon to make the conflicts as less as possible.

chenyanming commented 3 years ago

Thanks again for your commits, however, after I rebased and tested, it breaks a lot and can not work.

It is because of the modification on calibredb-query is not carefully handled enough.

Since the query is critial, it is better to test on every famous system, such as macOS, windows, Linux etc, and TUI as well, after making changes.

When I switched to the original design, every things work alright. I believe the original design works and is tested on most people's systems.

  1. calibredb-sql-separator has to be defined and it is better to not use any characters that will exist in metadata.
  2. Of course calibredb-sql-newline can not be \n which will appear in title, comments etc. Therefore, using calibredb-chomp is not necessary if calibredb-sql-newline is not \n.
  3. Use tmp file and -init option is ok but cumbersome. I think quote is not an issue.
  4. Would you rebase to develop branch and recommit? Since I already merged opds branch, now it is the latest codes already.
c1-g commented 3 years ago

Yeah ,I'm really sorry about this, I should have tested the code more. I'll adjust the code for calibredb-query to be less aggressive and rebase it to develop branch. Closing this for now.