mozilla / high-fidelity

HTML5 podcast app (for the Web, including FirefoxOS)
https://marketplace.firefox.com/app/podcasts
108 stars 40 forks source link

Better downloads #13

Closed tofumatt closed 11 years ago

tofumatt commented 11 years ago

Downloads episodes in a somewhat crude manner for now (happy to improve before we merge), but this seems to prevent the out of memory issues previously experienced downloading large files on devices.

Bonus: it doesn't assume almost any file is an MP3. Heh.

r? @potch or @sole?

sole commented 11 years ago

I tried to download and run the app but I'm clueless as how to do that. All I get is this empty screen :dancers:

screen shot 2013-05-21 at 16 03 25

tofumatt commented 11 years ago

I should totally update the instructions! But for now, so you know, you need to run nom install and make at the command line, then add the www-built folder to the simulator.

Sadly, packaged apps aren't very easy to test because of CSP restrictions. :-(

sole commented 11 years ago

I did that (btw, npm install took ages :tongue:), but I'm still getting the same black screen. I re-discovered the simulator has a "Console..." checkbox and used it, so I can provide you with the exact error:

Content JS LOG at app://ac440a39-7c1f-054b-a168-11ded1a34cf4/js/main.js:11423 in setLanguage: [Exception... "File error: Not found" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: app://ac440a39-7c1f-054b-a168-11ded1a34cf4/js/main.js :: setLanguage :: line 11421" data: no]

maybe this isn't helpful at all, or maybe I should create a new issue? Let me know :-)

I also forked the repo and added the instructions you provided me into README.md. A pull request is there :)

tofumatt commented 11 years ago

Hmm, that seems to be the language files not loading. Normally AJAX errors like that aren't a big deal but packaged apps go nuts if you try to load a "local" file and it fails.

For what it's worth, running make update_locale_json then make should fix it, but I'll add it into the make step so we don't get this error.

sole commented 11 years ago

screen shot 2013-05-22 at 14 52 12

So I did all that you said and I got it running, but I'm still getting errors :-/ and even worse, the simulator crashes! Could that be because I'm running Nightly? I could send you the stack trace but I'm not sure it'll help you. Should I report this to the b2g2b... guys?

I'm attaching an screenshot of the console, because if I click to copy the messages, it crashes.

I'm sorry this is being so inconvenient but I'm glad that it'll make the app more robust (I hope!)

tofumatt commented 11 years ago

Oh, that's quite odd. I'll hop on IRC in a few and we can see what's up with this perhaps?

tofumatt commented 11 years ago

Hey @sole, I just found the bug; I stupidly renamed a method but referenced its old name. This should be fixed now!

fwenzel commented 11 years ago

OH (tofumatt, 3 days ago): "It's probably Sole's fault!" ;)

tofumatt commented 11 years ago

Like a politician, I stand by my original remark, regardless of its evident truth.

tofumatt commented 11 years ago

So after some 1.0.1 testing it appears this code does fix some 1.1 bugs but does not work on 1.0.1.

I'd still love a quick r? to catch anything crazy, but I'm gonna keep at it for 1.0.1, because platform won't roll out any changes to the production devices and I'd rather this app work on shipping phones.

sole commented 11 years ago

OK, I did "the r"!! I'm not sure I did it the right way, so please bear with me :angel: (maybe there should be r? for r?'s!!)

I'm still not familiar with the whole codebase/backbone so I might have overseen something important :-P... And I'm not sure whether you prefer me to highlight the spelling mistakes or to send you a patch -let me know!

tofumatt commented 11 years ago

@sole, this was actually a perfect r? Sorry, I forgot to update it as I've been hacking on it locally.

I find it makes it a bit easier if you click on the files changed section and comment on it there rather than on the commit, but otherwise this was grand. Thanks!

I've updated the code to reflect changes.

If anyone else wants to comment, please do so soon as this should be merged into master soon!

sole commented 11 years ago

It was my first CR here! :angel:

Next time I'll click on the files changed for sure!

With those changes made, the code seemed nice to merge (you call it r+, right?)

tofumatt commented 11 years ago

Yeah, we usually do r? for "review?", r- for "review failed" (ie do another pass and ask for another review), r+ for "good to merge", and r+wc for good to merge if all comments are addressed.

sole commented 11 years ago

Then... r+wc!