> On April 15, 2013, 12:54 p.m., Alberto Villa wrote: > > src/musicbrainz/MusicBrainzTags.cpp, line 588 > > <http://git.reviewboard.kde.org/r/105290/diff/3-4/?file=87207#file87207line588> > > > > Is this too hacky? > > Matěj Laitl wrote: > Calling sort( ... Qt::Descending_or_how_it_is_called ) on the view would > be cleaner, but we could even live with this. :-) > > Matěj Laitl wrote: > But I see you need to sort ascendingly for the "track" items. The only > clear solution would be to implement custom sorting method in > QSortFilterProxyModel to account for different sorting on different levels. > See StatSyncing::SortFilterProxyModel in > src/statsyncing/ui/MatchedTrackPage.cpp > > Alberto Villa wrote: > What a pity, it was such an easy solution! :P
I believe it's not worth subclassing QSortFilterProxyModel and reimplement sort() just to seek a "clear solution" here. If you don't mind, I'd keep the current solution, otherwise I'll subclass. > On April 15, 2013, 12:54 p.m., Alberto Villa wrote: > > src/musicbrainz/MusicBrainzTags.cpp, line 957 > > <http://git.reviewboard.kde.org/r/105290/diff/3-4/?file=87207#file87207line957> > > > > I could tolerate the view knowing the specific model, but know it also > > needs to know if it's behind a proxy. I really don't like this approach, > > I'd rather prefer it to refernce only QAbstractItemModel like in other > > methods. I could move isChcked() and checkAllFromRelease() to the view > > (making them a bit more complex), but I'm not sure they belong there. > > > > Any advice? > > Matěj Laitl wrote: > > I could tolerate the view knowing the specific model, but know it also > needs to know if it's behind a proxy. I really don't like this approach, I'd > rather prefer it to refernce only QAbstractItemModel like in other methods. I > could move isChcked() and checkAllFromRelease() to the view (making them a > bit more complex), but I'm not sure they belong there. > > IMO the model is the right place. > > > Any advice? > > The "correct" approach would have been to provide additional role, let's > call it "CheckedRole" (or perhaps ChoosenRole?) in MusicBrainzTagsModel and > implement the setData() method to allow changing it through standard > interfaces. This works fine until you start actually filtering in > QSortFilterProxy model, where it would skip filtered items. > > There seems to be no plausible solution. Perhaps the view could have a > m_sourceModel attribute, but I leave it up to you; feel free to retain the > current solution, it's not that bad. > > I agree that the model is the right place for chooseBestMatchesFromRelease(), so I created a sourceModel() method and let's be happy with that. I reworked collapseChosen()/expandeUnchosen() to use ChosenStateRole; I could have used the already existing Qt::CheckedStateRole, but it would have been a bit more hacky. - Alberto ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105290/#review31090 ----------------------------------------------------------- On April 18, 2013, 10:13 a.m., Alberto Villa wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105290/ > ----------------------------------------------------------- > > (Updated April 18, 2013, 10:13 a.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/CMakeLists.txt d667d95 > src/dialogs/MusicBrainzTagger.h 5fa0271 > src/dialogs/MusicBrainzTagger.cpp 9ac99a6 > src/dialogs/MusicBrainzTagger.ui 6762418 > 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/MusicBrainzTagsItem.h PRE-CREATION > src/musicbrainz/MusicBrainzTagsItem.cpp PRE-CREATION > src/musicbrainz/MusicBrainzTagsModel.h PRE-CREATION > src/musicbrainz/MusicBrainzTagsModel.cpp PRE-CREATION > src/musicbrainz/MusicBrainzTagsModelDelegate.h PRE-CREATION > src/musicbrainz/MusicBrainzTagsModelDelegate.cpp PRE-CREATION > src/musicbrainz/MusicBrainzTagsView.h PRE-CREATION > src/musicbrainz/MusicBrainzTagsView.cpp PRE-CREATION > 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 > New toolbar with latest icons and descriptions. > > http://git.reviewboard.kde.org/media/uploaded/files/2013/04/18/toolbar_wider.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
