Bug 254580 - Last.fm: submit composer as artist if available
Summary: Last.fm: submit composer as artist if available
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: Services/Last.fm (show other bugs)
Version: 2.4-GIT
Platform: Debian unstable Linux
: HI major
Target Milestone: 2.4.0
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-18 21:10 UTC by Nicholas Wilson
Modified: 2011-07-24 12:51 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.4.2


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nicholas Wilson 2010-10-18 21:10:19 UTC
Version:           2.4-GIT (using KDE 4.4.2) 
OS:                Linux

When scrobbling, if the track has a composer tag set, the desired behaviour is almost certainly to submit the name of the composer as the "artist".

/ src / services / lastfm / ScrobblerAdapter.cpp
 135         m_current.setArtist( track->artist()->name() );

I suggest

m_current.setArtist ( track->composer()->name() ? track->composer()->name() : track->artist()->name() );

or something similar. I will make and test a real patch if you will take it.

Reproducible: Always
Comment 1 Myriam Schweingruber 2010-10-18 22:02:39 UTC
Please submit your patch at http://git.reviewboard.kde.org (register at http:77identity.kde.org first)
Comment 2 Nicholas Wilson 2010-10-19 20:46:09 UTC
Done.

http://git.reviewboard.kde.org/r/100089/
Comment 3 Leo Franchi 2010-10-20 04:41:38 UTC
commit c5b16718cc86474afb11a7de6e7da99c0f107f07
branch master
Author: Leo Franchi <lfranchi@kde.org>
Date:   Tue Oct 19 22:41:58 2010 -0400

    Patch to submit Composer as Artist for last.fm scrobbles when there is no artist but only composer field. Thanks to Nicholas Wilson for the patch!
    
    BUG: 254580

diff --git a/src/services/lastfm/ScrobblerAdapter.cpp b/src/services/lastfm/ScrobblerAdapter.cpp
index 47df3e3..8a546b6 100644
--- a/src/services/lastfm/ScrobblerAdapter.cpp
+++ b/src/services/lastfm/ScrobblerAdapter.cpp
@@ -77,8 +77,9 @@ ScrobblerAdapter::engineNewTrackPlaying()
         
         m_current.setTitle( track->name() );
         m_current.setDuration( track->length() / 1000 );
-        if( track->artist() )
-            m_current.setArtist( track->artist()->name() );
+        if( track->artist() || track->composer() )
+            m_current.setArtist(
+                track->composer() ? track->composer()->name() : track->artist()->name() );
         if( track->album() )
             m_current.setAlbum( track->album()->name() );
 
