search

Notalib / LYT

m.e17.dk
GNU Lesser General Public License v3.0
10 stars 12 forks source link

Fix #581 by implementing "skip state" that skips all meta-content sections the first time a book is played #592

Closed saebekassebil closed 10 years ago

saebekassebil commented 10 years ago

This PR basically implements what I just wrote in the title.

The player object (LYT.player) now has a inSkipState property that reflects whether we're in skip state. This is decided in the Lyt.player.load function by checking if there's a .lastmark property on the book.

I need someone to review and test this code. I've tested on book 38737 which should like a charm, because that's the one I coded against. I'd also like to know if there are any other tags/classes in the NCC file that we'd like to regard as meta-content sections. This is however easy to extend.

saebekassebil commented 10 years ago

@simmoe would you like to take a look at this? I've uploaded a demo to [no longer valid] Note that not all books will skip the intro, since they haven't got any metadata sections, just randomly named "track01", "track02", etc. sections.

simmoe commented 10 years ago

There's something buggy with the search section, so I can't play new titles when i've searched. Tricky thing is i need titles that are nt already on my shelf to test this. A lot easier to test when that's fixed - any perspectives?

S

Den 07/11/2013 kl. 14.26 skrev Jakob Miland notifications@github.com:

@simmoe would you like to take a look at this? I've uploaded a demo to [no longer valid] Note that not all books will skip the intro, since they haven't got any metadata sections, just randomly named "track01", "track02", etc. sections.

— Reply to this email directly or view it on GitHub.

simmoe commented 10 years ago

Problem occurs here: [no longer valid]

Den 07/11/2013 kl. 15.04 skrev Simon Moe simmoe@gmail.com:

There's something buggy with the search section, so I can't play new titles when i've searched. Tricky thing is i need titles that are nt already on my shelf to test this. A lot easier to test when that's fixed - any perspectives?

S

Den 07/11/2013 kl. 14.26 skrev Jakob Miland notifications@github.com:

@simmoe would you like to take a look at this? I've uploaded a demo to [no longer valid] Note that not all books will skip the intro, since they haven't got any metadata sections, just randomly named "track01", "track02", etc. sections.

— Reply to this email directly or view it on GitHub.

jarl-dk commented 10 years ago

DO NOT MERGE - needs review and testing

Why is this a pull request then? :-)

saebekassebil commented 10 years ago

DO NOT MERGE - needs review and testing

Why is this a pull request then? :-)

Well I need someone to review it - you're welcome, @jarl-dk :)

mzedeler commented 10 years ago

Whats up with the comment from @simmoe? Has it been resolved?

mzedeler commented 10 years ago

I'll test this now.

mzedeler commented 10 years ago

I've done a code review now. Testing has failed because of errors in the backend, so I'm postponing the test until monday.

mzedeler commented 10 years ago

I've run a short test and found that the skip code works, but we should skip sections named indhold, see book 39772.

Also, it seems that skip state is dropped implicitly when navigating because we call load when navigating in the book. This seems ok.

mzedeler commented 10 years ago

@simmoe: the DODP server is up again. Can you check that this works as expected?

mzedeler commented 10 years ago

I've just talked to @simmoe and we agreed to change the code so the disclaimer isn't skipped. Otherwise, the feature is approved.

saebekassebil commented 10 years ago

@mzedeler: Quick review again? Also, I've added the indhold field, but I know of no disclaimer field?

This is going to collide with the changes @LDHgithub and I are making to the infrastructure, but I think this should be pulled first, and then we'll fix it later when we conflict.

mzedeler commented 10 years ago

The disclaimer is called dbbcopyright and shouldn't be skipped.