[prev in list] [next in list] [prev in thread] [next in thread]
List: amarok-devel
Subject: Re: Review Request: MusicBrainz and MusicDNS services support
From: "Sergey Ivanov" <123kash () gmail ! com>
Date: 2010-10-02 17:39:51
Message-ID: 20101002173951.1380.89456 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On None, Sergey Ivanov wrote:
> > Looks works for me.
>
> Leo Franchi wrote:
> Doesn't compile when KDE4_BUILD_TESTS is enabled:
>
>
> [ 54%] Building CXX object \
> tests/core-impl/collections/proxycollection/CMakeFiles/testproxycollectionmeta.dir/TestProxyCollectionMeta.o
>
> /home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp: \
> In member function ‘void \
> TestProxyCollectionMeta::testEditableCapabilityOnMultipleTracks()': \
>
> /home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp:463:51: \
> error: cannot allocate an object of abstract type \
> ‘MyEditCapability'
> /home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp:433:1: \
> note: because the following virtual functions are pure within \
> ‘MyEditCapability':
> /home/leo/kde/src/amarok/src/core/capabilities/EditCapability.h:57:26: \
> note: virtual void Capabilities::EditCapability::setUid(const \
> QString&, const QString&)
> /home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp:464:51: \
> error: cannot allocate an object of abstract type \
> ‘MyEditCapability'
> /home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp:433:1: \
> note: since type ‘MyEditCapability' has pure virtual functions \
> make[2]: *** [tests/core-impl/collections/proxycollection/CMakeFiles/testproxycollectionmeta.dir/TestProxyCollectionMeta.o] \
> Error 1 make[1]: *** \
> [tests/core-impl/collections/proxycollection/CMakeFiles/testproxycollectionmeta.dir/all] \
> Error 2
>
> Leo Franchi wrote:
> I have some doubts about the mass tagging UI. When I first open the \
> MusicBrainz tagger with multiple files selected, I see this:
> http://imagebin.ca/view/e25y1kwP.html'
>
> I can select files in the left pane, and that populates the fields below. \
> It's not clear if the "Start Searching..." button operates on the \
> currently selected track, or all the tracks. I learned it operates on all \
> the tracks, but I didn't know until I tried it. The text should clarify \
> this. Then I see this:
> http://imagebin.ca/view/iKtHSXE.html'
>
> Now I am just confused. What happened to the tracks on the left? What are \
> the entries on the right? None of them are track names, they are actually \
> album names. It's not good to h ave the left listview be tracks and the \
> right listview be albums without any sort of help for the user to figure \
> that out. Anyway, as a new user i'm curious as to what these entries are, \
> so I expand one or two:
> http://imagebin.ca/view/jrUBpR3K.html'
>
> And so I am extrapolating here, it seems like it is picking albums and \
> bolding the names of tracks in the album that matches one of the tracks I \
> had on the left---but has now disappeared. What are all the non-bold \
> track names for? They are not related to the original tracks in the left \
> hand pane. I can't have an overview of what this dialog is actually going \
> to do.
> I'm not sure at this point if "Save" operates on the currently selected \
> tracks, or if it saves all the tracks (what tracks? the ones that are \
> bold? all of them? it is confusing).
> Also, I am unable to use the MusicDNS finder. No matter what, it never \
> find anything. Here is my debug output:
>
> amarok: BEGIN: MusicDNSFinder::MusicDNSFinder(const \
> Meta::TrackList&, QObject*, const QString&, int, const QString&, const \
> QString&, const QString&)
> amarok: [MusicDNSFinder] Initiating MusicDNS search:
> amarok: [MusicDNSFinder] Host: \
> "ofa.musicdns.org"
> amarok: [MusicDNSFinder] Port: 80
> amarok: [MusicDNSFinder] Path Prefix: "/ofa/1"
> amarok: [MusicDNSFinder] Client ID: \
> "0c6019606b1d8a54d0985e448f3603ca"
> amarok: [MusicDNSFinder] Client version: "2.3-GIT"
> amarok: BEGIN: MusicDNSAudioDecoder::MusicDNSAudioDecoder(const \
> Meta::TrackList&, int)
> amarok: END__: MusicDNSAudioDecoder::MusicDNSAudioDecoder(const \
> Meta::TrackList&, int) - Took 0.085s
> amarok: END__: MusicDNSFinder::MusicDNSFinder(const \
> Meta::TrackList&, QObject*, const QString&, int, const QString&, const \
> QString&, const QString&) - Took 0.086s
> amarok: END__: void MusicBrainzTagger::searchDone() - DELAY Took \
> (quite long) 10s
> amarok: END__: void MusicBrainzFinder::sendNewRequest() - DELAY Took \
> (quite long) 10s
> amarok: BEGIN: void MusicDNSFinder::decodingDone(ThreadWeaver::Job*)
> amarok: END__: void MusicDNSFinder::decodingDone(ThreadWeaver::Job*) - \
> Took 0.00011s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: [MusicDNSFinder] There is no any queued requests. Stopping \
> timer.
> amarok: BEGIN: void MusicBrainzTagger::mdnsSearchDone()
> amarok: END__: void MusicBrainzTagger::mdnsSearchDone() - Took \
> 6.9e-05s
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00026s
> amarok: BEGIN: void MusicBrainzFinder::sendNewRequest()
> amarok: [MusicBrainzFinder] There is no any queued requests. Stopping \
> timer.
> amarok: BEGIN: void MusicBrainzTagger::searchDone()
> amarok: END__: void MusicBrainzTagger::searchDone() - Took 8.5e-05s
> amarok: END__: void MusicBrainzFinder::sendNewRequest() - Took 0.00029s \
>
> am I doing something wrong?
>
> Sergey Ivanov wrote:
> This UI made to be like on Picard (original musicbrainz tagger). On a \
> left side we have tracks to search, on a right side - search result. When \
> a track found we move it from left side to the right one. On the right \
> table complete album's track list is shown. Albums, that contains a track \
> has a bold text, the same to tracks - all tracks in a album linked to \
> real tracks has a bold text. BTW all tracks are drag-able. So if don't \
> wanna save search result for some tracks, you can drop this tracks to \
> left panel. Could you send one of this track on 123kash@gmail.com, i \
> didn't have such troubles with my "test samples".
> Leo Franchi wrote:
> Sorry for the delay. Work has been keeping me busy.
>
> I've never used Picard, so that's why I was confused. But I think this \
> points to a valid criticism---the use of Picard is not necessary to the \
> use of Amarok. Users should not be expected to be familiar with the user \
> interface of another program to feel comfortable with Amarok's user \
> interface. For example, I had no idea i could drag tracks---there are no \
> visual indicators that dragging is possible, nor do I know what dragging \
> would do exactly (why is dragging right->left "choosing not to save \
> results"?). My point with this is that I think we need a better \
> explanation than "it is how Picard does it"---we want Amarok to be as \
> intuitive and user-friendly as possible, regardless of how good or bad \
> other programs are :) I think a few things at least need to be done:
> 1) Workflow clarified---what is the user supposed to do? What buttons do \
> what, and how is the user informed of that? 2) Text to explain to the \
> user what to do. Tooltips to explain what buttons do
> Re: files:
>
> What sort of files are supposed to 'work'? I tried some popular bands \
> (the beatles, the who, queen, led zeppelin) that they definitely should \
> have in their records, but got 0 results. I can send you some if you \
> want, or come find me on irc (nick lfranchi, #amarok on Freenode)
> Sergey Ivanov wrote:
> All known by musicdns.org, but It could be some problems due to missed \
> check in CMakeLists, so MusicDNS decomressor/fingerprint-generator should \
> not been compiled at all. Now It fixed, so could you try one more time?
> Leo Franchi wrote:
> Maybe the new ffmpeg stuff doesn't support flac?
>
> [flac @ 0x4cb4380] invalid coding type
> [flac @ 0x4cb4380] decode_frame() failed
>
> Tried also with an MP3 (AC/DC Back in Black, very very common file) and \
> it just seems to loop with a network request:
> amarok: BEGIN: void MusicBrainzTagger::searchDone()
> amarok(18683)/kdeui (KNotification) KNotificationManager::close: 23598
> amarok: BEGIN: MusicDNSFinder::MusicDNSFinder(const Meta::TrackList&, \
> QObject*, const QString&, int, const QString&, const QString&, const \
> QString&)
> amarok: [MusicDNSFinder] Initiating MusicDNS search:
> amarok: [MusicDNSFinder] Host: "ofa.musicdns.org"
> amarok: [MusicDNSFinder] Port: 80
> amarok: [MusicDNSFinder] Path Prefix: "/ofa/1"
> amarok: [MusicDNSFinder] Client ID: \
> "0c6019606b1d8a54d0985e448f3603ca"
> amarok: [MusicDNSFinder] Client version: "2.4-GIT"
> amarok: END__: MusicDNSFinder::MusicDNSFinder(const Meta::TrackList&, \
> QObject*, const QString&, int, const QString&, const QString&, const \
> QString&) - Took 0.00054s
> amarok: END__: void MusicBrainzTagger::searchDone() - Took 0.95s
> amarok: END__: void MusicBrainzFinder::sendNewRequest() - Took 0.95s
> [mp3 @ 0x42d7c90] max_analyze_duration reached
> [mp3 @ 0x42d7c90] Estimating duration from bitrate, this may be \
> inaccurate \
>
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest() \
>
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00013s
> amarok: BEGIN: void MusicDNSFinder::trackDecoded(Meta::TrackPtr, QString) \
>
> amarok: END__: void MusicDNSFinder::trackDecoded(Meta::TrackPtr, QString) \
> - Took 0.00027s
> amarok: BEGIN: void MusicDNSFinder::decodingDone(ThreadWeaver::Job*)
> amarok: END__: void MusicDNSFinder::decodingDone(ThreadWeaver::Job*) - \
> Took 0.00014s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok(18683)/kio (Scheduler) KIO::SchedulerPrivate::doJob: \
> KIO::SimpleJob(0x48b5170)
> amarok: [MusicDNSFinder] Request sent: \
> "http://ofa.musicdns.org:80/ofa/1/track/?gnr=&art=&rmd=0&cid=0c6019606b1d8 \
> a54d0985e448f3603ca&alb=&fmt=&brt=128&cvr=2.4-GIT&fpt=AQ1DLwI4DDxJF9sTSBS/ \
> D+UT6xEzDyQQTgqkFHUSZg+hCpsPhgsgDbcaJBWSEl4TzhFNC5MJHAb8BdYFXAR/BE4DrwPrA0 \
> sDMANiAucClQKS7/m6bdptYZz/SAuOAcD+HwF+Afb/e/zz/rD65fhOAMj/Vf/H/br+7vm0/BH+ \
> y/9D/1kAAP/7AJwBkQKOAnACBwGNAMYBJgGvAb0BdQGmAaMNCzRJII0sIv2C/dX1dv6N9Lr2hP \
> bS8iD5SOkK9cv50fsW8wD27u+jxSXbg9p23+zjtu6M9Vj46/pw/Aj81/4E/jH+J/01/ZX9vv5K \
> /wH+lf1zCLsTkRNe7crrTcyBBLbbcfGJ6ynwa/bbwLIKl+mP9d76/gURCURBgwfrF7YBE/zY/1 \
> L9wwBgAGEAYABpAMMBBADVARIARQBm//UAUgACJ4FATqeHERT2swTnDKn9XfB2/A8D+/W3BxQI \
> GeQaCXoC7PfR+8MA0xnIBwoHPf1JARoAMQB+/9MCCgKbArACBgL1AioBtgIXAgEBngIsAZnzcf \
> KfIKMCt/bN+wkvfPKX8TDmSf686sQCnTFw38cD8fnD7Mn2ZADjO9rlqwGE13DiyvNf9bX7Xvwi \
> /jr+ZgA2/4b//wFp/97/kP9g/sf/ugss/R8RBgZBAA8ERf/g+rTplNpk8NwE3gRhFRj2KO4x6I \
> LWtshtyArxVis/CLQkkxdFDvMFvgNKA8oDcQIZAXD/9gBHAN0AsQIAALsArwDKIDdBRQ==&ttl=&tnm=&lkt=&dur=254000&yrr=" \
>
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00059s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00011s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.9e-05s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00013s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00011s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.6e-05s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.8e-05s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9e-05s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.6e-05s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.7e-05s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.9e-05s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00013s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00011s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.9e-05s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00014s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00011s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00012s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00012s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00013s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.5e-05s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
> amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
> amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
>
>
> Sergey Ivanov wrote:
> Trouble with flac fixed.
> There is no any loops with request, sendNewRequest runs once a second by \
> timer. That (sendNewRequest() spam) happened due to some network lag, \
> 'cos there was no reply from server, or error report, and if you follow \
> the link from log, you'll find track PUID in XML wrap. Now timer stops \
> when there is no more requests to send.
> Sergey Ivanov wrote:
> And so? :) I filled up all hints, to make it's behavior clear for end \
> user, and fixed all found bugs (didn't I?). What should I do now? :)
> Leo Franchi wrote:
> Thanks for the work. I apologize that I am not able to reply to your \
> updates and test faster. Unfortunately I have a day job :P
> I just tested again the musicdns stuff, and worked.
>
> I appreciate your improvements to the UI. However, you have not addressed \
> some of my main concerns regarding the UI, mostly regarding the workflow \
> itself. Tooltips are good, but do not substitute for an easy to use \
> interface. I have also asked Thomas Pfeiffer, our resident usability \
> professional, for some more formal comments. He will comment as well as \
> soon as he has a chance, and I value his opinion as he actually knows \
> what he is talking about when he analyzes interfaces :)
> Thomas Pfeiffer wrote:
> I finally found the time to have closer look at the problem and post some \
> comments. Sorry for letting you wait so long. My personal life got in the \
> way.
> Let's have a look at this from a task/user perspective. First question: \
> What is the task the user wants to complete with this feature? In my \
> optinion it is the following: The user has some tracks with \
> missing/incomplete/incorrect tags. The user wants to find the correct \
> tags using musicbrainz. So what would the logical steps for this task be?
> 1. Select the tracks to update the tags for.
> 2. Send information on the tracks to musicbrainz.
> 3. Receive suggestions for tags from musicbrainz.
> 4. Select the tracks for which the correct tags have been found.
> 5. Update the tags on the files.
>
> So the user selects the tracks to be updated and activates the mass \
> tagging feature. The user is presented with a dialog with two list boxes, \
> two forms for tags and several buttons. The Save button doesn't make any \
> sense at this point since there is nothing to save. The user clicks \
> "Start searching". Several album and track names appear in the right \
> list and the left list is empty. Some tracks are bold. So the user is \
> presented with a lot of information she does not actually care about (the \
> other tracks on those albums for which she has not even searched) in an \
> unexpected place (not visually related to the original tracks that were \
> in the left box). What the user expects at this point is a clear \
> connection between the tracks to be tagged and the tags musicbrainz \
> suggests for them. What the user has: Tracks without correct tags. What \
> the user wants: Correct tags for these tracks. Nothing else.
> The next thing the user wants to do is to decide for each track if the \
> tags suggested for it are correct or not. The choice should not be "Do I \
> want this track to be updated with musicbrainz tags or not?" but "Are the \
> suggested tags for this track correct or not?"
> So I'd suggest the following:
> Use the same columns in the left as in the right field.
> Leave the selected tracks in the left box.
> Remove all the tag fields below the lists. We don't need them for this \
> task. In the right box, display the suggested tags for each track \
> side-by-side to the track on the left. Place a checkbox to the left of \
> each track. An "Update tags" button below the checkboxes should update \
> the tags of all checked tracks. If you want to use drag&drop as an \
> alternative, allow dragging of the tags on the right onto the track on \
> the left. This would instantly update the tags and show them on the left. \
>
> So, long story short, the essence is: Give the user exactly the \
> information she needs to make the following informed decision: "Which of \
> the tracks I searched for can be updated using musicbrainz because it has \
> found the correct tags?" No more, no less. And in the most direct manner \
> possible. The above suggestion is only one possible solution and I could \
> agree with others as long as they keep this essence.
> Regards,
> Thomas
>
> Sergey Ivanov wrote:
> Thank you very much, for you comment. Rebuild UI according to you scheme \
> (i hope so). Now I'm trying to find best way to make user know what's \
> going on (status bar, or splash screen during search process), and some \
> way to show that tracks were saved.
> Leo Franchi wrote:
> Great, it compiles and works! And awesome that it uses ffmpeg instead of \
> xine! It's already much much easier to use for someone unfamiliar with \
> Picard like me :)
> Here are my last few comments, I think after these we're all good to go! \
> We'll commit the fix after the patch is updated.
> 1) No progress info. The MusicDNS lookup took ~2 min here, but I had no \
> feedback that it was going on. A progress bar would be awesome, just so \
> the user doesn't get bored and close the dialog. Maybe between the start \
> and update buttons? I don't know, whatever looks good :) 2) When songs \
> have no tags, it's impossible to see them in the list: \
> http://imagebin.ca/view/4xMgfa.html' . Maybe when there are no tags, it \
> could display the filename instead? That way the user is actually able to \
> select th em :) 3) What do you think about auto-starting the search task \
> when the dialog is opened? I don't know if this is a good idea, but I \
> assume the user might want see what is available as soon as possible, \
> without having to press Search (after all, he already chose the "use \
> musicbrainz" button in the tag dialog). up for discussion, but just an \
> idea :) 4) I think there should be a "cancel" button next to the OK \
> button, and the OK button should also update the tags. I pressed OK a few \
> times thinking it would "save&close" but instead it closed without \
> saving. having a cancel button will make it clear how to exit with and \
> without saving the checked songs.
> thanks for all your work! i'm really excited to have this feature at long \
> last be a part of amarok.
> Sergey Ivanov wrote:
> 1) I'm working on It.
> 2) I's strange, 'cos in my case there is file name. \
> http://imagebin.ca/view/VRSsQnZw.html. I'll add check for It. 3) I'll try \
> to make one trick for this stuff. 4) When the user press "Update tags", \
> all changes have immediately sent to TagDialog. Probably It would be \
> better to remove this button at all, and give this job to Ok(Save&Close \
> whatever) button? In this case we definitely have to have Cancel or Close \
> button.
> Mark Kretschmann wrote:
> Sergey, I think your point 4) is an excellent idea! The button seems to \
> be duplicated functionality indeed, as you have to press Save/Cancel in \
> the TagDialog anyway.
>
> Thomas Pfeiffer wrote:
> It's great that you implemented the suggestions so quickly. That's what \
> makes F/OSS usbaility work fun ;) I agree that having one button to \
> update the tags and close the dialog and one cancel button would be best. \
> I'd prefer "Update tags" as the label for the first button, I think it \
> fits pretty well. And we don't need to add "& close" to the label. Users \
> expect the dialog to close after updating the tags anyway, since its work \
> is done then. And I agree with Leo that starting the search instantly \
> really makes sense. If you can achieve that, we have a dialog that \
> contains exactly the elements necessary for the task :)
> Just two more comments:
> 1. I currently don't compile Amarok from git, so I only have the \
> screenshots above, therefore I don't know: Are there checkboxes when \
> search results are diplayed or is drag&drop the only way to select which \
> tracks should be updated? That might be problematic since D&D is not \
> easily discoverable for all users. 2. Could you add labels above the two \
> list boxes? My suggestion: "Current tags" for the left one and "Tags \
> suggested by MusicBrainz" for the right one.
> Thank you for the great work,
> Thomas
I removed D&D ability. It doesn't make sense since we have check boxes and \
don't have complete album track list.
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100000/#review4
-----------------------------------------------------------
On 2010-10-02 17:32:14, Sergey Ivanov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100000/
> -----------------------------------------------------------
>
> (Updated 2010-10-02 17:32:14)
>
>
> Review request for amarok.
>
>
> Summary
> -------
>
> MusicBrainz and MusicDNS services support implementation.
> There are three independent parts:
> 1. MusicBrainzFinder class - used to make requests to MusicBrainz server \
> and process replies. All relies process in separate threads by \
> MusicBrainzXmlParser class. For a search uses guessed from a file name \
> track information. No external dependences required. 2. MusicDNSFinder \
> class - used for the same purpose as MusicBrainzFinder, but i communicate \
> with musicdns server and receives track's PUID as a reply. Replies ether \
> process in separate threads by MusicDNSXmlParser class. Fingerprints \
> generated by libofa (the only external dependence in entire patch). For \
> track decompressing (MusicDNSAudioDecoder class) used xine engine (I'm \
> not sure is It a good choice, but amarok based on phonon media-engine, \
> that uses xine. So we don't deed to pull any other dependences). Received \
> PUIDs sends to MusicBrainzFinder class, for a search routine. 3. View. \
> All classes used for store (MusicBrainzTagsModel, \
> MusicBrainzTrackListModel) and display (MusicBrainzTagsModelDelegate) \
> purposes.
>
> Diffs
> -----
>
> CMakeLists.txt 2a0961c
> cmake/modules/FindFFmpeg.cmake PRE-CREATION
> cmake/modules/FindLibOFA.cmake PRE-CREATION
> config-amarok.h.cmake 981b7b7
> src/CMakeLists.txt ac5db9f
> src/core-impl/capabilities/timecode/TimecodeEditCapability.h 6e15303
> src/core-impl/capabilities/timecode/TimecodeEditCapability.cpp 8205d45
> src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h e35b57f \
> src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp \
> 9be62d9 src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp \
> 8bdbf75 src/core-impl/collections/sqlcollection/CapabilityDelegateImpl.cpp \
> b5cb083 src/core-impl/collections/sqlcollection/SqlMeta.h ee3ec21
> src/core-impl/collections/sqlcollection/SqlMeta.cpp 2da0333
> src/core-impl/meta/file/File.h 6d4d395
> src/core-impl/meta/file/File.cpp 30cd2ff
> src/core-impl/meta/file/TagLibUtils.cpp 15b64a4
> src/core-impl/meta/proxy/MetaProxy.h 2ef3805
> src/core-impl/meta/proxy/MetaProxy.cpp 341e076
> src/core/capabilities/EditCapability.h 79344bd
> src/dialogs/MusicBrainzTagger.h PRE-CREATION
> src/dialogs/MusicBrainzTagger.cpp PRE-CREATION
> src/dialogs/MusicBrainzTagger.ui PRE-CREATION
> src/dialogs/TagDialog.h 50cd801
> src/dialogs/TagDialog.cpp 26d4eb8
> src/dialogs/TagDialogBase.ui 9974d0b
> src/musicbrainz/MusicBrainzFinder.h PRE-CREATION
> src/musicbrainz/MusicBrainzFinder.cpp PRE-CREATION
> src/musicbrainz/MusicBrainzMetaClasses.h PRE-CREATION
> src/musicbrainz/MusicBrainzMetaClasses.cpp PRE-CREATION
> src/musicbrainz/MusicBrainzTagsModel.h PRE-CREATION
> src/musicbrainz/MusicBrainzTagsModel.cpp PRE-CREATION
> src/musicbrainz/MusicBrainzTrackListModel.h PRE-CREATION
> src/musicbrainz/MusicBrainzTrackListModel.cpp PRE-CREATION
> src/musicbrainz/MusicBrainzXmlParser.h PRE-CREATION
> src/musicbrainz/MusicBrainzXmlParser.cpp PRE-CREATION
> src/musicbrainz/MusicDNSAudioDecoder.h PRE-CREATION
> src/musicbrainz/MusicDNSAudioDecoder.cpp PRE-CREATION
> src/musicbrainz/MusicDNSFinder.h PRE-CREATION
> src/musicbrainz/MusicDNSFinder.cpp PRE-CREATION
> src/musicbrainz/MusicDNSXmlParser.h PRE-CREATION
> src/musicbrainz/MusicDNSXmlParser.cpp PRE-CREATION
> src/widgets/CheckButton.h PRE-CREATION
> src/widgets/CheckButton.cpp PRE-CREATION
> tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp \
> 55d1914
> Diff: http://git.reviewboard.kde.org/r/100000/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px \
#c9c399 solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/100000/">http://git.reviewboard.kde.org/r/100000/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <p style="margin-top: 0;">On , <b>Sergey Ivanov</b> \
wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; \
white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">Looks works for me.</pre> \
</blockquote>
<p>On September 22nd, 2010, 2:27 a.m., <b>Leo Franchi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Doesn't compile when KDE4_BUILD_TESTS is enabled:
[ 54%] Building CXX object \
tests/core-impl/collections/proxycollection/CMakeFiles/testproxycollectionmeta.dir/TestProxyCollectionMeta.o
/home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp: \
In member function ‘void \
TestProxyCollectionMeta::testEditableCapabilityOnMultipleTracks()': \
/home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp:463:51: \
error: cannot allocate an object of abstract type \
‘MyEditCapability'
/home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp:433:1: \
note: because the following virtual functions are pure within \
‘MyEditCapability':
/home/leo/kde/src/amarok/src/core/capabilities/EditCapability.h:57:26: \
note: virtual void Capabilities::EditCapability::setUid(const \
QString&, const QString&)
/home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp:464:51: \
error: cannot allocate an object of abstract type \
‘MyEditCapability'
/home/leo/kde/src/amarok/tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp:433:1: \
note: since type ‘MyEditCapability' has pure virtual functions make[2]: \
*** [tests/core-impl/collections/proxycollection/CMakeFiles/testproxycollectionmeta.dir/TestProxyCollectionMeta.o] \
Error 1 make[1]: *** \
[tests/core-impl/collections/proxycollection/CMakeFiles/testproxycollectionmeta.dir/all] \
Error 2 </pre>
</blockquote>
<p>On September 22nd, 2010, 2:44 a.m., <b>Leo Franchi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">I have some doubts about the mass tagging UI. When I first \
open the MusicBrainz tagger with multiple files selected, I see this:
http://imagebin.ca/view/e25y1kwP.html'
I can select files in the left pane, and that populates the fields below. \
It's not clear if the "Start Searching..." button operates on \
the currently selected track, or all the tracks. I learned it operates on \
all the tracks, but I didn't know until I tried it. The text should \
clarify this. Then I see this:
http://imagebin.ca/view/iKtHSXE.html'
Now I am just confused. What happened to the tracks on the left? What are \
the entries on the right? None of them are track names, they are actually \
album names. It's not good to h ave the left listview be tracks and the \
right listview be albums without any sort of help for the user to figure \
that out. Anyway, as a new user i'm curious as to what these entries \
are, so I expand one or two:
http://imagebin.ca/view/jrUBpR3K.html'
And so I am extrapolating here, it seems like it is picking albums and \
bolding the names of tracks in the album that matches one of the tracks I \
had on the left---but has now disappeared. What are all the non-bold track \
names for? They are not related to the original tracks in the left hand \
pane. I can't have an overview of what this dialog is actually going to \
do.
I'm not sure at this point if "Save" operates on the \
currently selected tracks, or if it saves all the tracks (what tracks? the \
ones that are bold? all of them? it is confusing).
Also, I am unable to use the MusicDNS finder. No matter what, it never find \
anything. Here is my debug output:
amarok: BEGIN: MusicDNSFinder::MusicDNSFinder(const \
Meta::TrackList&, QObject*, const QString&, int, const \
QString&, const QString&, const QString&)
amarok: [MusicDNSFinder] Initiating MusicDNS search:
amarok: [MusicDNSFinder] Host: \
"ofa.musicdns.org"
amarok: [MusicDNSFinder] Port: 80
amarok: [MusicDNSFinder] Path Prefix: "/ofa/1" \
amarok: [MusicDNSFinder] Client ID: \
"0c6019606b1d8a54d0985e448f3603ca"
amarok: [MusicDNSFinder] Client version: \
"2.3-GIT"
amarok: BEGIN: MusicDNSAudioDecoder::MusicDNSAudioDecoder(const \
Meta::TrackList&, int)
amarok: END__: MusicDNSAudioDecoder::MusicDNSAudioDecoder(const \
Meta::TrackList&, int) - Took 0.085s
amarok: END__: MusicDNSFinder::MusicDNSFinder(const \
Meta::TrackList&, QObject*, const QString&, int, const \
QString&, const QString&, const QString&) - \
Took 0.086s
amarok: END__: void MusicBrainzTagger::searchDone() - DELAY Took (quite \
long) 10s
amarok: END__: void MusicBrainzFinder::sendNewRequest() - DELAY Took \
(quite long) 10s
amarok: BEGIN: void MusicDNSFinder::decodingDone(ThreadWeaver::Job*)
amarok: END__: void MusicDNSFinder::decodingDone(ThreadWeaver::Job*) - \
Took 0.00011s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: [MusicDNSFinder] There is no any queued requests. Stopping \
timer.
amarok: BEGIN: void MusicBrainzTagger::mdnsSearchDone()
amarok: END__: void MusicBrainzTagger::mdnsSearchDone() - Took 6.9e-05s \
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00026s
amarok: BEGIN: void MusicBrainzFinder::sendNewRequest()
amarok: [MusicBrainzFinder] There is no any queued requests. Stopping \
timer.
amarok: BEGIN: void MusicBrainzTagger::searchDone()
amarok: END__: void MusicBrainzTagger::searchDone() - Took 8.5e-05s
amarok: END__: void MusicBrainzFinder::sendNewRequest() - Took 0.00029s
am I doing something wrong?</pre>
</blockquote>
<p>On September 22nd, 2010, 9:30 a.m., <b>Sergey Ivanov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">This UI made to be like on Picard (original musicbrainz \
tagger). On a left side we have tracks to search, on a right side - search \
result. When a track found we move it from left side to the right one. On \
the right table complete album's track list is shown. Albums, that \
contains a track has a bold text, the same to tracks - all tracks in a \
album linked to real tracks has a bold text. BTW all tracks are drag-able. \
So if don't wanna save search result for some tracks, you can drop this \
tracks to left panel. Could you send one of this track on \
123kash@gmail.com, i didn't have such troubles with my "test \
samples".</pre> </blockquote>
<p>On September 24th, 2010, 1:11 a.m., <b>Leo Franchi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Sorry for the delay. Work has been keeping me busy.
I've never used Picard, so that's why I was confused. But I think \
this points to a valid criticism---the use of Picard is not necessary to \
the use of Amarok. Users should not be expected to be familiar with the \
user interface of another program to feel comfortable with Amarok's \
user interface. For example, I had no idea i could drag tracks---there are \
no visual indicators that dragging is possible, nor do I know what dragging \
would do exactly (why is dragging right->left "choosing not to save \
results"?). My point with this is that I think we need a better \
explanation than "it is how Picard does it"---we want Amarok to \
be as intuitive and user-friendly as possible, regardless of how good or \
bad other programs are :) I think a few things at least need to be done:
1) Workflow clarified---what is the user supposed to do? What buttons do \
what, and how is the user informed of that? 2) Text to explain to the user \
what to do. Tooltips to explain what buttons do
Re: files:
What sort of files are supposed to 'work'? I tried some popular \
bands (the beatles, the who, queen, led zeppelin) that they definitely \
should have in their records, but got 0 results. I can send you some if you \
want, or come find me on irc (nick lfranchi, #amarok on Freenode)</pre> \
</blockquote>
<p>On September 24th, 2010, 1:05 p.m., <b>Sergey Ivanov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">All known by musicdns.org, but It could be some problems due \
to missed check in CMakeLists, so MusicDNS \
decomressor/fingerprint-generator should not been compiled at all. Now It \
fixed, so could you try one more time?</pre> </blockquote>
<p>On September 25th, 2010, 2:58 p.m., <b>Leo Franchi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Maybe the new ffmpeg stuff doesn't support flac?
[flac @ 0x4cb4380] invalid coding type
[flac @ 0x4cb4380] decode_frame() failed
Tried also with an MP3 (AC/DC Back in Black, very very common file) and it \
just seems to loop with a network request:
amarok: BEGIN: void MusicBrainzTagger::searchDone()
amarok(18683)/kdeui (KNotification) KNotificationManager::close: 23598
amarok: BEGIN: MusicDNSFinder::MusicDNSFinder(const \
Meta::TrackList&, QObject*, const QString&, int, const \
QString&, const QString&, const QString&)
amarok: [MusicDNSFinder] Initiating MusicDNS search:
amarok: [MusicDNSFinder] Host: \
"ofa.musicdns.org"
amarok: [MusicDNSFinder] Port: 80
amarok: [MusicDNSFinder] Path Prefix: "/ofa/1"
amarok: [MusicDNSFinder] Client ID: \
"0c6019606b1d8a54d0985e448f3603ca"
amarok: [MusicDNSFinder] Client version: "2.4-GIT"
amarok: END__: MusicDNSFinder::MusicDNSFinder(const \
Meta::TrackList&, QObject*, const QString&, int, const \
QString&, const QString&, const QString&) - \
Took 0.00054s
amarok: END__: void MusicBrainzTagger::searchDone() - Took 0.95s
amarok: END__: void MusicBrainzFinder::sendNewRequest() - Took 0.95s
[mp3 @ 0x42d7c90] max_analyze_duration reached
[mp3 @ 0x42d7c90] Estimating duration from bitrate, this may be inaccurate \
amarok: BEGIN: void MusicDNSFinder::sendNewRequest() \
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00013s
amarok: BEGIN: void MusicDNSFinder::trackDecoded(Meta::TrackPtr, QString)
amarok: END__: void MusicDNSFinder::trackDecoded(Meta::TrackPtr, QString) - \
Took 0.00027s
amarok: BEGIN: void MusicDNSFinder::decodingDone(ThreadWeaver::Job*)
amarok: END__: void MusicDNSFinder::decodingDone(ThreadWeaver::Job*) - Took \
0.00014s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok(18683)/kio (Scheduler) KIO::SchedulerPrivate::doJob: \
KIO::SimpleJob(0x48b5170)
amarok: [MusicDNSFinder] Request sent: \
"http://ofa.musicdns.org:80/ofa/1/track/?gnr=&art=&rmd=0&ci \
d=0c6019606b1d8a54d0985e448f3603ca&alb=&fmt=&brt=128&cvr=2.4 \
-GIT&fpt=AQ1DLwI4DDxJF9sTSBS/D+UT6xEzDyQQTgqkFHUSZg+hCpsPhgsgDbcaJBWSEl4 \
TzhFNC5MJHAb8BdYFXAR/BE4DrwPrA0sDMANiAucClQKS7/m6bdptYZz/SAuOAcD+HwF+Afb/e/z \
z/rD65fhOAMj/Vf/H/br+7vm0/BH+y/9D/1kAAP/7AJwBkQKOAnACBwGNAMYBJgGvAb0BdQGmAaM \
NCzRJII0sIv2C/dX1dv6N9Lr2hPbS8iD5SOkK9cv50fsW8wD27u+jxSXbg9p23+zjtu6M9Vj46/p \
w/Aj81/4E/jH+J/01/ZX9vv5K/wH+lf1zCLsTkRNe7crrTcyBBLbbcfGJ6ynwa/bbwLIKl+mP9d7 \
6/gURCURBgwfrF7YBE/zY/1L9wwBgAGEAYABpAMMBBADVARIARQBm//UAUgACJ4FATqeHERT2swT \
nDKn9XfB2/A8D+/W3BxQIGeQaCXoC7PfR+8MA0xnIBwoHPf1JARoAMQB+/9MCCgKbArACBgL1Aio \
BtgIXAgEBngIsAZnzcfKfIKMCt/bN+wkvfPKX8TDmSf686sQCnTFw38cD8fnD7Mn2ZADjO9rlqwG \
E13DiyvNf9bX7Xvwi/jr+ZgA2/4b//wFp/97/kP9g/sf/ugss/R8RBgZBAA8ERf/g+rTplNpk8Nw \
E3gRhFRj2KO4x6ILWtshtyArxVis/CLQkkxdFDvMFvgNKA8oDcQIZAXD/9gBHAN0AsQIAALsArwDKIDdBRQ==&ttl=&tnm=&lkt=&dur=254000&yrr=" \
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00059s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00011s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.9e-05s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00013s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00011s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.6e-05s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.8e-05s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9e-05s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.6e-05s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.7e-05s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.9e-05s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00013s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00011s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.9e-05s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00014s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00011s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00012s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00012s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.00013s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 9.5e-05s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
amarok: BEGIN: void MusicDNSFinder::sendNewRequest()
amarok: END__: void MusicDNSFinder::sendNewRequest() - Took 0.0001s
</pre>
</blockquote>
<p>On September 25th, 2010, 9:11 p.m., <b>Sergey Ivanov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Trouble with flac fixed. There is no any loops with request, \
sendNewRequest runs once a second by timer. That (sendNewRequest() spam) \
happened due to some network lag, 'cos there was no reply from server, \
or error report, and if you follow the link from log, you'll find track \
PUID in XML wrap. Now timer stops when there is no more requests to \
send.</pre> </blockquote>
<p>On September 28th, 2010, 8:41 p.m., <b>Sergey Ivanov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">And so? :) I filled up all hints, to make it's behavior \
clear for end user, and fixed all found bugs (didn't I?). What should I \
do now? :)</pre> </blockquote>
<p>On September 29th, 2010, 1:51 a.m., <b>Leo Franchi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Thanks for the work. I apologize that I am not able to reply \
to your updates and test faster. Unfortunately I have a day job :P
I just tested again the musicdns stuff, and worked.
I appreciate your improvements to the UI. However, you have not addressed \
some of my main concerns regarding the UI, mostly regarding the workflow \
itself. Tooltips are good, but do not substitute for an easy to use \
interface. I have also asked Thomas Pfeiffer, our resident usability \
professional, for some more formal comments. He will comment as well as \
soon as he has a chance, and I value his opinion as he actually knows what \
he is talking about when he analyzes interfaces :) </pre> </blockquote>
<p>On September 29th, 2010, 9:39 p.m., <b>Thomas Pfeiffer</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">I finally found the time to have closer look at the problem \
and post some comments. Sorry for letting you wait so long. My personal \
life got in the way.
Let's have a look at this from a task/user perspective. First question: \
What is the task the user wants to complete with this feature? In my \
optinion it is the following: The user has some tracks with \
missing/incomplete/incorrect tags. The user wants to find the correct tags \
using musicbrainz. So what would the logical steps for this task be?
1. Select the tracks to update the tags for.
2. Send information on the tracks to musicbrainz.
3. Receive suggestions for tags from musicbrainz.
4. Select the tracks for which the correct tags have been found.
5. Update the tags on the files.
So the user selects the tracks to be updated and activates the mass tagging \
feature. The user is presented with a dialog with two list boxes, two forms \
for tags and several buttons. The Save button doesn't make any sense at \
this point since there is nothing to save. The user clicks "Start \
searching". Several album and track names appear in the right list \
and the left list is empty. Some tracks are bold. So the user is presented \
with a lot of information she does not actually care about (the other \
tracks on those albums for which she has not even searched) in an \
unexpected place (not visually related to the original tracks that were in \
the left box). What the user expects at this point is a clear connection \
between the tracks to be tagged and the tags musicbrainz suggests for them. \
What the user has: Tracks without correct tags. What the user wants: \
Correct tags for these tracks. Nothing else.
The next thing the user wants to do is to decide for each track if the tags \
suggested for it are correct or not. The choice should not be "Do I \
want this track to be updated with musicbrainz tags or not?" but \
"Are the suggested tags for this track correct or not?"
So I'd suggest the following:
Use the same columns in the left as in the right field.
Leave the selected tracks in the left box.
Remove all the tag fields below the lists. We don't need them for this \
task. In the right box, display the suggested tags for each track \
side-by-side to the track on the left. Place a checkbox to the left of \
each track. An "Update tags" button below the checkboxes should \
update the tags of all checked tracks. If you want to use drag&drop as \
an alternative, allow dragging of the tags on the right onto the track on \
the left. This would instantly update the tags and show them on the left.
So, long story short, the essence is: Give the user exactly the information \
she needs to make the following informed decision: "Which of the \
tracks I searched for can be updated using musicbrainz because it has found \
the correct tags?" No more, no less. And in the most direct manner \
possible. The above suggestion is only one possible solution and I could \
agree with others as long as they keep this essence.
Regards,
Thomas</pre>
</blockquote>
<p>On October 1st, 2010, 7:44 a.m., <b>Sergey Ivanov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Thank you very much, for you comment. Rebuild UI according to \
you scheme (i hope so). Now I'm trying to find best way to make user \
know what's going on (status bar, or splash screen during search \
process), and some way to show that tracks were saved.</pre> </blockquote>
<p>On October 1st, 2010, 5:14 p.m., <b>Leo Franchi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Great, it compiles and works! And awesome that it uses ffmpeg \
instead of xine! It's already much much easier to use for someone \
unfamiliar with Picard like me :)
Here are my last few comments, I think after these we're all good to \
go! We'll commit the fix after the patch is updated.
1) No progress info. The MusicDNS lookup took ~2 min here, but I had no \
feedback that it was going on. A progress bar would be awesome, just so the \
user doesn't get bored and close the dialog. Maybe between the start \
and update buttons? I don't know, whatever looks good :) 2) When songs \
have no tags, it's impossible to see them in the list: \
http://imagebin.ca/view/4xMgfa.html' . Maybe when there are no tags, it \
could display the filename instead? That way the user is actually able to \
select th em :) 3) What do you think about auto-starting the search task \
when the dialog is opened? I don't know if this is a good idea, but I \
assume the user might want see what is available as soon as possible, \
without having to press Search (after all, he already chose the "use \
musicbrainz" button in the tag dialog). up for discussion, but just an \
idea :) 4) I think there should be a "cancel" button next to the \
OK button, and the OK button should also update the tags. I pressed OK a \
few times thinking it would "save&close" but instead it \
closed without saving. having a cancel button will make it clear how to \
exit with and without saving the checked songs.
thanks for all your work! i'm really excited to have this feature at \
long last be a part of amarok.</pre> </blockquote>
<p>On October 2nd, 2010, 7:06 a.m., <b>Sergey Ivanov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">1) I'm working on It. 2) I's strange, 'cos in my \
case there is file name. http://imagebin.ca/view/VRSsQnZw.html. I'll \
add check for It. 3) I'll try to make one trick for this stuff.
4) When the user press "Update tags", all changes have \
immediately sent to TagDialog. Probably It would be better to remove this \
button at all, and give this job to Ok(Save&Close whatever) button? In \
this case we definitely have to have Cancel or Close button. </pre> \
</blockquote>
<p>On October 2nd, 2010, 7:15 a.m., <b>Mark Kretschmann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Sergey, I think your point 4) is an excellent idea! The button \
seems to be duplicated functionality indeed, as you have to press \
Save/Cancel in the TagDialog anyway. </pre>
</blockquote>
<p>On October 2nd, 2010, 4:04 p.m., <b>Thomas Pfeiffer</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">It's great that you implemented the suggestions so \
quickly. That's what makes F/OSS usbaility work fun ;) I agree that \
having one button to update the tags and close the dialog and one cancel \
button would be best. I'd prefer "Update tags" as the label \
for the first button, I think it fits pretty well. And we don't need to \
add "& close" to the label. Users expect the dialog to close \
after updating the tags anyway, since its work is done then. And I agree \
with Leo that starting the search instantly really makes sense. If you can \
achieve that, we have a dialog that contains exactly the elements necessary \
for the task :)
Just two more comments:
1. I currently don't compile Amarok from git, so I only have the \
screenshots above, therefore I don't know: Are there checkboxes when \
search results are diplayed or is drag&drop the only way to select \
which tracks should be updated? That might be problematic since D&D is \
not easily discoverable for all users. 2. Could you add labels above the \
two list boxes? My suggestion: "Current tags" for the left one \
and "Tags suggested by MusicBrainz" for the right one.
Thank you for the great work,
Thomas</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I removed \
D&D ability. It doesn't make sense since we have check boxes and \
don't have complete album track list. </pre> <br />
<p>- Sergey</p>
<br />
<p>On October 2nd, 2010, 5:32 p.m., Sergey Ivanov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px \
black solid;"> <tr>
<td>
<div>Review request for amarok.</div>
<div>By Sergey Ivanov.</div>
<p style="color: grey;"><i>Updated 2010-10-02 17:32:14</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description \
</h1> <table width="100%" bgcolor="#ffffff" cellspacing="0" \
cellpadding="10" style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">MusicBrainz and MusicDNS services support implementation. \
There are three independent parts: 1. MusicBrainzFinder class - used to \
make requests to MusicBrainz server and process replies. All relies process \
in separate threads by MusicBrainzXmlParser class. For a search uses \
guessed from a file name track information. No external dependences \
required. 2. MusicDNSFinder class - used for the same purpose as \
MusicBrainzFinder, but i communicate with musicdns server and receives \
track's PUID as a reply. Replies ether process in separate threads by \
MusicDNSXmlParser class. Fingerprints generated by libofa (the only \
external dependence in entire patch). For track decompressing \
(MusicDNSAudioDecoder class) used xine engine (I'm not sure is It a \
good choice, but amarok based on phonon media-engine, that uses xine. So we \
don't deed to pull any other dependences). Received PUIDs sends to \
MusicBrainzFinder class, for a search routine. 3. View. All classes used \
for store (MusicBrainzTagsModel, MusicBrainzTrackListModel) and display \
(MusicBrainzTagsModelDelegate) purposes.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> \
</h1> <ul style="margin-left: 3em; padding-left: 0;">
<li>CMakeLists.txt <span style="color: grey">(2a0961c)</span></li>
<li>cmake/modules/FindFFmpeg.cmake <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>cmake/modules/FindLibOFA.cmake <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>config-amarok.h.cmake <span style="color: grey">(981b7b7)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(ac5db9f)</span></li>
<li>src/core-impl/capabilities/timecode/TimecodeEditCapability.h <span \
style="color: grey">(6e15303)</span></li>
<li>src/core-impl/capabilities/timecode/TimecodeEditCapability.cpp <span \
style="color: grey">(8205d45)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h \
<span style="color: grey">(e35b57f)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp \
<span style="color: grey">(9be62d9)</span></li>
<li>src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp \
<span style="color: grey">(8bdbf75)</span></li>
<li>src/core-impl/collections/sqlcollection/CapabilityDelegateImpl.cpp \
<span style="color: grey">(b5cb083)</span></li>
<li>src/core-impl/collections/sqlcollection/SqlMeta.h <span style="color: \
grey">(ee3ec21)</span></li>
<li>src/core-impl/collections/sqlcollection/SqlMeta.cpp <span \
style="color: grey">(2da0333)</span></li>
<li>src/core-impl/meta/file/File.h <span style="color: \
grey">(6d4d395)</span></li>
<li>src/core-impl/meta/file/File.cpp <span style="color: \
grey">(30cd2ff)</span></li>
<li>src/core-impl/meta/file/TagLibUtils.cpp <span style="color: \
grey">(15b64a4)</span></li>
<li>src/core-impl/meta/proxy/MetaProxy.h <span style="color: \
grey">(2ef3805)</span></li>
<li>src/core-impl/meta/proxy/MetaProxy.cpp <span style="color: \
grey">(341e076)</span></li>
<li>src/core/capabilities/EditCapability.h <span style="color: \
grey">(79344bd)</span></li>
<li>src/dialogs/MusicBrainzTagger.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/dialogs/MusicBrainzTagger.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/dialogs/MusicBrainzTagger.ui <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/dialogs/TagDialog.h <span style="color: \
grey">(50cd801)</span></li>
<li>src/dialogs/TagDialog.cpp <span style="color: \
grey">(26d4eb8)</span></li>
<li>src/dialogs/TagDialogBase.ui <span style="color: \
grey">(9974d0b)</span></li>
<li>src/musicbrainz/MusicBrainzFinder.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzFinder.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzMetaClasses.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzMetaClasses.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTagsModel.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTagsModel.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTrackListModel.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTrackListModel.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzXmlParser.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzXmlParser.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicDNSAudioDecoder.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicDNSAudioDecoder.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicDNSFinder.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicDNSFinder.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicDNSXmlParser.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicDNSXmlParser.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/widgets/CheckButton.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/widgets/CheckButton.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>tests/core-impl/collections/proxycollection/TestProxyCollectionMeta.cpp \
<span style="color: grey">(55d1914)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100000/diff/" \
style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic