search

Gnurou / tagainijisho

A free Japanese dictionary and learning assistant
http://www.tagaini.net
GNU General Public License v3.0
364 stars 51 forks source link

Show all dictionary data from KANJIDIC. #231

Closed ErezVolk closed 3 years ago

ErezVolk commented 4 years ago

For those of us still using paper kanji dictionaries, this optionally adds a table with all page numbers available in KANJIDIC2.

My biggest problem was that the full dictionary names (e.g., "A Guide To Reading and Writing Japanese" 3rd edition, edited by Henshall, Seeley and De Groot.) wouldn't fit in the table. What I did was include the dr_type field as-is (henshall3), and add a tooltip with the full info. Is isn't too pretty. I guess we could use our own acronyms, but that would require another lookup table in SQLite and I didn't really know what to put in there.

What do you think? Useful? Needs some modification?

image
Gnurou commented 4 years ago

Hi! Just a quick note to apologize for being slow with reviewing this - I'm a bit busy at the moment. Will come back to this PR soon.

ErezVolk commented 4 years ago

No problem!

ErezVolk commented 4 years ago

OK, let me know what you think :)

Gnurou commented 4 years ago

Hey! I'm reviewing the latest version and it looks mostly ok (want to do a second pass though). I see that you added your fixes as follow-up patches instead of force-pushing to the same branch, do you mind if I squash/split your patches as appropriate? I'll of course preserve authorship.

ErezVolk commented 4 years ago

Sure, squash away!

wsakernel commented 3 years ago

I merged this branch into master as of today, and it still worked fine. I use Spahn & Hadamitzky books, so this is really helpful to me. I can't really help wit the review sadly, yet I'd love to see this applied.

Gnurou commented 3 years ago

Sorry, I completely forgot about this issue. >_< IIRC this was looking good last time I checked, so leaving a note to myself to squash and merge as appropriate.

wsakernel commented 3 years ago

I could also squash (and provide) if that helps. I just can't merge ;)

Gnurou commented 3 years ago

So first, I am very sorry for taking so long to merge this. But after reviewing, squashing the commits into one (since having several follow-ups that fixed a newly-introduced feature did not make much sense) and tuning a few Qt deprecation warnings, I am happy to say that this is now merged! Thanks for the very useful feature!