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

Sat Mar 24 12:01:54 UTC 2012

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/AmarokUrl.h, line 47
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54398#file54398line47>
> >
> >     Please add the following documentation comment before the method:
> >     
> >     /**
> >      * Sets the url argument named @param name to @param value. Overrides any possible previous value.
> >      */

Done.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/AmarokUrl.cpp, line 22
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54399#file54399line22>
> >
> >     Not needed anymore.

Removed. That's just plain ignorance on my part. Should have checked before uploading it.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/AmarokUrl.cpp, line 27
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54399#file54399line27>
> >
> >     Ditto.

Removed.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/AmarokUrl.cpp, line 118
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54399#file54399line118>
> >
> >     Sorry, I didn't formulate my remark well: the documenting comments should be added to method declarations in .h files (see above).

I should have known. Ah well, you learn through mistakes.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/BookmarkModel.h, line 105
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54400#file54400line105>
> >
> >     This method does 2 things (renaming and setting argument), but should do only one (setting argument). The signature should be just:
> >     
> >     void setBookmarkArg(const QString &bookmarkName, const QString &key, const QString &value);
> >     
> >     Plase add documentation, too, in a format similat to what I haved showed above.

Done.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/BookmarkModel.h, line 118
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54400#file54400line118>
> >
> >     Ditto, should just set argument. (and the newName argument should therefore vanish)

Done.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/BookmarkModel.cpp, line 582
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54401#file54401line582>
> >
> >     Just to note, this debugging print should stay, because this actually points some kind of a mistake. But in order to be useful, it should be formulated more verbosely and perhaps turned into a warning(), like this:
> >     
> >     warning() << "Cannot set argument" << key << "of the bookmark" << name << "to value" << value << "- bookmark not found.";
> >     
> >     spaces are automatically added in << operator.

Taken care of.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/PlayUrlGenerator.h, line 42
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54404#file54404line42>
> >
> >     Please document what this method does and its parameters in a docstring and don't forget to note that caller must supply a valid bookmark name including the " - mm:ss" part.

Done.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/PlayUrlGenerator.cpp, line 95
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54405#file54405line95>
> >
> >     This will have to be split into 2 call wrt BookmarkModel changes.

Done. The renaming part is now a different function call.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/widgets/BookmarkTriangle.h, line 22
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54411#file54411line22>
> >
> >     Minor thing: is seems it would suffice to include MetaUtility in the .cpp file and therefore reduce compilation times.

Taken care of.

> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/widgets/BookmarkTriangle.cpp, line 135
> > <http://git.reviewboard.kde.org/r/104307/diff/2/?file=54412#file54412line135>
> >
> >     Minor things: trailing whitespace, excessive debugging.

Taken care of.

And finally, I apologize for the silly mistakes I made. I should have been more careful and will make sure not to repeat them in the future.

- Jasneet

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

On March 23, 2012, 10:50 p.m., Jasneet Bhatti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104307/
> -----------------------------------------------------------
> 
> (Updated March 23, 2012, 10:50 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.
> 
> 
> Diffs
> -----
> 
>   src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 
>   src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 
>   src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 
>   src/amarokurls/PlayUrlGenerator.h 131b737 
>   src/amarokurls/PlayUrlGenerator.cpp 90e71ff 
>   src/amarokurls/ContextUrlGenerator.cpp 16986f6 
>   src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 
>   src/amarokurls/BookmarkModel.h 73ae345 
>   src/amarokurls/BookmarkModel.cpp 9218088 
>   src/amarokurls/AmarokUrl.cpp 19ba210 
>   src/amarokurls/AmarokUrl.h 6a1d67f 
>   src/playlist/PlaylistViewUrlGenerator.cpp 0ffcb9f 
>   src/services/ServiceCapabilities.cpp 6129f8e 
>   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/20120324/b075ce6b/attachment-0001.html>