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

[Matěj Laitl]

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



src/amarokurls/AmarokUrl.h
<http://git.reviewboard.kde.org/r/104307/#comment9354>

    Please add the following documentation comment before the method:
    
    /**
     * Sets the url argument named @param name to @param value. Overrides any possible previous value.
     */



src/amarokurls/AmarokUrl.cpp
<http://git.reviewboard.kde.org/r/104307/#comment9355>

    Not needed anymore.



src/amarokurls/AmarokUrl.cpp
<http://git.reviewboard.kde.org/r/104307/#comment9356>

    Ditto.



src/amarokurls/AmarokUrl.cpp
<http://git.reviewboard.kde.org/r/104307/#comment9357>

    Sorry, I didn't formulate my remark well: the documenting comments should be added to method declarations in .h files (see above).



src/amarokurls/BookmarkModel.h
<http://git.reviewboard.kde.org/r/104307/#comment9358>

    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.



src/amarokurls/BookmarkModel.h
<http://git.reviewboard.kde.org/r/104307/#comment9359>

    Ditto, should just set argument. (and the newName argument should therefore vanish)



src/amarokurls/BookmarkModel.cpp
<http://git.reviewboard.kde.org/r/104307/#comment9360>

    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.



src/amarokurls/PlayUrlGenerator.h
<http://git.reviewboard.kde.org/r/104307/#comment9362>

    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.



src/amarokurls/PlayUrlGenerator.cpp
<http://git.reviewboard.kde.org/r/104307/#comment9363>

    This will have to be split into 2 call wrt BookmarkModel changes.



src/widgets/BookmarkTriangle.h
<http://git.reviewboard.kde.org/r/104307/#comment9365>

    Minor thing: is seems it would suffice to include MetaUtility in the .cpp file and therefore reduce compilation times.



src/widgets/BookmarkTriangle.h
<http://git.reviewboard.kde.org/r/104307/#comment9364>

    Documented attributes, good!



src/widgets/BookmarkTriangle.cpp
<http://git.reviewboard.kde.org/r/104307/#comment9366>

    Minor things: trailing whitespace, excessive debugging.


- Matěj Laitl


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/5ca61570/attachment-0001.html>