Bug 29 - allow CDDB pushing for new discs
Summary: allow CDDB pushing for new discs
Status: CONFIRMED
Alias: None
Product: Asunder
Classification: Unclassified
Component: All (show other bugs)
Version: 2.5
Hardware: PC Linux
: enhancement
Assignee: Andrew Smith
URL:
Depends on:
Blocks:
 
Reported: 2016-05-08 02:22 EDT by manuk7
Modified: 2021-05-30 06:48 EDT (History)
2 users (show)

See Also:


Attachments
patchzes for the new CDDB upload feature (26.17 KB, patch)
2021-05-23 19:00 EDT, Andreas Hünnebeck
Details | Diff

Description manuk7 2016-05-08 02:22:49 EDT
Hello,
when a disc is unknown in the CDDB, I have to fill in manually the datas (artists, track titles ...)in asunder.
It could be great to be able to push these datas to the CDDB so that others could benefit from them.
Do you think this is doable ?
thanks
Comment 1 Andrew Smith 2021-05-22 19:56:35 EDT
I just commited a patch from Andreas Hünnebeck that allows Asunder to do CDDB uploads (r454).

Here are my notes:

What is the purpose of the WRITE_APPEND macro? Can't everything from its body go into the for loop?

Why declare functions as "extern"? I removed the keyword.

I changed the default cddb_email_address to an empty string instead of null, for consistency.
p->cddb_email_address shouldn't be set for "unused". That implies that preference is not used by Asunder any more and it's only kept in the config file for compatibility reasons.

p->cddb_email_address needs to be last in the config file, for compatibility.

Need to make Category row and "Upload to CDDB" button grey out on cddb lookup. I also just noticed that the first track number UI elements have the same problem.

Should have another checkbox in the advanced preferences just above e-mail to "Enable CDDB upload". That would show/hide the "Upload to CDDB" button and the "Category" label/dropdown row.

Upload button needs to check whether the email address is set.

I tried a test upload and got "CDDB upload failed: posted data rejected" for this:

>>>
Disc ID : a0124e0b
Revision: 37
Artist  : Led Zeppelin
Title   : Latter Days: The Best Of Led Zeppelin, Volume 2
Category: country
Genre   : Rock
Year    : 2000
Track  1: Led Zeppelin - The Song Remains The Same
Track  2: Led Zeppelin - No Quarter
Track  3: Led Zeppelin - Houses Of The Holy
Track  4: Led Zeppelin - Trampled Under Foot
Track  5: Led Zeppelin - Kashmir
Track  6: Led Zeppelin - Ten Years Gone
Track  7: Led Zeppelin - Achilles' Last Stand
Track  8: Led Zeppelin - Nobody's Fault But Mine
Track  9: Led Zeppelin - All My Love
Track 10: Led Zeppelin - In The Evening
Track 11: Led Zeppelin - DATA
<<<

