----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/#review11770 -----------------------------------------------------------
Hi, thanks for the patch, I'd really like to see this feature implemented. But there are issues with the patch; please resolve them. Please don't feel discouraged, no-one can make perfect patches for such big project with rather lacking developer documentation. src/amarokurls/AmarokUrl.h <http://git.reviewboard.kde.org/r/104307/#comment9329> This method shoudn't exist. AmarokUrl is used for all kinds of internal Amarok bookmarks, not just track position bookmarks. AmarokUrl should know nothing about track position bookmarks. I suggest you rename AmarokUrl::appendArg() to setArg() because it is what it does. (and please document it along the way) Then you can call this method to replace "pos" argument followed by saveToDb(); src/amarokurls/AmarokUrl.cpp <http://git.reviewboard.kde.org/r/104307/#comment9330> Err, what is this? This does something that I woudn't expect and it doesn't even document why. Please explain what you try to achieve with this, with examples. src/amarokurls/BookmarkModel.h <http://git.reviewboard.kde.org/r/104307/#comment9333> Similar issue here: moveBookmark() is play-bookmark-specific and BookMarkModel shoudn't know about it. I suggest you rather implement setBookmarkArg( name, key, value ); Also, this method then shouldn't be called directly by BookmarkTriangle, but rather trough PlayUrlGenerator, where moveTrackBookmark( Meta::Track, qint64 newMiliseconds, QString name = QString()); can be. Remaining issue is that would still have to rename the bookmark by name, which I consider a really bad design. src/amarokurls/BookmarkModel.cpp <http://git.reviewboard.kde.org/r/104307/#comment9332> Please don't add DEBUG_BLOCKs and debug() for code that you are not actually debugging. (I know, it is in other methods here, you can take this as an opportunity to remove these, too) src/widgets/BookmarkTriangle.h <http://git.reviewboard.kde.org/r/104307/#comment9331> What if slider width changes during the bookmark lifetime? Also, please also document new (or better, even the old) parameters. - Matěj Laitl On March 22, 2012, 8:33 p.m., Jasneet Bhatti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104307/ > ----------------------------------------------------------- > > (Updated March 22, 2012, 8:33 p.m.) > > > Review request for Amarok. > > > Description > ------- > > This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The > bookmark is movable within the slider. If it is dragged outside the range, it > will revert back to its previous valid location. The bookmark is activated( > seek is called ) only when the bookmark is clicked and its position hasn't > changed. > > In addition, also fixed a bug that caused deletion of the wrong bookmark when > two bookmarks had the same name(possible by manual renaming), by making sure > the location of the bookmark is appended to its name at all times. > > > Diffs > ----- > > src/amarokurls/BookmarkModel.cpp 9218088 > src/amarokurls/AmarokUrl.h 6a1d67f > src/amarokurls/AmarokUrl.cpp 19ba210 > src/amarokurls/BookmarkModel.h 73ae345 > src/widgets/BookmarkTriangle.h 46e9118 > src/widgets/BookmarkTriangle.cpp 4c59d42 > src/widgets/SliderWidget.cpp 5e72e13 > > Diff: http://git.reviewboard.kde.org/r/104307/diff/ > > > Testing > ------- > > Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. > > > Thanks, > > Jasneet Bhatti > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
