> On April 15, 2013, 10:47 p.m., Matěj Laitl wrote: > > Thanks for the update, glad to see it. I'm just trying out the code, > > however I got a deadlock - I just clicked Expand Unchosen after or during > > the lookup, backtrace: > > #0 syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:39 > > #1 0x00007ff6f7528b1b in _q_futex (val2=0, addr2=0x0, timeout=0x0, val=2, > > op=0, addr=0x40481f0) at thread/qmutex_unix.cpp:99 > > #2 QMutexPrivate::wait (this=0x40481f0, timeout=<optimized out>) at > > thread/qmutex_unix.cpp:113 > > #3 0x00007ff6f752490d in QMutex::lockInternal (this=<optimized out>) at > > thread/qmutex.cpp:450 > > #4 0x00007ff6f75257ea in lockInline (this=0x4081470) at thread/qmutex.h:198 > > #5 QMutexLocker (m=0x4081470, this=0x7fff238ff4f0) at thread/qmutex.h:109 > > #6 QReadWriteLock::lockForRead (this=0x40815c8) at > > thread/qreadwritelock.cpp:149 > > #7 0x00007ff6f91ea94d in QReadLocker::relock (this=0x7fff238ff570) at > > /usr/include/qt4/QtCore/qreadwritelock.h:111 > > #8 0x00007ff6f9437259 in MusicBrainzTagsItem::isChecked (this=0x40815a0) > > at /home/strohel/projekty/amarok/src/musicbrainz/MusicBrainzTags.cpp:392 > > #9 0x00007ff6f943750d in MusicBrainzTagsView::expandUnchecked > > (this=0x493a8c0) at > > /home/strohel/projekty/amarok/src/musicbrainz/MusicBrainzTags.cpp:945 > > #10 0x00007ff6f764ccb7 in QMetaObject::activate (sender=0x49183e0, > > m=<optimized out>, local_signal_index=<optimized out>, argv=0x7fff238ff7c0) > > at kernel/qobject.cpp:3539 > > #11 0x00007ff6f806e962 in QAction::triggered (this=<optimized out>, > > _t1=false) at .moc/debug-shared/moc_qaction.cpp:277 > > #12 0x00007ff6f806eb4f in QAction::activate (this=0x49183e0, > > event=<optimized out>) at kernel/qaction.cpp:1257 > > #13 0x00007ff6f8492b8a in QAbstractButtonPrivate::click (this=0x491dcd0) at > > widgets/qabstractbutton.cpp:530 > > #14 0x00007ff6f8492e3c in QAbstractButton::mouseReleaseEvent > > (this=0x491dca0, e=0x7fff239002b0) at widgets/qabstractbutton.cpp:1123 > > #15 0x00007ff6f856209a in QToolButton::mouseReleaseEvent (this=<optimized > > out>, e=<optimized out>) at widgets/qtoolbutton.cpp:718 > > #16 0x00007ff6f80cec1e in QWidget::event (this=0x491dca0, > > event=0x7fff239002b0) at kernel/qwidget.cpp:8375 > > #17 0x00007ff6f8075402 in notify_helper (e=0x7fff239002b0, > > receiver=0x491dca0, this=0x11cf820) at kernel/qapplication.cpp:4562 > > #18 QApplicationPrivate::notify_helper (this=0x11cf820, receiver=0x491dca0, > > e=0x7fff239002b0) at kernel/qapplication.cpp:4534 > > #19 0x00007ff6f807b81a in QApplication::notify (this=<optimized out>, > > receiver=0x491dca0, e=0x7fff239002b0) at kernel/qapplication.cpp:4105 > > #20 0x00007ff6f9db51a6 in KApplication::notify (this=0x7fff23900e10, > > receiver=0x491dca0, event=0x7fff239002b0) > > at > > /var/tmp/portage/kde-base/kdelibs-4.10.1-r1/work/kdelibs-4.10.1/kdeui/kernel/kapplication.cpp:311 > > > > Please have a look at this. > > > > When I tried the Expand Unchosen and Collapse Chosen buttons, they didn't > > seem to work as expected (but I tested just quickly). This is not yet a > > complete review, but should be a good start for further work. Anyways, > > thanks for the *massive* changes!
It's not supposed to work, as I stopped the porting to the proxy model to ask for that suggestion. I thought it wasn't even buildable. ;) - Alberto ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105290/#review31124 ----------------------------------------------------------- On April 15, 2013, 12:51 p.m., Alberto Villa wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105290/ > ----------------------------------------------------------- > > (Updated April 15, 2013, 12:51 p.m.) > > > Review request for Amarok and Sergey Ivanov. > > > Description > ------- > > The attached patch addresses several issues and brings some improvements, > listed below. > > Web service 2 is now being used in place of the deprecated version 1: disc > number and artist credit are now better defined. This alone required a > complete rewrite of MusicBrainzXmlParser.(cpp|h). > > Make track search query more complicated and let it handle some mistakes > (documented in code). > > As in web service 2 multiple artists (e.g., Hans Zimmer & James Newton > Howard) are returned splitted (i.e., they do not have one page, but each one > has its own), the "Go to Artist Page" menu button was changed to a > KActionMenu to support showing a sub list. > > Get track title from release information, as different releases can change > track title (e.g., adding (single edit) or similar). > > Instead of doing *release* requests to get album artist and year, do *release > group* requests. It often means fewer requests will be done (and each one > takes at least 1 second, so...). > See http://tickets.musicbrainz.org/browse/SEARCH-214 and > http://tickets.musicbrainz.org/browse/SEARCH-218 > > Reimplement Levenshtein distance algorithm using a more efficient version > (pirated from Wikibooks as well as the current one). > > Tracks without results are now listed and disabled, to let user know what > results are missing. Prior to this, you had to count them by hand. > > File name is shown in track tooltip. Helpful when existing tags are equivocal. > > Selected results are shown in bold font to be easily spotted. > > Track number, track count, disc number (when existing) and release year > (first release date from the release group, actually) are now shown to help > distinguish among results. This means that all the fetched information is now > shown to the user. > > Tracks are sorted by file name (fair choice). It's done after each insertion, > but since insertions are quite sparse, it doesn't cause any performance > problem. > > Toolbar features "Select Best Matches" and "Deselect", the latter being new, > and the former being moved here from the hidden header button. To keep it not > too wide, Expand and Collapse buttons were modified to be KActionMenu. They > now require two clicks, but are easier to distinguish (I always had to stop > and read them to distinguish between "Collapse All" and "Collapse Chosen", > for example), so the time needed to click them looks mostly the same as > before. > > The progress bar now gets to 100% even if MusicDNS search is disabled. > > All releases per track are processed instead of only the best one. I'll > explain better. Each track is associated to several releases. In current > code, all the releases are parsed, but only the one with the highest score is > returned to the dialog. This excludes lots of good results. Now, all of them > can be returned. This obviously can increase the number of returned resuts, > but that's acceptable given that, before, you were not able to tag a lot of > tracks. Also, two following paragraphs are about two features that mitigate > this "problem". > > Tracks with the same visible information are merged. There's no point in > showing several tracks with same title, artist, album name and artist, album > year, disc number, track count and track number, as the user will not be able > to distinguish among them. Just merge them into only one result, keeping a > list of artist, release and track IDs (they are and will be used). Also, the > score is updated to reflect the higher one. The "Go to ... Page" menu buttons > currently link to the top result (i.e., highest scoring) IDs, but in the > future I might add support for showing a list. > > A "Select Best Matches from This Album" menu button was added. It matches the > top result release ID in other *unselected* results, to make album tagging > much easier. Since a result might reference many release IDs (see the > paragraph above), the match is done on the whole list of them. This is highly > recommended over the "Select Best Matches" toolbar button. > > Process MusicDNS results just like MusicBrainz ones (i.e., do not duplicate > the logic). Simply, as they will not carry existing tags, they'll end up > being checked only by track length. > > Rewrite MusicDNS result matching method in the list view. Actually, remove it > (it was checking for equal track ID, but that is a very weak method, > especially after my updates), and use the matching system described above. > > Make MusicBrainzTagsView implementation agnostic (i.e., never use > MusicBrainzTagsItem inside it, and make use of UserRole). > > Remove stale code from MusicBrainzFinder.cpp and MusicBrainzXmlParser.(cpp|h) > dealing with MBID requests (talked to maintainer about this). > > Move and rename methods to enhance consistency and tidyness. Source files are > a bit easier to navigate now. > > Adapt to global Amarok coding style. > > > That's about it. The attached screenshot shows most of the visible updates. > > Even if the patch rewrites many parts of the tagger, this work was possible > only thanks to the solid foundations made by Sergey, who I thank for his work. > > > Diffs > ----- > > src/dialogs/MusicBrainzTagger.h 5fa0271 > src/dialogs/MusicBrainzTagger.cpp 9ac99a6 > src/musicbrainz/MusicBrainzFinder.h db8cb05 > src/musicbrainz/MusicBrainzFinder.cpp 6635d04 > src/musicbrainz/MusicBrainzMeta.h 5cbd190 > src/musicbrainz/MusicBrainzTags.h f70d494 > src/musicbrainz/MusicBrainzTags.cpp c90c2f8 > src/musicbrainz/MusicBrainzXmlParser.h 171a340 > src/musicbrainz/MusicBrainzXmlParser.cpp 473d27a > > Diff: http://git.reviewboard.kde.org/r/105290/diff/ > > > Testing > ------- > > > File Attachments > ---------------- > > Toolbar with icons and QToolButtons > http://git.reviewboard.kde.org/media/uploaded/files/2013/04/15/snapshot1.png > > > Screenshots > ----------- > > > http://git.reviewboard.kde.org/r/105290/s/605/ > > > Thanks, > > Alberto Villa > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