Does that mean anything to you?
Comment 2 Andreas Hünnebeck 2021-05-23 08:04:45 EDT
(In reply to Andrew Smith from comment #1)
> I just commited a patch from Andreas Hünnebeck that allows Asunder to do
> CDDB uploads (r454).
> 
> Here are my notes:
> 
> What is the purpose of the WRITE_APPEND macro? Can't everything from its
> body go into the for loop?

The purpose of the WRITE_APPEND macro in upload.c is to prevent overwriting the character buffer 'buf'. Since the variable 'remaining' us updated for each call it must be used for all writes into 'buf'.

> Why declare functions as "extern"? I removed the keyword.

Regarding the 'extern' keyword in header files: Me and my colleagues in our software department always did this. It's probably for historic reasons when some 30 years ago we switched from Kernighan&Ritchie C-compiler (which did not support forward declarations to function calls) to Ansi-C (where people added forward declared functions in the .c files as 'extern' and later moved these declarations into header files). I used plain old C very seldom in the last 20 years after switching to C++.  Fine with me to remove the 'extern' keywords if not required.

> I changed the default cddb_email_address to an empty string instead of null,
> for consistency.

Fine with me but the change broke two if-conditions (check against NULL) in on_upload_button_clicked() (where the check you want to have was already implemented) and get_cddb_connection(). I will add a patch later.

> p->cddb_email_address shouldn't be set for "unused". That implies that
> preference is not used by Asunder any more and it's only kept in the config
> file for compatibility reasons.
> 
> p->cddb_email_address needs to be last in the config file, for compatibility.
> 
> Need to make Category row and "Upload to CDDB" button grey out on cddb
> lookup. I also just noticed that the first track number UI elements have the
> same problem.

OK, will do.

> Should have another checkbox in the advanced preferences just above e-mail
> to "Enable CDDB upload". That would show/hide the "Upload to CDDB" button
> and the "Category" label/dropdown row.

What about hiding "Upload to CDDB" and "Category" if no email address is set?

> Upload button needs to check whether the email address is set.

See above, patch will come.

> I tried a test upload and got "CDDB upload failed: posted data rejected" for
> this:
> 
> >>>
> Disc ID : a0124e0b
> Revision: 37
> Artist  : Led Zeppelin
> Title   : Latter Days: The Best Of Led Zeppelin, Volume 2
> Category: country
> Genre   : Rock
> Year    : 2000
> Track  1: Led Zeppelin - The Song Remains The Same
> Track  2: Led Zeppelin - No Quarter
> Track  3: Led Zeppelin - Houses Of The Holy
> Track  4: Led Zeppelin - Trampled Under Foot
> Track  5: Led Zeppelin - Kashmir
> Track  6: Led Zeppelin - Ten Years Gone
> Track  7: Led Zeppelin - Achilles' Last Stand
> Track  8: Led Zeppelin - Nobody's Fault But Mine
> Track  9: Led Zeppelin - All My Love
> Track 10: Led Zeppelin - In The Evening
> Track 11: Led Zeppelin - DATA
> <<<
> 
> Does that mean anything to you?

Nope, never had this. The entry looks fine for me.

Enable test mode in upload.c and retry. If you receive a mail that this would be ok, you may contact gnudb.org and ask for advice (you know the email address from our private conversation per email).
Comment 3 Andrew Smith 2021-05-23 10:38:02 EDT
(In reply to Andreas Hünnebeck from comment #2)

Great, thanks!

> What about hiding "Upload to CDDB" and "Category" if no email address is set?

I feel it will be much more clear for the user what's going on if there's a checkbox.

> Nope, never had this. The entry looks fine for me.
> 
> Enable test mode in upload.c and retry. If you receive a mail that this
> would be ok, you may contact gnudb.org and ask for advice (you know the
> email address from our private conversation per email).

That was in test mode. I'll ask Rene if I can't think of any other reason.
Comment 4 Andreas Hünnebeck 2021-05-23 18:57:07 EDT
(In reply to Andrew Smith from comment #3)
> (In reply to Andreas Hünnebeck from comment #2)
> > What about hiding "Upload to CDDB" and "Category" if no email address is set?
> 
> I feel it will be much more clear for the user what's going on if there's a
> checkbox.

OK, done. I'll attach the patch file 'patch-cddb-upload3.patch' which contains all desired features:
* added 'Enable CDDB upload' button in the Preferences/Advanced dialog
* unchecking the 'Enable CDDB upload' button disables the email text field in the 
Preferences/Advanced dialog and hides the 'CDDB upload' button in the main window
* fixed this bug: If you insert a disc which is not known in CDDB and you click the upload button without doing any change there was no error/warning that you did not change anything.

I also moved code which handles the upload feature from prefs.c and interface.c I had added earlier into upload.c, so the original code is mostly unchanged and allmost all code which handles the upload feature is located in upload.c.
Comment 5 Andreas Hünnebeck 2021-05-23 19:00:32 EDT
Created attachment 45 [details]
patchzes for the new CDDB upload feature

* added 'Enable CDDB upload' button in the Preferences/Advanced dialog
* unchecking the 'Enable CDDB upload' button disables the email text field in the Preferences/Advanced dialog and hides the 'CDDB upload' button in the main window
* fixed this bug: If you insert a disc which is not known in CDDB and you click the upload button without doing any change there was no error/warning that you did not change anything.
* moved code which handles the upload feature from prefs.c and interface.c into upload.c, so the original code is mostly unchanged and almost all code which handles the upload feature is located in upload.c.

Note You need to log in before you can comment on or make changes to this bug.