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

rip cd info seems to fail #3

Closed
abendebury opened this issue Dec 11, 2015 · 6 comments
Closed

rip cd info seems to fail #3

abendebury opened this issue Dec 11, 2015 · 6 comments
Labels
Bug Generic bug: can be used together with more specific labels
Milestone

Comments

@abendebury
Copy link

https://what.cd/forums.php?action=viewthread&threadid=163034&postid=5702764#post5702764

@JoeLametta JoeLametta added the Bug Generic bug: can be used together with more specific labels label Dec 11, 2015
@JoeLametta
Copy link
Collaborator

Below I'm including, available to the public, the temporary ugly fix which solves this bug:

--- cd.py
+++ cd_fix.py
@@ -113,11 +113,19 @@
                     self.program.ejectDevice(self.device)
                 return -1

-        # now, read the complete index table, which is slower
+        # Ugly fix for broken commit
+        offset = 0
+        info = drive.getDeviceInfo(self.parentCommand.options.device)
+        if info:
+            try:
+                offset = self.getRootCommand().config.getReadOffset(*info)
+            except KeyError:
+                pass

+        # now, read the complete index table, which is slower
         self.itable = self.program.getTable(self.runner,
             self.ittoc.getCDDBDiscId(),
-            self.ittoc.getMusicBrainzDiscId(), self.device, self.options.offset)
+            self.ittoc.getMusicBrainzDiscId(), self.device, offset)

         assert self.itable.getCDDBDiscId() == self.ittoc.getCDDBDiscId(), \
             "full table's id %s differs from toc id %s" % (

@abendebury abendebury modified the milestone: 1.0 Dec 13, 2015
@abendebury
Copy link
Author

So what's actually wrong with this fix? What would be a better way to do it?

@JoeLametta
Copy link
Collaborator

It feels "hackish": it doesn't follow the structure of the other code.

@abendebury
Copy link
Author

Could you be more specific? :P

How should this be implemented correctly?

@JoeLametta
Copy link
Collaborator

@PlasmaSheep My "solution" is just a slightly edited copy & paste from the Rip class of the same file.
I'm not sure about it but maybe a better solution would be to implement it through the consolidated addOptions and handleOptions functions.
In the meantime I've committed the ugly fix to the master branch.

What's your opinion about this?

@abendebury
Copy link
Author

You're more familiar with the code base I think, so if there's a nicer way to do it we can do that/

For now the ugly fix will serve, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Generic bug: can be used together with more specific labels
Projects
None yet
Development

No branches or pull requests

2 participants