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

2012-03-25 Thread Jasneet Bhatti 


> On March 25, 2012, 1:48 p.m., Matěj Laitl wrote:
> > Okay, I've commited this with a few formatting changes (breaking long 
> > lines), ChangeLog and appendArg() -> setArg() rename in tests. (Jasneet, 
> > you may want to build Amarok with KDE4_BUILD_TESTS=ON cmake swtich); I've 
> > tried to upload updated patch here twice so that you can see the changes, 
> > but it somehow doesn't show.
> > 
> > Anyways, thanks and I'd be glad to commit more your review requests, 
> > Jasneet, pick any little bug that annoys you.
> 
> Jasneet Bhatti wrote:
> Yoohoo!!!
> Thanks for the suggestions and improvements. And I didn't know much about 
> the testing aspect, but I'm glad I'm finding out more. I've also been 
> studying about it recently.

Oh, I forgot ! If it's convenient, please mail me the updated patch at 
jazneetbha...@gmail.com


- Jasneet


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


On March 24, 2012, 12:03 p.m., Jasneet Bhatti wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104307/
> ---
> 
> (Updated March 24, 2012, 12:03 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/amarokurls/AmarokUrl.h 6a1d67f 
>   src/amarokurls/AmarokUrl.cpp 19ba210 
>   src/amarokurls/BookmarkModel.h 73ae345 
>   src/amarokurls/BookmarkModel.cpp 9218088 
>   src/amarokurls/ContextUrlGenerator.cpp 16986f6 
>   src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 
>   src/amarokurls/PlayUrlGenerator.h 131b737 
>   src/amarokurls/PlayUrlGenerator.cpp 90e71ff 
>   src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 
>   src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 
>   src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 
>   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
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-25 Thread Jasneet Bhatti 


> On March 25, 2012, 1:48 p.m., Matěj Laitl wrote:
> > Okay, I've commited this with a few formatting changes (breaking long 
> > lines), ChangeLog and appendArg() -> setArg() rename in tests. (Jasneet, 
> > you may want to build Amarok with KDE4_BUILD_TESTS=ON cmake swtich); I've 
> > tried to upload updated patch here twice so that you can see the changes, 
> > but it somehow doesn't show.
> > 
> > Anyways, thanks and I'd be glad to commit more your review requests, 
> > Jasneet, pick any little bug that annoys you.

Yoohoo!!!
Thanks for the suggestions and improvements. And I didn't know much about the 
testing aspect, but I'm glad I'm finding out more. I've also been studying 
about it recently.


- Jasneet


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


