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

[Jasneet Bhatti]

> 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)
> 
> Jasneet Bhatti wrote:
>     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 ?
> 
> Matěj Laitl wrote:
>     > I don't completely understand the concept of DEBUG_BLOCKs
>     
>     They are simple, they print BEGIN methodName() to Amarok's debugging output (amarok --debug) when method is entered and __END: methodName() when the block goes out of scope.
>     
>     > 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 ?
>     
>     When adding new code, you know whether you debug it or not. For existing code, I usually look at the time the line was last modified (git blame shows that). If it is a year or so, the debugging was probably just left out by mistake or laziness. (Ok, there are cases where debugging printouts should be keft forefer for tricky code, these should ba scarce though)
>     
>     Answering your question, probably all DEBUG_BLOCKs and debug() calls should be removed from BookmarkModel, but please don't do it in this patch, just don't add it to the new code. (because each commit should focus on just one thing)

Done.


> 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.
> 
> Jasneet Bhatti wrote:
>     I don't know of a case when the slider width changes. Does it happen while streaming live ?
> 
> Matěj Laitl wrote:
>     I thought about the case when Amarok window is resized during playback. (plase correct me if I'm wrong)

The patch works fine even in that case. I believe on resizing the window, updateTimecodes() is called that takes care of the redrawing the bookmarks at the correct location.


- Jasneet


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


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/57526f85/attachment-0001.html>