Refactor cdrdao toc/table functions into Task and provide progress output #345

jtl999 · 2018-12-08T23:12:16Z

This PR fixes issue #299 and also refactors the cdrdao read toc function into a Task as a prerequisite for this. I've rebased my changes against the upstream/develop branch and ran a test rip just now.

A question. Now that at present we aren't using fast_toc, do we really need to read the TOC and table separately, when it appears they contain the same data, except for the offset in the table. Would shave some time off ripping if it's possible to only need to do it once, but I feel such a change would be out of scope of this PR.

Fixes #299.

jtl999 · 2018-12-08T23:31:04Z

Didn't know you used Flake8. I'll go take a look at implementing the changes so it passes later if needed/wanted.

JoeLametta · 2018-12-09T10:18:28Z

💖 Thanks for opening your first pull request here! 💖

Didn't know you used Flake8. I'll go take a look at implementing the changes so it passes later if needed/wanted.

I've just rebased your branch on whipper's develop and fixed flake8's warnings with a new commit.
I haven't reviewed the changes, maybe you could split them into several commits? (for increased clarity)

jtl999 · 2018-12-09T21:24:50Z

Argh. It took me longer to rebase them onto a single commit but I'll see what I can do.

jtl999 · 2018-12-09T23:58:05Z

From my personal feature branch (without the single commit rebase)

I'll look over each commit and provide a more detailed commit message for each commit.

commit 3445eb2804d840508319779c21c4acee22df18fa (HEAD -> verbose-toc, origin/verbose-toc, gitlab/verbose-toc)
Author: JTL <jtl@teamclassified.ca>
Date:   Fri Dec 7 23:25:02 2018 -0800

    Another attempt at getting preserve-toc working

commit 4ccf4093f4d1cd88a417f8d0d8dcfebb3124a69c
Author: JTL <jtl@teamclassified.ca>
Date:   Wed Dec 5 22:14:55 2018 -0800

    First attempt at getting toc path working

commit 0761111934c9830f95dffceb55091d4418881ea2
Author: JTL <jtl@teamclassified.ca>
Date:   Sun Dec 2 21:46:44 2018 -0800

    Provide progress of cdrdao toc reading to console

commit 825ca380227a985c47379f269aed7ce12d346ac9
Author: JTL <jtl@teamclassified.ca>
Date:   Sat Dec 1 21:35:51 2018 -0800

    Add cdrdao regexes to cdrdao.py

# This was a private notes text file for myself, not needed to be merged.
commit b89fde4439f92e268530a669cf89ebdd21783d75
Author: JTL <jtl@teamclassified.ca>
Date:   Sat Dec 1 17:40:53 2018 -0800

    Add notes for cdrdao regex parsing

commit 1e69a3db50cfd25831b8dada364e2c61e6ac3993
Author: JTL <jtl@teamclassified.ca>
Date:   Sat Dec 1 17:26:21 2018 -0800

    asynchronously be able to parse cdrdao output now

commit b9fe29d6df06ce5c8de2ab5c60d110ab4410862e
Author: JTL <jtl@teamclassified.ca>
Date:   Fri Nov 30 21:48:23 2018 -0800

    cdrdao is now a Task

commit 1a9e64ce6061bb705acb491ad2ee9b82a412fc01
Author: JTL <jtl@teamclassified.ca>
Date:   Fri Nov 30 19:23:06 2018 -0800

    Remove some unneeded code

commit a92145d36135e16227b5098bb7bfa11168e6bdd1
Author: JTL <jtl@teamclassified.ca>
Date:   Fri Nov 30 19:08:56 2018 -0800

    Work on inital attempt to move cdrdao to a Task

jtl999 · 2018-12-11T05:54:56Z

Alright.

Redid my issue/299 branch privately with more commits and more of an explanation per commit. That being said I merged the bugfix/issue-335-drop-caching branch from upstream into it and then realized that it's technically not merged into master yet.

What's the best course of action in your opinion?

jtl999 · 2018-12-11T06:15:42Z

Managed to undo that merge via rebase and force pushed an update to issue/299

jtl999 · 2018-12-11T06:40:51Z

A note: The progress for reading TOC/table isn't "exact" because it updates the progress upon cdrdao having started analyzing a track, as opposed after it's finished analyzing a track.

Just thought I'd mention that.

JoeLametta · 2018-12-11T14:58:09Z

I've just rebased your branch on whipper's develop, squashed some commits and fixed flake8's warnings with a separate commit.

MerlijnWajer · 2018-12-13T19:05:48Z

Why is it called ReadTOC_Task with a '_' now? (Just a nitpick)

jtl999 · 2018-12-13T20:24:12Z

Because while I was debugging cdrdao.py I wanted to have both functions still available.

I'm sure it can be safely changed back now. Oops.

JoeLametta · 2018-12-13T21:04:19Z

I've just rebased your branch on whipper's develop (fixing the conflicts) and restored the old task name (ReadTOCTask)... 😉

jtl999 · 2018-12-13T21:05:08Z

I see. Thanks!

JoeLametta · 2018-12-13T21:48:55Z

