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

Sat Mar 24 10:27:35 UTC 2012

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

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

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

I thought about the case when Amarok window is resized during playback. (plase correct me if I'm wrong)

> 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.
> 
> Jasneet Bhatti wrote:
>     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.

Hmm, okay. So don't include this fix in this patch, and give me some time to think more about this, because I see no quick solution right now. (Or open another review request if you come up with something)

- Matěj

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