@@ -122,7 +123,7 @@ ScrobblerAdapter::engineNewMetaData( const QHash<qint64, QString> &newMetaData,
     Meta::TrackPtr track = The::engineController()->currentTrack();
     if( track &&
         ( track->type() == "stream" && ( !track->name().isEmpty() 
-          && track->artist() ) ) ) // got a stream, and it has enough info to be a new track
+          && ( track->artist() || track->composer() ) ) ) ) // got a stream, and it has enough info to be a new track
     {
         // don't use checkScrobble as we don't need to check timestamps, it is a stream
         debug() << "scrobble: " << m_current.artist() << " - " << m_current.album() << " - " << m_current.title();
@@ -132,7 +133,7 @@ ScrobblerAdapter::engineNewMetaData( const QHash<qint64, QString> &newMetaData,
         resetVariables();
                     
         m_current.setTitle( track->name() );
-        m_current.setArtist( track->artist()->name() );
+        m_current.setArtist( track->composer() ? track->composer()->name() : track->artist()->name() );
         m_current.stamp();
         
         m_current.setSource( lastfm::Track::NonPersonalisedBroadcast );
@@ -210,8 +211,8 @@ ScrobblerAdapter::loveTrack( Meta::TrackPtr track ) // slot
     {
         lastfm::MutableTrack trackInfo;
         trackInfo.setTitle( track->name() );
-        if( track->artist() )
-            trackInfo.setArtist( track->artist()->name() );
+        if( track->artist() || track->composer() )
+            trackInfo.setArtist( track->composer() ? track->composer()->name() : track->artist()->name() );
         if( track->album() )
             trackInfo.setAlbum( track->album()->name() );
Comment 4 robert marshall 2010-10-21 16:18:24 UTC
From the patch, it looks to me that it scrobbles the composer as artist if a composer is set (whether or not the artist is empty). I'd prefer that if it is this way round - rather than that in the Leo Franchi comment to the patch - to a user settable option. If I have 5 versions of Brahms' Symphony No 4 I'd prefer the artist to be the conductor - if one is set.
Also (probably should be another bug entry) - in this case the opposite to the above if the composer is set then the Lyrics search ought always to use that field
Comment 5 Nicholas Wilson 2010-10-21 16:56:03 UTC
True. I am not sure about making this a user option on the basis of the tiny number of people who want that much control.

In my mind, the main point is that the data sent to last.fm has to be enough to identify the piece, so at least the composer should be sent it it were available. I have nine 'Symphony 5's in my collection, so sending just the album and conductor is not a good option. Somehow you want a way to shoe-horn two bits of information into one field.

Currently, I tag my collection to duplicate the main performer in the album field when I have two copies of the same piece, so I have:

Beethoven, Ludwig van
    Violin Concerto in D Major, op. 61 (Mutter)
    Violin Concerto in D Major, op. 61 (Schneiderhan)
    Violin Sonata No. 1 in D Major op. 12 No. 1 [no need to distinguish recordings]

Given that the conductor has to be sent, how do you want the performers' data to be sent? We would have to munge two fields into one.

Sending artist as "<composer> (<artist>)" is probably not right, because works by one composer do not get grouped together.
Sending album as "<work> (<performers>)" seems to me to be preferable, but then I would want a pref to turn it off for myself, given the way I have tagged my collection.

Any comment?
Comment 6 robert marshall 2010-10-21 20:07:21 UTC
Tricky isn't it! Maybe we should petition last.fm to support the composer field!
Unfortunately I've gone the opposite way and have the title as "[composer] work/movement name" so I don't think we're going to get a common view here.
Comment 7 Matt Howe 2010-10-23 03:04:28 UTC
Woah! This is really bad! Composer being submitted as Artist? Scrobbling a "Pearl Jam" track with proper tags now scrobbles as "Eddie Vedder/Mike McCready/Stone Gossard/Dave Abbruzzese/Jeff Ament - [track_name]". Please, please revert this.
Comment 8 Myriam Schweingruber 2010-10-23 14:29:25 UTC
Reopened, as it seems to submit the composer field despite the artist field being set.
Comment 9 Myriam Schweingruber 2010-10-23 14:31:34 UTC
Changed priority
Comment 10 Rick W. Chen 2010-10-23 20:10:37 UTC
The patch is great for classical music and the like. However, as Comment #7 pointed out, it doesn't work well for tracks that are composed by specific members in a band, which is often the case. This depends too much on the collection and user preference; not everyone would want composer to be submitted. At least with the artist it has been the convention and that's what most people would expect to be used. If adding a config interface is not an option, I propose a revert.
Comment 11 Leo Franchi 2010-10-25 04:37:06 UTC
Comment #7 brings up a good point. I don't mind reverting either, as this patch never affected me either way :)
Comment 12 Rick W. Chen 2010-10-25 07:04:58 UTC
Anyone else has issue with reverting?
Comment 13 robert marshall 2010-10-25 16:23:10 UTC
If it did what the checkin comment suggested - use composer only if artist is not set (so wants some !track->artist())  then - although not perfect for classical users- would be of some use. Otherwise if a user settable option isn't possible then IMHO drop it
Comment 14 Rick W. Chen 2010-10-25 23:25:10 UTC
It doesn't actually (commit c5b16718cc86474afb11a7de6e7da99c0f107f07).
Composer is used no matter what if it exists.
Comment 15 robert marshall 2010-10-26 14:43:48 UTC
That was my point, I think the setArtist calls should read
if( track->artist() 
            m_current.setArtist(
                track->artist() );
else if(track->composer() )
            m_current.setArtist(
                track->composer()->name() );
but, for preference, a user settable option to determine which the usage priority if it exists
Comment 16 Myriam Schweingruber 2010-10-26 16:06:17 UTC
I would very much like to make this optional, not all use cases are the same, and I bet quite a few people would not like to scrobble that way.
BTW, it still doesn't work correctly, could we please revert this until it is really tested?
Comment 17 Nicholas Wilson 2011-03-06 01:36:40 UTC
OK, so, it's been a while. I got back to this today and made a bigger patch adding a config setting for this. As far as I can tell, people either have important composer tags, which need to be scrobbled to identify the piece (like in my classical collection), or do not want them scrobbled. The config option I added toggles between the old behaviour and the new. The people who will fall down the gaps are those with mixed collections of popular and classical music. I can't a way around that problem, really; you can't test against the genre, because it might be "Classical" or "Baroque" or "Classíca" or "Classique" or any number of variations.

Please comment. Thanks for help!
https://git.reviewboard.kde.org/r/100806/
Comment 18 Sam Lade 2011-07-24 12:49:27 UTC
This was committed a while ago as cd8f7865fd93bfcb4612c3f19124b47f0298142f.