Re: Review Request 107473: Changes in processing playlist files

2013-03-28 Thread Matěj Laitl 


> On March 28, 2013, 12:16 a.m., Matěj Laitl wrote:
> > Hmmm, I'm apparently able to update the diff, please disregard the attached 
> > file.
> > 
> > Please have a look a differences between v11 and v12, most of them are 
> > style fixes or cleanups, but it also boasts fixes for a couple of subtle 
> > bugs (like mismatched calls to XSPFPlaylist constructor). The patch v12 
> > works rather well here, just one minor problem I've faced - if I drag a 
> > playlist from File Browser to play queue (the "main" playlist), the track 
> > order is not preserved. Could you please have a look at it? Perhaps it is 
> > better to query all tracks at once after trackLoaded() is called instead of 
> > using trackAdded() method.
> 
> Tatjana Gornak wrote:
> Yes, sure I'll try to fix it.
> 
> But there is a problem with v12. When I tried to build it with 
> -DKDE4_BUILD_TESTS=ON, build failed because of undefined reference to symbol 
> 'ThreadWeaver::Weaver::instance()' in testmetamultitrack. It happened because 
> ${KDE4_THREADWEAVER_LIBRARIES} was not added as link library for 
> testmetamultitrack.

Oh, please fix that link error. It is strange but known that something on my 
box links ThreadWeaver even though I don't explicitly mention it, which then 
causes build failures on other boxes.


- Matěj


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