A question. Now that at present we aren't using fast_toc, do we really need to read the TOC and table separately, when it appears they contain the same data, except for the offset in the table. Would shave some time off ripping if it's possible to only need to do it once, but I feel such a change would be out of scope of this PR.

Related issue #173.
More information about fast_toc in #264. 😉

Freso

Just from looking at the code, I don't see anything wrong, but the meat of it is also more grok at 11 PM as I'm winding down and getting ready for sleep. :)

A couple of stylistic comments at least though.

whipper/program/cdrdao.py

jtl999 · 2018-12-14T04:58:14Z

Implemented the style changes.

With regards to your comment on IRC (about the mix of single and double quotes) how do you feel it should be done? I was trying to emulate the style of whipper/programs/cdparanoia.py (Which I read to understand the task system)

Freso · 2018-12-14T09:39:05Z

With regards to your comment on IRC (about the mix of single and double quotes) how do you feel it should be done?

I usually go for single quoting in my own projects, but am fine with double quotes too. Just as long as it's consistent. Anyway, I didn't remark on it on my previous comment to this PR, because it is irrelevant to this PR. Please just keep your commits atomic and your PRs self-contained. If we want to fix quotation style, that is its own PR (or something that can be done gradually in other PRs once it's been decided on one way or another).

Edit: I just now see that @JoeLametta went ahead and added a commit to this PR anyway, making https://github.com/whipper-team/whipper/pull/345/files even harder to make sense of. 🤦‍♂️ @JoeLametta, can we please try and keep PRs self-contained? Please? 🙇‍♂️

whipper/program/cdrdao.py

JoeLametta · 2018-12-14T20:32:59Z

@Freso Sorry for the mess, I've redone everything from scratch. Hope it's better this time...

jtl999 · 2018-12-14T20:43:38Z

can we please try and keep PRs self-contained?

I agree.

JoeLametta · 2019-01-18T11:20:01Z

@Freso Do you think latest revision of this pull requests is ready for merging?

jtl999 · 2019-01-18T22:45:14Z

Something that might be out of scope for this PR but worthwhile for the 1.0 milestone: My uncle pointed out recently when I was showing him the code and explaining to him how it works (He's been a long time Linux user, but not much of a programmer outside of very trivial Python tasks and specifically was an early user of cdparanoia/cdda2wav and EAC in late 90's.

He thinks the functionality of the code is great but it should have some documentation explaining the concept of Q sub-channel errors and what it means for ripping the disc. In my case with a TSSTCorp SH-S223L and some scratched CD's I tested with, getting above 1500 Q sub-channel errors on a track usually means I cant reliably rip said track because whipper gets CRC mismatches in Test and Copy mode.

JoeLametta · 2019-01-20T21:17:59Z

Something that might be out of scope for this PR but worthwhile for the 1.0 milestone

👍

welcome · 2019-02-02T17:36:07Z

Congrats on merging your first pull request! 🎉🎉🎉

Freso · 2019-03-17T13:59:27Z

This introduced #374

Broken since whipper-team#345 was merged. Signed-off-by: Andreas Oberritter <obi@saftware.de>

Output lines of cdrdao for single digit track numbers with a whitespace character. Broken since whipper-team#345 was merged. Signed-off-by: Andreas Oberritter <obi@saftware.de>

Broken since whipper-team#345 was merged. Signed-off-by: Andreas Oberritter <obi@saftware.de>

Output lines of cdrdao for single digit track numbers start with a whitespace character. Broken since whipper-team#345 was merged. Signed-off-by: Andreas Oberritter <obi@saftware.de>

JoeLametta force-pushed the issue/299 branch from 8361301 to 2565ba9 Compare December 9, 2018 10:07

JoeLametta changed the title ~~Refactor cdrdao toc/table functions into Task and provide progress output~~ WIP: Refactor cdrdao toc/table functions into Task and provide progress output Dec 9, 2018

jtl999 force-pushed the issue/299 branch from 2565ba9 to fa3bdff Compare December 11, 2018 06:14

JoeLametta force-pushed the issue/299 branch from fa3bdff to 899e15f Compare December 11, 2018 14:53

JoeLametta requested a review from MerlijnWajer December 11, 2018 14:58

JoeLametta force-pushed the issue/299 branch 2 times, most recently from 960cba1 to d123215 Compare December 12, 2018 09:18

JoeLametta force-pushed the issue/299 branch from d123215 to be1e275 Compare December 13, 2018 21:01

jtl999 force-pushed the issue/299 branch from be1e275 to 1cddc55 Compare December 13, 2018 21:02

JoeLametta force-pushed the issue/299 branch from 1cddc55 to be1e275 Compare December 13, 2018 21:03

JoeLametta requested a review from Freso December 13, 2018 21:49

Freso requested changes Dec 13, 2018

View reviewed changes

whipper/program/cdrdao.py Show resolved Hide resolved

whipper/program/cdrdao.py Outdated Show resolved Hide resolved

whipper/program/cdrdao.py Outdated Show resolved Hide resolved

JoeLametta force-pushed the issue/299 branch from 7aab6d4 to d121feb Compare December 14, 2018 08:44

Freso reviewed Dec 14, 2018

View reviewed changes

whipper/program/cdrdao.py Outdated Show resolved Hide resolved

JoeLametta force-pushed the issue/299 branch from d121feb to 4d313b7 Compare December 14, 2018 20:28

jtl999 and others added 5 commits January 18, 2019 10:55

Begin work on moving cdrdao to a task

0aad3af

Add code to start cdrdao task

38cdfa6

Allow cdrdao output to be asynchronously parsable

61eacae

Provide progress of cdrdao read toc/table to console

aea6af3

Flake8 fixes, Freso's advices

3852e6e

JoeLametta force-pushed the issue/299 branch from 4d313b7 to 3852e6e Compare January 18, 2019 09:55

JoeLametta merged commit 3e79032 into whipper-team:develop Feb 2, 2019

JoeLametta changed the title ~~WIP: Refactor cdrdao toc/table functions into Task and provide progress output~~ Refactor cdrdao toc/table functions into Task and provide progress output Feb 2, 2019

Freso mentioned this pull request Mar 17, 2019

Failure to rip CD: "ValueError: could not convert string to float: " #374

Closed

mtdcr mentioned this pull request Sep 21, 2019

Figure out why morituri compares the cdrdao fast-toc against the slow-toc #173

Open

mtdcr added a commit to mtdcr/whipper that referenced this pull request Sep 22, 2019

Make getFastToc() fast again.

4a45db8

Broken since whipper-team#345 was merged. Signed-off-by: Andreas Oberritter <obi@saftware.de>

mtdcr added a commit to mtdcr/whipper that referenced this pull request Oct 20, 2019

Make getFastToc() fast again.

342bce9

Broken since whipper-team#345 was merged. Signed-off-by: Andreas Oberritter <obi@saftware.de>

mtdcr mentioned this pull request Oct 20, 2019

Make getFastToc() fast again #417

Merged

mtdcr mentioned this pull request Oct 20, 2019

Fix ripping discs with less than ten tracks #418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor cdrdao toc/table functions into Task and provide progress output #345

Refactor cdrdao toc/table functions into Task and provide progress output #345

jtl999 commented Dec 8, 2018 •

edited by JoeLametta

Loading

jtl999 commented Dec 8, 2018 •

edited

Loading

JoeLametta commented Dec 9, 2018

jtl999 commented Dec 9, 2018 •

edited

Loading

jtl999 commented Dec 9, 2018

jtl999 commented Dec 11, 2018 •

edited

Loading

jtl999 commented Dec 11, 2018

jtl999 commented Dec 11, 2018

JoeLametta commented Dec 11, 2018

MerlijnWajer commented Dec 13, 2018

jtl999 commented Dec 13, 2018

JoeLametta commented Dec 13, 2018

jtl999 commented Dec 13, 2018

JoeLametta commented Dec 13, 2018

Freso left a comment

jtl999 commented Dec 14, 2018

Freso commented Dec 14, 2018 •

edited

Loading

JoeLametta commented Dec 14, 2018

jtl999 commented Dec 14, 2018 •

edited

Loading

JoeLametta commented Jan 18, 2019

jtl999 commented Jan 18, 2019

JoeLametta commented Jan 20, 2019

welcome bot commented Feb 2, 2019

Freso commented Mar 17, 2019

Refactor cdrdao toc/table functions into Task and provide progress output #345

Refactor cdrdao toc/table functions into Task and provide progress output #345

Conversation

jtl999 commented Dec 8, 2018 • edited by JoeLametta Loading

jtl999 commented Dec 8, 2018 • edited Loading

JoeLametta commented Dec 9, 2018

jtl999 commented Dec 9, 2018 • edited Loading

jtl999 commented Dec 9, 2018

jtl999 commented Dec 11, 2018 • edited Loading

jtl999 commented Dec 11, 2018

jtl999 commented Dec 11, 2018

JoeLametta commented Dec 11, 2018

MerlijnWajer commented Dec 13, 2018

jtl999 commented Dec 13, 2018

JoeLametta commented Dec 13, 2018

jtl999 commented Dec 13, 2018

JoeLametta commented Dec 13, 2018

Freso left a comment

Choose a reason for hiding this comment

jtl999 commented Dec 14, 2018

Freso commented Dec 14, 2018 • edited Loading

JoeLametta commented Dec 14, 2018

jtl999 commented Dec 14, 2018 • edited Loading

JoeLametta commented Jan 18, 2019

jtl999 commented Jan 18, 2019

JoeLametta commented Jan 20, 2019

welcome bot commented Feb 2, 2019

Freso commented Mar 17, 2019

jtl999 commented Dec 8, 2018 •

edited by JoeLametta

Loading

jtl999 commented Dec 8, 2018 •

edited

Loading

jtl999 commented Dec 9, 2018 •

edited

Loading

jtl999 commented Dec 11, 2018 •

edited

Loading

Freso commented Dec 14, 2018 •

edited

Loading

jtl999 commented Dec 14, 2018 •

edited

Loading