search

fonol / anki-search-inside-add-card

An add-on providing full-text-search and PDF reading functionality to Anki's Add card dialog
https://ankiweb.net/shared/info/1781298089
GNU Affero General Public License v3.0
179 stars 24 forks source link

Added threading and options to limit the search #195

Closed aviral-batra closed 3 years ago

aviral-batra commented 3 years ago

1) Put the search in a QRunnable 2) Gave an option to limit the number of items in the search. 3) Simplified the search function into one function i.e. instead of only choosing between top level or everything, now you can specify how many 'levels' of recursion you want to limit the search by

Could you add the button press to cancel the search that you mentioned in your comment? I'm not sure how to kill the thread safely while it's running using pyqt.

Also, I've used a pretty 'hacky' way to manage the thread i.e. to prevent a new worker being created every time the refresh function is called (I set a property of the dialog class to turn to False or True depending if the thread has started or finished). Please improve it if you see it fit.

fonol commented 3 years ago

Thanks a lot.

Could you add the button press to cancel the search that you mentioned in your comment? I'm not sure how to kill the thread safely while it's running using pyqt.

Sure, I'll try.

fonol commented 3 years ago

The scan is now running in a QThread, and aborting should work in theory, and works sometimes in practice. There is still something wrong somewhere, as the behaviour is inconsistent for me when testing. I'll post back here if I got it working entirely.

aviral-batra commented 3 years ago

I'm a little confused - I think my initial approach may have made it a bit messy. Since we now have self.thread.isRunning(), can't we get rid of the original thread_running? Or maybe I'm missing something.

I'll try and have a think too.

fonol commented 3 years ago

I think my self.thread.isRunning() is superfluous and will always be True. So the thread_running is the important flag. The problem that drove me nuts yesterday was that the Abort button has immediate focus after showing it / enabling it, so if it was shown when I triggered a search with returnPressed, sometimes the search was immediately aborted.

There is still a way to freeze the GUI, if you select a large dir like C:/, uncheck limit searches to and limit the number of levels shown to. Maybe the problem then is that too many items are added to the list in too little time which keeps the GUI thread busy. But otherwise, it now works fine for me.

I think I have misgrouped the options a bit, limit searches to and limit the number of levels shown to would apply to the final scan for files too, right? Maybe limit the number of levels shown to should be renamed limit the number of levels scanned to, and those two options should be in their own GroupBox, right after the path input, like 2. General Scanning Options?

aviral-batra commented 3 years ago

There is still a way to freeze the GUI, if you select a large dir like C:/, uncheck limit searches to and limit the number of levels shown to. Maybe the problem then is that too many items are added to the list in too little time which keeps the GUI thread busy. But otherwise, it now works fine for me.

Yeah, I thought that. That's why I made a general worker which ran any function in a separate thread rather than a specific worker for the scan. If we had to do the adding to the list widget in the scan, we would have to reference the widget and the ui form in the separate worker, which would make it cluttered.

I think I have misgrouped the options a bit, limit searches to and limit the number of levels shown to would apply to the final scan for files too, right? Maybe limit the number of levels shown to should be renamed limit the number of levels scanned to, and those two options should be in their own GroupBox, right after the path input, like 2. General Scanning Options?

No, limit the number of levels shown only apply to what is displayed - whatever directories you select are not scanned and then if you select recursive, then all the subdirectories are not scanned too. The limit is simply there so you don't see loads of stuff, but perhaps it should be different.

Maybe we could ditch the line edit and instead use QFileDialog to select multiple files? A dialog opens for you to select the directory to scan and then another opens so you can select the files you want to ignore. We can keep the current options we have and then add options to ignore certain file types, as you suggested.

fonol commented 3 years ago

Yeah, I thought that. That's why I made a general worker which ran any function in a separate thread rather than a specific worker for the scan. If we had to do the adding to the list widget in the scan, we would have to reference the widget and the ui form in the separate worker, which would make it cluttered.

I would have kept that, but I couldn't send an abort signal to QRunnable, which according to https://doc.qt.io/qt-5/threads-technologies.html doesn't support it. The current implementation is not very generic I agree, but I was unsure how to handle the reporting of the found results for generic tasks.

Maybe we could ditch the line edit and instead use QFileDialog to select multiple files? A dialog opens for you to select the directory to scan and then another opens so you can select the files you want to ignore. We can keep the current options we have and then add options to ignore certain file types, as you suggested.

That's also an idea. I really don't care that much tbh, as long as I can lazily import my folders full of PDFs I am happy.

If you want, I can add the note creation part, e.g. the GUI elements that allow to select priority, special schedule and tags. Happy new year btw :).

aviral-batra commented 3 years ago

That's also an idea. I really don't care that much tbh, as long as I can lazily import my folders full of PDFs I am happy.

😆 That's completely relatable.

I would have kept that, but I couldn't send an abort signal to QRunnable, which according to https://doc.qt.io/qt-5/threads-technologies.html doesn't support it. The current implementation is not very generic I agree, but I was unsure how to handle the reporting of the found results for generic tasks.

That makes sense.

If you want, I can add the note creation part, e.g. the GUI elements that allow to select priority, special schedule and tags.

Sounds good.

I have some interviews coming up so I don't know if I'll be able to work on this for a week or two, but after that I can implement the QFiledialog stuff, unless you want to do it yourself in the meantime.

Happy new year btw :).

Happy new year 🎆 Let's hope it's a good one.