On March 27, 2013, 7:35 p.m., Tatjana Gornak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> ---
> 
> (Updated March 27, 2013, 7:35 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> 
> I've started my changes with an implementation of a ASXPlaylist class,
> due this work I've found that implementation of different playlist file
> types has different logic, as result I end up with a rewriting
> playlist's implementations.
> 
> As example of difference:
>   -- Constructor M3UPlaylist::M3UPlaylist( const KUrl &url ) sets a url, but
>  does not load playlist, therefore loading of playlist are
> posponed till data from playlist are actualy needed (lazy loading)
>  On the other hand constructor XSPFPlaylist::XSPFPlaylist( const
> KUrl &url, bool autoAppend )  loads playlist.
> 
> The main idea of proposed changes is to create a unify code for
> processing playlist files:
>   -- lazy loading through LazyLoadPlaylist
>   -- common functionality was moved to PlaylistFile.
> 
> It is worth to be noticed that this patch changes structure of
> playlist's classes, bit does not changes their behavior, even if this behavior
> is inconsistent in some cases.
> 
> In following patch-requests I plan to submit ASX Playlist parser and
> organize playlists processing in more consistent way.
> 
> 
> This addresses bug 291934.
> https://bugs.kde.org/show_bug.cgi?id=291934
> 
> 
> Diffs
> -
> 
>   src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b 
>   src/CMakeLists.txt 4cdee5e 
>   src/DirectoryLoader.h e53a30d 
>   src/DirectoryLoader.cpp bd7fa34 
>   src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d 
>   src/context/applets/info/InfoApplet.cpp f215885 
>   src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 
>   src/core-impl/collections/umscollection/podcasts/UmsPodcastProvider.cpp 
> 1a4a934 
>   src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb 
>   src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 
>   src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h PRE-CREATION 
>   src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp PRE-CREATION 
>   src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 
>   src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd 
>   src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 7b982fe 
>   src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 
>   src/core-impl/playlists/types/file/pls/PLSPlaylist.h e69e133 
>   src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 8fe4b51 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 
>   src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce 
>   src/core-impl/podcasts/sql/SqlPodcastProvider.cpp 0da9b51 
>   src/core/playlists/Playlist.h 018d2af 
>   src/core/playlists/Playlist.cpp 36879a1 
>   src/core/playlists/PlaylistProvider.h 0fb5818 
>   src/playlist/PlaylistActions.cpp 00bf13a 
>   src/playlist/PlaylistController.cpp e5bde8b 
>   src/playlist/PlaylistRestorer.h PRE-CREATION 
>   src/playlist/PlaylistRestorer.cpp PRE-CREATION 
>   src/playlistmanager/PlaylistManager.h cbf65b0 
>   src/playlistmanage

Re: Review Request 107473: Changes in processing playlist files

2013-03-28 Thread Tatjana Gornak 


> On March 28, 2013, 12:16 a.m., Matěj Laitl wrote:
> > Hmmm, I'm apparently able to update the diff, please disregard the attached 
> > file.
> > 
> > Please have a look a differences between v11 and v12, most of them are 
> > style fixes or cleanups, but it also boasts fixes for a couple of subtle 
> > bugs (like mismatched calls to XSPFPlaylist constructor). The patch v12 
> > works rather well here, just one minor problem I've faced - if I drag a 
> > playlist from File Browser to play queue (the "main" playlist), the track 
> > order is not preserved. Could you please have a look at it? Perhaps it is 
> > better to query all tracks at once after trackLoaded() is called instead of 
> > using trackAdded() method.

Yes, sure I'll try to fix it.

But there is a problem with v12. When I tried to build it with 
-DKDE4_BUILD_TESTS=ON, build failed because of undefined reference to symbol 
'ThreadWeaver::Weaver::instance()' in testmetamultitrack. It happened because 
${KDE4_THREADWEAVER_LIBRARIES} was not added as link library for 
testmetamultitrack.


- Tatjana


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


On March 27, 2013, 7:35 p.m., Tatjana Gornak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> ---
> 
> (Updated March 27, 2013, 7:35 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> 
> I've started my changes with an implementation of a ASXPlaylist class,
> due this work I've found that implementation of different playlist file
> types has different logic, as result I end up with a rewriting
> playlist's implementations.
> 
> As example of difference:
>   -- Constructor M3UPlaylist::M3UPlaylist( const KUrl &url ) sets a url, but
>  does not load playlist, therefore loading of playlist are
> posponed till data from playlist are actualy needed (lazy loading)
>  On the other hand constructor XSPFPlaylist::XSPFPlaylist( const
> KUrl &url, bool autoAppend )  loads playlist.
> 
> The main idea of proposed changes is to create a unify code for
> processing playlist files:
>   -- lazy loading through LazyLoadPlaylist
>   -- common functionality was moved to PlaylistFile.
> 
> It is worth to be noticed that this patch changes structure of
> playlist's classes, bit does not changes their behavior, even if this behavior
> is inconsistent in some cases.
> 
> In following patch-requests I plan to submit ASX Playlist parser and
> organize playlists processing in more consistent way.
> 
> 
> This addresses bug 291934.
> https://bugs.kde.org/show_bug.cgi?id=291934
> 
> 
> Diffs
> -
> 
>   src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b 
>   src/CMakeLists.txt 4cdee5e 
>   src/DirectoryLoader.h e53a30d 
>   src/DirectoryLoader.cpp bd7fa34 
>   src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d 
>   src/context/applets/info/InfoApplet.cpp f215885 
>   src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 
>   src/core-impl/collections/umscollection/podcasts/UmsPodcastProvider.cpp 
> 1a4a934 
>   src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb 
>   src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 
>   src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h PRE-CREATION 
>   src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp PRE-CREATION 
>   src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 
>   src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd 
>   src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 7b982fe 
>   src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 
>   src/core-impl/playlists/types/file/pls/PLSPlaylist.h e69e133 
>   src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 8fe4b51 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 
>   src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce 
>   src/core-impl/podcasts/sql/SqlPodcastProvider.cpp 0da9b51 
>   src/core/playlists/Playlist.h 018d2af 
>   src/core/playlists/Playlist.cpp 36879a1 
>   src/core/playlists/PlaylistProvider.h 0fb5818 
>   src/playlist/PlaylistActions.cpp 00bf13a 
>   src/playlist/PlaylistController.cpp e5bde8b 
>   src/playlist/PlaylistRestorer.h PRE-CREATION 
>   src/playlist/PlaylistRestorer.cpp PRE-CREATION 
>   src/playlistmanager/PlaylistManager.h cbf65b0 
>   src/playlistmanager/PlaylistManager.cpp 89c754b 
>   src/playlistmanager/SyncedPlaylist.h fce3d14 
>   src/playlistmanager/SyncedPlaylist.cpp 4f7c0b7 
>   src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 
>   src/playlistmanager/sql/SqlPlaylist.cpp 88e68

Re: Review Request 109695: JJ#241066: Added a prepareToQuit() signal to amarokWindowScript

2013-03-28 Thread Commit Hook 

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

(Updated March 28, 2013, 8:48 p.m.)


Status
--

This change has been marked as submitted.


Review request for Amarok.


Description
---

Bug 241066 - JJ: Add Signals trackStop and amarokShutdown to Amarok scripting 
interface

Changes:
1. Added a prepareToQuit() signal to amarokWindowScript
2. Replaced kapp macro calls with App::instance() because quit() is not virtual

Note:
Signal trackFinished()[trackStop] already exists, so only added 
prepareToQuit()[amarokShutdown]


Diffs
-

  src/scriptengine/AmarokScript.cpp 922e71d 
  src/dbus/mpris2/MediaPlayer2.cpp f86ccb3 
  src/scriptengine/AmarokWindowScript.h 5407579 
  src/dbus/mpris1/RootHandler.cpp e60eb1b 
  src/MainWindow.cpp 66f4f76 
  ChangeLog 284f188 
  src/App.cpp fdb4255 
  src/scriptengine/AmarokWindowScript.cpp 897e2da 

Diff: http://git.reviewboard.kde.org/r/109695/diff/


Testing
---

Tested the new signal in a script


Thanks,

Anmol Ahuja

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

Re: Review Request 109695: JJ#241066: Added a prepareToQuit() signal to amarokWindowScript

2013-03-28 Thread Commit Hook 

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


This review has been submitted with commit 
b0c79900429e8987c018112f875e09e565126408 by Matěj Laitl on behalf of Anmol 
Ahuja to branch master.

- Commit Hook


On March 27, 2013, 6:21 p.m., Anmol Ahuja wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109695/
> ---
> 
> (Updated March 27, 2013, 6:21 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> Bug 241066 - JJ: Add Signals trackStop and amarokShutdown to Amarok scripting 
> interface
> 
> Changes:
> 1. Added a prepareToQuit() signal to amarokWindowScript
> 2. Replaced kapp macro calls with App::instance() because quit() is not 
> virtual
> 
> Note:
> Signal trackFinished()[trackStop] already exists, so only added 
> prepareToQuit()[amarokShutdown]
> 
> 
> Diffs
> -
> 
>   src/scriptengine/AmarokScript.cpp 922e71d 
>   src/dbus/mpris2/MediaPlayer2.cpp f86ccb3 
>   src/scriptengine/AmarokWindowScript.h 5407579 
>   src/dbus/mpris1/RootHandler.cpp e60eb1b 
>   src/MainWindow.cpp 66f4f76 
>   ChangeLog 284f188 
>   src/App.cpp fdb4255 
>   src/scriptengine/AmarokWindowScript.cpp 897e2da 
> 
> Diff: http://git.reviewboard.kde.org/r/109695/diff/
> 
> 
> Testing
> ---
> 
> Tested the new signal in a script
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

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

Review Request 109781: JJ 312407 - don't transcode from mp3 to mp3

2013-03-28 Thread Anmol Ahuja 

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

Review request for Amarok.


Description
---

/***This patch is still IN PROGRESS***/

Added a checkbox in the dialog:
"Ignore files which're already the selected format"
which lets users select whether to re-encode files that already are in the 
output format.


Diffs
-

  src/core/transcoding/TranscodingConfiguration.h 98b2bb8 
  src/core/transcoding/TranscodingConfiguration.cpp ca84f0d 
  src/transcoding/TranscodingAssistantDialog.h 76287a7 
  src/transcoding/TranscodingAssistantDialog.cpp 6bba0ec 
  src/transcoding/TranscodingAssistantDialog.ui 2505bd3 
  src/transcoding/TranscodingJob.h 6170c2a 
  src/transcoding/TranscodingJob.cpp cab76a7 
  src/transcoding/TranscodingOptionsStackedWidget.cpp cf4f328 

Diff: http://git.reviewboard.kde.org/r/109781/diff/


Testing
---


Thanks,

Anmol Ahuja

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

Re: Review Request 109752: JJ 316128: Handle Data CDs in Amarok

2013-03-28 Thread Commit Hook 

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

(Updated March 28, 2013, 8:05 p.m.)


Status
--

This change has been marked as submitted.


Review request for Amarok.


Description
---

Added a check for data CDs in UmsCollection.cpp so they can be handled by 
UmsCollection in Amarok


Diffs
-

  ChangeLog 284f188 
  src/core-impl/collections/umscollection/UmsCollection.cpp 2b5c53b 

Diff: http://git.reviewboard.kde.org/r/109752/diff/


Testing
---

Tested a data CD to see if it plays
(Not tested with mixed mode disks)

Environment:
Ubunu 12.04 (KDE 4.8.5, udisks1)


Thanks,

Anmol Ahuja

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

Re: Review Request 109752: JJ 316128: Handle Data CDs in Amarok

2013-03-28 Thread Commit Hook 

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


This review has been submitted with commit 
6dd3e1e190118dd079c7c414a44f22eaecffc5d5 by Matěj Laitl on behalf of Anmol 
Ahuja to branch master.

- Commit Hook


On March 27, 2013, 5:11 p.m., Anmol Ahuja wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109752/
> ---
> 
> (Updated March 27, 2013, 5:11 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> Added a check for data CDs in UmsCollection.cpp so they can be handled by 
> UmsCollection in Amarok
> 
> 
> Diffs
> -
> 
>   ChangeLog 284f188 
>   src/core-impl/collections/umscollection/UmsCollection.cpp 2b5c53b 
> 
> Diff: http://git.reviewboard.kde.org/r/109752/diff/
> 
> 
> Testing
> ---
> 
> Tested a data CD to see if it plays
> (Not tested with mixed mode disks)
> 
> Environment:
> Ubunu 12.04 (KDE 4.8.5, udisks1)
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

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

Re: Review Request 109585: FIX UI doesn't say I have to run Moodbar generator manually. (BR 289483)

2013-03-28 Thread Harsh Gupta 

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

(Updated March 29, 2013, 1:01 a.m.)


Review request for Amarok and Myriam Schweingruber.


Changes
---

Fixed all issues.


Description
---

Added a label in moodbar options saying moodbar files must be generated 
manually.


This addresses bug 289483.
https://bugs.kde.org/show_bug.cgi?id=289483


Diffs (updated)
-

  ChangeLog 284f188 
  src/configdialog/dialogs/GeneralConfig.ui 4e33f64 

Diff: http://git.reviewboard.kde.org/r/109585/diff/


Testing
---

All test case passed.


File Attachments


Screenshot
  http://git.reviewboard.kde.org/media/uploaded/files/2013/03/19/moodbar.png


Thanks,

Harsh Gupta

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

Looking for a GSoC mentor

2013-03-28 Thread vedant agarwala 
Hello,
I want to do the project Improving and modularizing tag
guessing
for
this year's Google Summer of Code. For this I need a mentor who can guide
me. I have thought about how the project can be implemented but obviously
it will require modifications and betterment by a mentor.
As I understand the project requires a documented directory (say
"taggetters") containing abstract classes (like "Controller" and
"Provider"). Sub-directories will be created under this directory that will
implement the abstract classes. The existing musicbrainz directory will be
ported into such a sub-directory and it will use the newly created
framework. Later on, more sub-directories can be created that follow this
framework.
A new GUI will have to be created so that these services can be used. The
existing button- Get tags from MusicBrainz- will be replaced by "Get tags
from the following services: ", and followed by QGroupBox containing names
of the features. Getting tags from the individual services will run in
parallel and results will be displayed as soon as they are fetched. The
user can choose the best tags from the generated list.
An alternative GUI would be to keep different buttons for different
services- only the clicked service would be used to get tags.
It might be good to add a feature by which tags for multiple tracks (if not
the entire local collection) can be changed. Getting and/or changing Tags
requires many user clicks as of now. To implement this, a "tag getter
wizard" can be created that selects the best tags for every track (maybe by
comparing results of different services) and it is written to file after
user approval.
This rough proposal is largely based on the idea page with a
few suggestions of my own. A mentor would be able to tell me in a more
concrete way how the project is to be done and also guide me during the
actual process.
I hope someone will come forward and help me out in outlining the needs of
my project. Also, any suggestions/comments will be very helpful.
Thank-you in advance,
Vedant.
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request 108995: FIX Pre-amplifier in equalizer doesn't work

2013-03-28 Thread Matěj Laitl 


> On March 28, 2013, 6:33 p.m., Harsh Gupta wrote:
> > src/EngineController.cpp, line 793
> > 
> >
> > How am I suppose to stage changes in a line ( line 791 ) in which a 
> > variable is first renamed and then get deleted ? Should I commit all the 
> > changes again in order to make two different patches ?
> 
> Harsh Gupta wrote:
> Oops... sorry for poor tagging !!! (first time  :P)
> Line no. is actually 791.

`git add -p` allows you to edit the hunks while staging, so you can use it to 
"generate" changes that "annihilated". From "oldVarName" -> "" you can create 
"oldVarname" -> "newVarName" -> "". (where oldVarName would be in tree, 
newVarName would be in index (staged to commit) and "" would be in working tree)

But perhaps this is too complicated for an user new to git. You may instead do:
1. start a new branch starting just a commit before your changes: git branch 
newbranch oldbranch^ (or master^ if you've worked directly in master)
2. perform the exact same rename using your IDE (should be quick to do), commit
3. check out the working tree of oldbranch (master?) without switching to it: 
git checkout oldbranch -- .
4. `git diff` should show the "real" changes w/thout the renames; commit
5. now you have 2 commits, yay! Ensure that both commits build.


- Matěj


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


On March 14, 2013, 5:30 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated March 14, 2013, 5:30 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> 1. Disabled Pre-amplifier in equalizer if it is not supported by phonon.
> 2. Fixed top and bottom labels of first slider. Earlier band name label was 
> at the top and slider value label was at the bottom for first slider.
> 3. Removed an extra semicolon EqualizerDialog.h .
> Note : I have made an assumption that if at all preamp is present then it 
> will the first element of Effect Parameter list.
> 
> 
> This addresses bug 301311.
> https://bugs.kde.org/show_bug.cgi?id=301311
> 
> 
> Diffs
> -
> 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 58d7360 
>   src/dialogs/EqualizerDialog.h fd9032b 
>   src/dialogs/EqualizerDialog.cpp 7d62e10 
>   src/dialogs/EqualizerDialog.ui 43b0187 
> 
> Diff: http://git.reviewboard.kde.org/r/108995/diff/
> 
> 
> Testing
> ---
> 
> All unit test cases passed.
> Note : I have tested it with gstreamer only. Xine phonon keep crashing on my 
> PC.
> 
> 
> File Attachments
> 
> 
> Equalizer snapshot
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/02/17/equalizer.png
> 
> 
> Thanks,
> 
> Harsh Gupta
> 
>

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

Re: Review Request 108995: FIX Pre-amplifier in equalizer doesn't work

2013-03-28 Thread Harsh Gupta 


> On March 29, 2013, 12:03 a.m., Harsh Gupta wrote:
> > src/EngineController.cpp, line 793
> > 
> >
> > How am I suppose to stage changes in a line ( line 791 ) in which a 
> > variable is first renamed and then get deleted ? Should I commit all the 
> > changes again in order to make two different patches ?

Oops... sorry for poor tagging !!! (first time  :P)
Line no. is actually 791.


- Harsh


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


On March 14, 2013, 11 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated March 14, 2013, 11 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> 1. Disabled Pre-amplifier in equalizer if it is not supported by phonon.
> 2. Fixed top and bottom labels of first slider. Earlier band name label was 
> at the top and slider value label was at the bottom for first slider.
> 3. Removed an extra semicolon EqualizerDialog.h .
> Note : I have made an assumption that if at all preamp is present then it 
> will the first element of Effect Parameter list.
> 
> 
> This addresses bug 301311.
> https://bugs.kde.org/show_bug.cgi?id=301311
> 
> 
> Diffs
> -
> 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 58d7360 
>   src/dialogs/EqualizerDialog.h fd9032b 
>   src/dialogs/EqualizerDialog.cpp 7d62e10 
>   src/dialogs/EqualizerDialog.ui 43b0187 
> 
> Diff: http://git.reviewboard.kde.org/r/108995/diff/
> 
> 
> Testing
> ---
> 
> All unit test cases passed.
> Note : I have tested it with gstreamer only. Xine phonon keep crashing on my 
> PC.
> 
> 
> File Attachments
> 
> 
> Equalizer snapshot
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/02/17/equalizer.png
> 
> 
> Thanks,
> 
> Harsh Gupta
> 
>

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

Re: Review Request 108995: FIX Pre-amplifier in equalizer doesn't work

2013-03-28 Thread Harsh Gupta 

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



src/EngineController.cpp


How am I suppose to stage changes in a line ( line 791 ) in which a 
variable is first renamed and then get deleted ? Should I commit all the 
changes again in order to make two different patches ?


- Harsh Gupta


On March 14, 2013, 11 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated March 14, 2013, 11 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> 1. Disabled Pre-amplifier in equalizer if it is not supported by phonon.
> 2. Fixed top and bottom labels of first slider. Earlier band name label was 
> at the top and slider value label was at the bottom for first slider.
> 3. Removed an extra semicolon EqualizerDialog.h .
> Note : I have made an assumption that if at all preamp is present then it 
> will the first element of Effect Parameter list.
> 
> 
> This addresses bug 301311.
> https://bugs.kde.org/show_bug.cgi?id=301311
> 
> 
> Diffs
> -
> 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 58d7360 
>   src/dialogs/EqualizerDialog.h fd9032b 
>   src/dialogs/EqualizerDialog.cpp 7d62e10 
>   src/dialogs/EqualizerDialog.ui 43b0187 
> 
> Diff: http://git.reviewboard.kde.org/r/108995/diff/
> 
> 
> Testing
> ---
> 
> All unit test cases passed.
> Note : I have tested it with gstreamer only. Xine phonon keep crashing on my 
> PC.
> 
> 
> File Attachments
> 
> 
> Equalizer snapshot
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/02/17/equalizer.png
> 
> 
> Thanks,
> 
> Harsh Gupta
> 
>

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

Re: Review Request 109470: A patch which allows us to distinguish between cached lyrics and lyrics newly downloaded from the internet.

2013-03-28 Thread Matěj Laitl 

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


Is there a wish request or any other motivation to implement this feature? I 
don't see many reasons why user should care that the lyrics have been just 
downloaded or already cached. On the other hand, user may want to know whether 
lyrics have been previously edited by her. Additionally, some file formats 
allow storing lyrics in tags, it would be nice if Amarok supported it. (then it 
would make sense to show her what is the source of lyrics being shown)

- Matěj Laitl


On March 27, 2013, 7:22 p.m., mayank jha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109470/
> ---
> 
> (Updated March 27, 2013, 7:22 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> It required modifications, when there is no change in the lyrics downloaded 
> and lyrics retrieved from cache the title display of the lyrics browser 
> changes to "Cached Lyrics" from "Lyrics" so we can tell the difference 
> between old and new. 
> 
> 
> Diffs
> -
> 
>   src/context/engines/lyrics/LyricsEngine.cpp 2befa91 
>   src/context/applets/lyrics/LyricsApplet.cpp 2394964 
>   src/context/engines/lyrics/LyricsEngine.h b187b73 
> 
> Diff: http://git.reviewboard.kde.org/r/109470/diff/
> 
> 
> Testing
> ---
> 
> Its working fine!
> 
> 
> Thanks,
> 
> mayank jha
> 
>

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