Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port downloadaudio to Anki 2.1 #95

Merged
merged 8 commits into from
Nov 30, 2016

Conversation

marciomazza
Copy link
Contributor

What I did

I completely port the Download Audio add-on until I couldn't find any bug.

I did not test other add-ons, but incidentally made these broad changes to all codebase:

  • some porting from PyQt4 to PyQt5
  • some 2to3 fixes
  • porting to BeautfulSoup 4 (the version used by Anki)

I used these references as guides:

Please note that I do not know PyQt and this is the first time I see Anki's or this repo's source, so mistakes are expected. I tried my best though, to make at least downloadaudio work.

Points I suggest you revise

  • The uses of BeautifulSoup constructor, especially the occurences of BeautifulSoup(text, 'html.parser'). Maybe they should use another parser.
  • Some suppressions I made of encoding/decoding strings. I removed only parts which where causing bugs by changed semantics on python 3

Copy link
Owner

@ospalh ospalh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand this change.
Yes, AudioDownloader.init sets its url.
But that is called before this. So url gots set to "" after it got set in the old line 37?
(Honest question. I thought it worked before.)

@ospalh
Copy link
Owner

ospalh commented Nov 30, 2016

Thanks.
It is at least a great starting point. And possibly some will work just like this. For those, this will be just great. 😄
As i noted, i don't exactly get one of your changes. I'll deal with that some time later.
Anyway, I'll just merge this now.

@ospalh ospalh merged commit cfea1fd into ospalh:develop-anki21 Nov 30, 2016
@ospalh
Copy link
Owner

ospalh commented Nov 30, 2016

Oh, there is one change i should do before i release these:
I should credit you in the file headers. 😄
(Unless you explicitly say you don't want to.)

@ospalh
Copy link
Owner

ospalh commented Nov 30, 2016

One more point:
I stuck to BS, even though i find the name rather fitting, because that is what Anki itself uses. I know that BS will be there. Easier than bringing your own parser along.

@marciomazza
Copy link
Contributor Author

The bug in WiktionaryDownloader was actually caused by this line:
https://github.com/ospalh/anki-addons/blob/develop/downloadaudio/downloaders/downloader.py#L134
which expects self.url to be just a prefix.
Before the fix it, when this line was executed, self.url would have the value http://{0}.wiktionary.org/wiki/{1}. The language part {0} had to be resolved by then.

All the rest of the downloaders treat AudioDownloader.url as a prefix, so now WiktionaryDownloader goes along.

The property setter for WiktionaryDownloader.url is necessary because without it the line
https://github.com/ospalh/anki-addons/blob/develop/downloadaudio/downloaders/downloader.py#L55
would raise an exception.

@marciomazza
Copy link
Contributor Author

Thanks for the credit and for the add-on. It really makes a difference on my study.

@ospalh
Copy link
Owner

ospalh commented Nov 30, 2016

Did you really see any problems without turning url into a property?
Because the line you pointed to should not be called for WiktionaryDownloader/wiktionary.py, because that reimplements maybe_get_icon(), the function you pointed to.
eta:
here

@marciomazza
Copy link
Contributor Author

marciomazza commented Nov 30, 2016 via email

@marciomazza
Copy link
Contributor Author

marciomazza commented Nov 30, 2016 via email

@ospalh
Copy link
Owner

ospalh commented Nov 30, 2016

Well, i can’t run it with Anki 2.1/Python3, and with Anki 2.0/Python 2 it is fetching the wiktionary site icon.

@marciomazza
Copy link
Contributor Author

marciomazza commented Nov 30, 2016 via email

@marciomazza marciomazza mentioned this pull request Jun 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants