Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved

Fri Mar 23 22:46:13 UTC 2012

> On March 22, 2012, 9:46 p.m., Matěj Laitl wrote:
> > src/widgets/BookmarkTriangle.h, line 37
> > <http://git.reviewboard.kde.org/r/104307/diff/1/?file=53589#file53589line37>
> >
> >     What if slider width changes during the bookmark lifetime? Also, please also document new (or better, even the old) parameters.

I don't know of a case when the slider width changes. Does it happen while streaming live ?

> On March 22, 2012, 9:46 p.m., Matěj Laitl wrote:
> > src/amarokurls/AmarokUrl.h, line 60
> > <http://git.reviewboard.kde.org/r/104307/diff/1/?file=53585#file53585line60>
> >
> >     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();

Done.

> On March 22, 2012, 9:46 p.m., Matěj Laitl wrote:
> > src/amarokurls/AmarokUrl.cpp, line 225
> > <http://git.reviewboard.kde.org/r/104307/diff/1/?file=53586#file53586line225>
> >
> >     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.

This is actually not really related to this bug. It's another bug I found: Whenever you rename two bookmarks to the same name, and then delete the bookmark that was created/renamed first, the one created later gets deleted.

So I was trying to implement the rename function in such a way that whenever the user renames the bookmark, the position of the bookmark stays appended to the bookmark name(the regular expression and subsequent conditions check if the user has kept the position or has deleted it after renaming the bookmark). But as you said, AmarokUrl is used for other kinds of bookmarks too, so I guess this method might not be a good idea. Please do suggest an alternative. Meanwhile, I'm reverting back to the original rename function.

> On March 22, 2012, 9:46 p.m., Matěj Laitl wrote:
> > src/amarokurls/BookmarkModel.h, line 103
> > <http://git.reviewboard.kde.org/r/104307/diff/1/?file=53587#file53587line103>
> >
> >     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.

Done(except the renaming part).

> On March 22, 2012, 9:46 p.m., Matěj Laitl wrote:
> > src/amarokurls/BookmarkModel.cpp, line 571
> > <http://git.reviewboard.kde.org/r/104307/diff/1/?file=53588#file53588line571>
> >
> >     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)

I don't completely understand the concept of DEBUG_BLOCKs. So I tried to structure the function like other similar functions. How do you determine that we are not debugging a piece of code, because there is debug() to output the messages ? And what all can be deleted here ?

- Jasneet

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104307/#review11770
-----------------------------------------------------------

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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120323/dfa521fa/attachment-0001.html>