[GSoC Ideas]Unit testing
Hello, my name is Veaceslav Munteanu and I'm year 2 student at Polytechnic University from Bucharest. I'm interested into writting unit tests for Amarok. Can you please provide me with more info about this task(how many tests I'm must write, for which part of the program, another things that I should take care ...) I want to mention that I have some experience with C++, Qt and KDE Libs. I still have no experience with writting tests but, I strongly believe that kde must be more stable and thats why I'm very motivated to make it so. Last summer I worked for digikam(not at GSoC) and plugin FlashExport was refactored and improved by me: https://projects.kde.org/projects/extragear/graphics/kipi-plugins/repository/revisions/master/show/flashexport Thanks, -- Veaceslav ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Updated Proposal for QML Context View
Hi, I have updated my proposal for QMLify Amarok Context View. I have changed some aspects of the timeline, and am still working on it. Please go through it, and suggest changes. Also, I made a sample class for Proof of Concepts, which I successfully integrated with the existing Amarok code. Do i submit it on the reviewboard? Regards,. Saurabh QMLify Context View.pdf Description: Adobe PDF document ___ 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: GSoC 2012 : Improvised proposal for 'Semantic Collection for Amarok'
On 2012-03-22, Phalgun Guduthur phalgun.gudut...@gmail.com wrote:
I have been working towards the 2012 GSoC idea 'Semantic Collection for
Amarok' since a month now. I have already sent in my first rough draft of
the proposal.
At that time, I promised a proof of Concept and you can it submitted on the
review board https://git.reviewboard.kde.org/r/104369/
It is a small patch demonstrating the read and write of Nepomuk index
through Amarok.
I was very positively surprised how simple the patch is, good!
I have also attached my improvised proposal after interactions with both
the mentors Teo and Vishesh.
Please review my proposal whenever you find time. Any feedback is
appreciated.
Hey, the proposal is very good. If I have a chance to implement the inter-
collection statistics synchronization, you'll get a way of synchronizing
Nepomuk and SQL collection for free, which will be good for users
transitioning betweem Nepomuk and SQL collections.
I would reword a few paragraphs of the technical details though:
The core of the project is to implement a NepomukCollection and
NepomukQueryMaker classes. The classes to handle the generated and indexed
metadata will be needed as well, eg a handler to write data back to Nepomuk,
to update the UI using the metadata from Nepomuk index etc.
These will be probably the Meta::{Track,Album,Artist,Year,Genre} subclasses.
Updating the GUI is done through their notifyObservers() methods and through
Collection's updated() signal for bigger changes. (But I agree that you may
want to have sime kind of Nepomuk helper class; something can go directly into
NepomukCollection though)
Also please note that the Meta::Track interaction with other Meta::
{Album,Artist,...} classes and Collection is a bit tricky to figure out:
1) if 2 tracks have same album (artist, ...), their album() (artist(), ...)
methods should return the exact same Album object.
2) There is a kind of a double link between Track and Album (Artist, ...)
classes: Track has album() and Album has tracks(). Given that all Meta classes
are memory-managed using reference counting trough KSharedPtr, you cannot
simply store a pointer to Album in Track and a list of track pointers in
Album, because that would essentially create a memory leak. But thanks to
Nepomuk, you might not face this problem. (for some inspiration how this is
solved in UMS and iPod Collections, you may have a look at src/core-
impl/collections/support/MemoryMeta.{h,cpp};)
3) While undocumented, all Meta classes' methods should be thread-safe
(Collection's need not)
4) Lifetime of a Meta class object is out of your control. It can happen that
their collection is deleted when they are still alive. This has been the
source of some crashes in past, so please count with it.
I'm confident you'll be able to get through this, I just wanted to point out
some things in advance that I faced when re-writing IpodCollection.
The NepomukQueryMaker can be developed into an API which can be used by
plugin developers to exploit the Nepomuk backend.
Hmm, I think it would be suboptimal to add plugin API just to
NepomukQueryMaker, better add it to the generic QueryMaker, so it will work
for all subclasses. But this feature should be IMO very low priority, this can
be easily done after the GSoC.
The existing database schema will be followed so as to not break the
existing applications and plugins. So, the propagation to a Nepomuk backend
would be seamless.
Hmm, I think I know what you wanted to say here, but database schema is an
implementation detail of the SQL collection. Perhaps you can say something
like Existing design of Meta classes [1] will be followed..
[1] http://amarok.kde.org/wiki/Development/Architecture
Cheers,
Matěj
___
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
Amarok idea need feedback
I hope someone will consider my idea and I'll get feedback https://docs.google.com/document/d/1akDrnxW2aBOSVaOdOmwQOvi0XD3h1gl8xvhZGV18MA4/edit ___ 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
