ds300 / jetzt

Speed reader extension for chrome
Other
485 stars 124 forks source link

Dev master merge #86

Closed ds300 closed 10 years ago

ds300 commented 10 years ago

Hey folks. I did a big refactor here. I think things are working as they should again, but there may be bugs lurking.

One thing I've noticed is that the text playback seems a little choppy sometimes. I haven't changed anything about the way it works, so that's weird. It might just be my computer; if I hold down the backspace key in this textarea I'm writing in right now, the motion of the cursor is rather choppy too.

I'd like to merge this into master ASAP so could anyone comment on whether or not there is choppiness for you?

Also whether you find bugs.

nomicode commented 10 years ago

Testing now.

ds300 commented 10 years ago

I forgot to say what the refactor was all about... Simply put: I made the modules modular. They were polluting the global namespace after the original split out of jetzt.js, which was bad for the bookmarklet. I also cleaned up some abstractions and fleshed out window.jetzt significantly.

So now we have window.jetzt.view, window.jetzt.control, window.jetzt.parse, etc etc. I'll try to write up a proper API some time this week.

The changes also make it a heck of a lot easier to write tests and add new features.

nomicode commented 10 years ago

Thanks for the info. Tested the branch now. Works fine, no choppiness.

h0ru5 commented 10 years ago

None here either. However, I did just run it on this site, but did no in-depth testing or dive into the code.

Lets call it alpha and test it MS-style, e.g. just merge, cross fingers and wait for bug reports :grinning:

h0ru5 commented 10 years ago

one thing I noticed is that changes to the modifiers in the options page do not get persisted or even land in the upper scope. But this is probably totally unrelated to your commits, but some quirk in my angular-code (the ng-repeat on object properties is a bit flaky)

I'll see to it tomorrow, but its not a blocker for the big merge IMHO

nomicode commented 10 years ago

Just noticed that this breaks the bookmarklet, which still depends on jetzt.js.

ds300 commented 10 years ago

@h0ru5 thanks, I'll look into that. if it was working before than it may well be my fault.

@nslater The plan is to use the minified version for the bookmarklet, and host it on gh-pages, but this should happen at the same time as merging into master.

RE: choppiness; Restared computer, went away.

nomicode commented 10 years ago

Now this is merged, what branch should I be sending PRs to?

ds300 commented 10 years ago

dev.

ds300 commented 10 years ago

@h0ru5 see here

Angular can be weird like that. I'll try and find a better workaround

h0ru5 commented 10 years ago

@ds300 I think the simplest would be to have an array of { name : 'bla', value : 1.0 } as modifiers, I think it also works when we do ng-model="options.modifiers[name]", but that's maybe a bit unintuitive. I'll look at it when I find some time this evening.

ds300 commented 10 years ago

@h0ru5 sorry I forgot to say that I fixed this last night. I gave ng-repeat a separate array of the modifier names to iterate over and used ng-model="options.modifiers[name]".