On March 24, 2012, 12:03 p.m., Jasneet Bhatti wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104307/
> ---
> 
> (Updated March 24, 2012, 12:03 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/amarokurls/AmarokUrl.h 6a1d67f 
>   src/amarokurls/AmarokUrl.cpp 19ba210 
>   src/amarokurls/BookmarkModel.h 73ae345 
>   src/amarokurls/BookmarkModel.cpp 9218088 
>   src/amarokurls/ContextUrlGenerator.cpp 16986f6 
>   src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 
>   src/amarokurls/PlayUrlGenerator.h 131b737 
>   src/amarokurls/PlayUrlGenerator.cpp 90e71ff 
>   src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 
>   src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 
>   src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 
>   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
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-25 Thread Commit Hook 

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


This review has been submitted with commit 
55eb69bc3b0b58c930c8e957ef8f7985a586dd71 by Jasneet Bhatti to branch master.

- Commit Hook


On March 24, 2012, 12:03 p.m., Jasneet Bhatti wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104307/
> ---
> 
> (Updated March 24, 2012, 12:03 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/amarokurls/AmarokUrl.h 6a1d67f 
>   src/amarokurls/AmarokUrl.cpp 19ba210 
>   src/amarokurls/BookmarkModel.h 73ae345 
>   src/amarokurls/BookmarkModel.cpp 9218088 
>   src/amarokurls/ContextUrlGenerator.cpp 16986f6 
>   src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 
>   src/amarokurls/PlayUrlGenerator.h 131b737 
>   src/amarokurls/PlayUrlGenerator.cpp 90e71ff 
>   src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 
>   src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 
>   src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 
>   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
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-25 Thread Matěj Laitl 

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

Ship it!


Okay, I've commited this with a few formatting changes (breaking long lines), 
ChangeLog and appendArg() -> setArg() rename in tests. (Jasneet, you may want 
to build Amarok with KDE4_BUILD_TESTS=ON cmake swtich); I've tried to upload 
updated patch here twice so that you can see the changes, but it somehow 
doesn't show.

Anyways, thanks and I'd be glad to commit more your review requests, Jasneet, 
pick any little bug that annoys you.

- Matěj Laitl


On March 24, 2012, 12:03 p.m., Jasneet Bhatti wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104307/
> ---
> 
> (Updated March 24, 2012, 12:03 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/amarokurls/AmarokUrl.h 6a1d67f 
>   src/amarokurls/AmarokUrl.cpp 19ba210 
>   src/amarokurls/BookmarkModel.h 73ae345 
>   src/amarokurls/BookmarkModel.cpp 9218088 
>   src/amarokurls/ContextUrlGenerator.cpp 16986f6 
>   src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 
>   src/amarokurls/PlayUrlGenerator.h 131b737 
>   src/amarokurls/PlayUrlGenerator.cpp 90e71ff 
>   src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 
>   src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 
>   src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 
>   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
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-24 Thread Jasneet Bhatti 

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

(Updated March 24, 2012, 12:03 p.m.)


Review request for Amarok.


Changes
---

Refined the patch : Made more sensible function calls, provided documentation, 
removed unnecessary statements.


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

  src/amarokurls/AmarokUrl.h 6a1d67f 
  src/amarokurls/AmarokUrl.cpp 19ba210 
  src/amarokurls/BookmarkModel.h 73ae345 
  src/amarokurls/BookmarkModel.cpp 9218088 
  src/amarokurls/ContextUrlGenerator.cpp 16986f6 
  src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 
  src/amarokurls/PlayUrlGenerator.h 131b737 
  src/amarokurls/PlayUrlGenerator.cpp 90e71ff 
  src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 
  src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 
  src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 
  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

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-24 Thread Jasneet Bhatti 


> On March 22, 2012, 9:46 p.m., Matěj Laitl wrote:
> > src/amarokurls/BookmarkModel.cpp, line 571
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-24 Thread Jasneet Bhatti 


> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/AmarokUrl.h, line 47
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > Ditto.

Removed.


> On March 24, 2012, 11:03 a.m., Matěj Laitl wrote:
> > src/amarokurls/AmarokUrl.cpp, line 118
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

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

2012-03-24 Thread Matěj Laitl 

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


The patch greatly improved, thanks! There is just a bunch of minor issues; when 
there are resolved, it will be perfect.

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

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-24 Thread 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


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


Not needed anymore.



src/amarokurls/AmarokUrl.cpp


Ditto.



src/amarokurls/AmarokUrl.cpp


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


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


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



src/amarokurls/BookmarkModel.cpp


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


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


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



src/widgets/BookmarkTriangle.h


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



src/widgets/BookmarkTriangle.h


Documented attributes, good!



src/widgets/BookmarkTriangle.cpp


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

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-24 Thread Matěj Laitl 


> On March 22, 2012, 9:46 p.m., Matěj Laitl wrote:
> > src/amarokurls/BookmarkModel.cpp, line 571
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 16986

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

2012-03-24 Thread Jasneet Bhatti 

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


Changes
---

Updated diff and description.


Description (updated)
---

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

  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

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-24 Thread Jasneet Bhatti 


> On March 22, 2012, 9:46 p.m., Matěj Laitl wrote:
> > src/widgets/BookmarkTriangle.h, line 37
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

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

2012-03-23 Thread Jasneet Bhatti 

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


Changes
---

Updated the description


Description (updated)
---

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

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-22 Thread Matěj Laitl 

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


Hi, thanks for the patch, I'd really like to see this feature implemented. But 
there are issues with the patch; please resolve them. Please don't feel 
discouraged, no-one can make perfect patches for such big project with rather 
lacking developer documentation.


src/amarokurls/AmarokUrl.h


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



src/amarokurls/AmarokUrl.cpp


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.



src/amarokurls/BookmarkModel.h


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.



src/amarokurls/BookmarkModel.cpp


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)



src/widgets/BookmarkTriangle.h


What if slider width changes during the bookmark lifetime? Also, please 
also document new (or better, even the old) parameters.


- Matěj Laitl


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

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

2012-03-18 Thread Jasneet Bhatti 

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

(Updated March 17, 2012, 9:02 a.m.)


Review request for Amarok.


Description (updated)
---

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 to its previous valid location. The bookmark is activated( seek is 
called ) only when the bookmark's position hasn't changed.

In addition, also fixed a bug that deleted a different bookmark that shared the 
same name(possible by manual renaming), by appending the location of the 
bookmark even in case of manual renaming.


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

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel