Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- 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
--- 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
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
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
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. 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 http://git.reviewboard.kde.org/r/104307/diff/1/?file=53585#file53585line60 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 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. 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 http://git.reviewboard.kde.org/r/104307/diff/1/?file=53587#file53587line103 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 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) 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 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff:
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- 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
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
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- 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 ___ 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
--- 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
--- 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
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 ___ 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
--- 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
--- 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 http://git.reviewboard.kde.org/r/104307/#comment9329 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 http://git.reviewboard.kde.org/r/104307/#comment9330 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 http://git.reviewboard.kde.org/r/104307/#comment9333 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 http://git.reviewboard.kde.org/r/104307/#comment9332 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 http://git.reviewboard.kde.org/r/104307/#comment9331 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
--- 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
Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- 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 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
