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

Bug 1000784 #26

Merged
merged 8 commits into from Jul 3, 2013
Merged

Bug 1000784 #26

merged 8 commits into from Jul 3, 2013

Conversation

troyane
Copy link
Contributor

@troyane troyane commented Jun 26, 2013

That is my first pull request.
My idea was to fix bug https://bugs.launchpad.net/mixxx/+bug/1000784.

if (result)
return OK;
return ERR;
if (result==ERR) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces around the ==

if (result)
return OK;
return ERR;
if (result==ERR) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one == slipped through

@rryan
Copy link
Member

rryan commented Jun 26, 2013

These all look like reasonable error messages to add to the SoundSources but they would only be triggered when we are loading the file (e.g. loading to a deck or analyzing). TagLib is what we use to analyze the files during library scanning and it doesn't look like this changes our taglib code at all (located in src/soundsource.cpp).

@rryan
Copy link
Member

rryan commented Jun 26, 2013

Also @troyane, you should merge with master to get the latest changes. I think you have some conflicts with changes I made in soundsourcecoreaudio.cpp in master.

@rryan
Copy link
Member

rryan commented Jun 26, 2013

Sorry -- misread. Adding logging for when parseHeader() would return ERR is good but maybe that belongs in the SoundSource::process* functions? That way you could report more details about the erorr (TagLib couldn't parse AudioProperties, etc.) And you don't have to duplicate the error checks in every SoundSource's parseHeader.

return OK;
return ERR;
if (result == ERR) {
qWarning() << "Error parsing header of file" << m_qFilename;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is a type mismatch: const int ERR = -1; bool result

@daschuer
Copy link
Member

Please fix also the implicit cast at:
https://github.com/troyane/mixxx/blob/master/src/soundsource.cpp#L271

Improved errors with const int ERR and bool result; avoided implicit
cast in soundsource.cpp. Deleted variable qurlStr.
@daschuer
Copy link
Member

daschuer commented Jul 3, 2013

For me this is in a merge-able state. But RJ has made a valid point:

So it would be nice if you could move all warnings from the bottom of each parseHeader() to
https://github.com/mixxxdj/mixxx/blob/HEAD/src/soundsourceproxy.cpp#L365.
If this is not the only place where parseHeader(), then on the other places as well.

By the way: I think setting p->setHeaderParsed(false); looks like a bug, because the Header was actual passed but without success. So it makes no scene to try it again later.

@troyane
Copy link
Contributor Author

troyane commented Jul 3, 2013

@daschuer, thanks. I'll do it.

@troyane
Copy link
Contributor Author

troyane commented Jul 3, 2013

Please, check it, troyane@9f710b4.

@daschuer
Copy link
Member

daschuer commented Jul 3, 2013

@troyane: Thank you!

daschuer added a commit that referenced this pull request Jul 3, 2013
First part of Bug 1000784 "Specify file name on scanning error"
@daschuer daschuer merged commit c8f4af9 into mixxxdj:master Jul 3, 2013
@troyane troyane deleted the bug_1000784 branch July 3, 2013 09:29
@daschuer daschuer mentioned this pull request Dec 28, 2014
ronso0 referenced this pull request in ronso0/mixxx Jan 29, 2018
 shadow position feature fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants