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

2013-04-15 Thread Matěj Laitl 

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

Ship it!


Good to go, but it seems that no phonon equalizer effect now supports pre-amp, 
so the second case is hard to fix.

Harsh, also please update ~/.gitconfig to mention your real name and surname, 
not a nickname.

- Matěj Laitl


On April 15, 2013, 8:09 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 15, 2013, 8:09 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
> -
> 
>   ChangeLog a278be3 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 28fb256 
>   src/dialogs/EqualizerDialog.cpp f42a033 
>   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-04-15 Thread Commit Hook 

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

(Updated April 15, 2013, 8:09 p.m.)


Status
--

This change has been marked as submitted.


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
-

  ChangeLog a278be3 
  src/EngineController.h 5de4beb 
  src/EngineController.cpp 28fb256 
  src/dialogs/EqualizerDialog.cpp f42a033 
  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-04-15 Thread Commit Hook 

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


This review has been submitted with commit 
e344f608faf236e8faa455e0516f55d4ffeee80c by Matěj Laitl on behalf of Harsh 
Gupta to branch master.

- Commit Hook


On April 15, 2013, 3:55 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 15, 2013, 3:55 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
> -
> 
>   ChangeLog a278be3 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 28fb256 
>   src/dialogs/EqualizerDialog.cpp f42a033 
>   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-04-15 Thread Matěj Laitl 


> On April 14, 2013, 1:38 p.m., Matěj Laitl wrote:
> > src/dialogs/EqualizerDialog.cpp, lines 100-102
> > 
> >
> > code style: no space between if and (
> 
> Harsh Gupta wrote:
> ooppss !!!
> 
> Matěj Laitl wrote:
> I'll fix it while committin, no need to update the patch.
> 
> Harsh Gupta wrote:
> Actually I have already done that in the next patch

Oh I see, sorry, I was confused.


- Matěj


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


On April 15, 2013, 3:55 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 15, 2013, 3:55 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
> -
> 
>   ChangeLog a278be3 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 28fb256 
>   src/dialogs/EqualizerDialog.cpp f42a033 
>   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-04-15 Thread Harsh Gupta 


> On April 14, 2013, 7:08 p.m., Matěj Laitl wrote:
> > src/dialogs/EqualizerDialog.cpp, lines 100-102
> > 
> >
> > code style: no space between if and (
> 
> Harsh Gupta wrote:
> ooppss !!!
> 
> Matěj Laitl wrote:
> I'll fix it while committin, no need to update the patch.

Actually I have already done that in the next patch


- Harsh


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


On April 15, 2013, 9:25 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 15, 2013, 9:25 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
> -
> 
>   ChangeLog a278be3 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 28fb256 
>   src/dialogs/EqualizerDialog.cpp f42a033 
>   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-04-15 Thread Matěj Laitl 


> On April 14, 2013, 1:38 p.m., Matěj Laitl wrote:
> > src/dialogs/EqualizerDialog.cpp, lines 100-102
> > 
> >
> > code style: no space between if and (
> 
> Harsh Gupta wrote:
> ooppss !!!

I'll fix it while committin, no need to update the patch.


- Matěj


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


On April 15, 2013, 3:55 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 15, 2013, 3:55 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
> -
> 
>   ChangeLog a278be3 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 28fb256 
>   src/dialogs/EqualizerDialog.cpp f42a033 
>   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-04-15 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 ?
> 
> Harsh Gupta wrote:
> Oops... sorry for poor tagging !!! (first time  :P)
> Line no. is actually 791.
> 
> Matěj Laitl wrote:
> `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.
> 
> Harsh Gupta wrote:
> Should I submit both the patches in this review request one by one ?
> PS: Please ignore my latest patch. I was expecting two different patches 
> (one as a parent diff) but review board merged the two patches.
> 
> Matěj Laitl wrote:
> > Should I submit both the patches in this review request one by one?
> 
> Please submit the renaming patch as new review request here. Indicate in 
> this review request that it depends on the renaming one. (and optionally 
> update it so that it applied on top of the renaming patch - but this seems 
> already done)
> 
> > I was just wondering what should I write in the Changelog file?
> 
> BUGFIXES:
>  * Pre-aplifier in equalizer is now only enabled if it is actually 
> supported; patch by Harsh Gupta. (BR 301311)
> 
> Harsh Gupta wrote:
> I have created a new review request for the renaming patch. Should I wait 
> for that patch to get shipped ? 
> 
> PS : Sorry for the late response. University projects kept me occupied :(
> 
> Matěj Laitl wrote:
> > I have created a new review request for the renaming patch. Should I 
> wait for that patch to get shipped?
> 
> It already is. :-) 
> 
> > PS: Sorry for the late response. University projects kept me occupied :(
> 
> No problem, glad to know you weren't hit by a bus. Actual review below.

:D


- Harsh


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


On April 15, 2013, 9:25 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 15, 2013, 9:25 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
> -
> 
>   ChangeLog a278be3 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 28fb256 
>   src/dialogs/EqualizerDialog.cpp f42a033 
>   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-04-15 Thread Harsh Gupta 


> On April 14, 2013, 7:08 p.m., Matěj Laitl wrote:
> > src/dialogs/EqualizerDialog.cpp, lines 100-102
> > 
> >
> > code style: no space between if and (

ooppss !!!


- Harsh


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


On April 15, 2013, 9:25 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 15, 2013, 9:25 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
> -
> 
>   ChangeLog a278be3 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 28fb256 
>   src/dialogs/EqualizerDialog.cpp f42a033 
>   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-04-15 Thread Harsh Gupta 

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

(Updated April 15, 2013, 9:25 p.m.)


Review request for Amarok.


Changes
---

All issues resolved.


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

  ChangeLog a278be3 
  src/EngineController.h 5de4beb 
  src/EngineController.cpp 28fb256 
  src/dialogs/EqualizerDialog.cpp f42a033 
  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-04-14 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.
> 
> Matěj Laitl wrote:
> `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.
> 
> Harsh Gupta wrote:
> Should I submit both the patches in this review request one by one ?
> PS: Please ignore my latest patch. I was expecting two different patches 
> (one as a parent diff) but review board merged the two patches.
> 
> Matěj Laitl wrote:
> > Should I submit both the patches in this review request one by one?
> 
> Please submit the renaming patch as new review request here. Indicate in 
> this review request that it depends on the renaming one. (and optionally 
> update it so that it applied on top of the renaming patch - but this seems 
> already done)
> 
> > I was just wondering what should I write in the Changelog file?
> 
> BUGFIXES:
>  * Pre-aplifier in equalizer is now only enabled if it is actually 
> supported; patch by Harsh Gupta. (BR 301311)
> 
> Harsh Gupta wrote:
> I have created a new review request for the renaming patch. Should I wait 
> for that patch to get shipped ? 
> 
> PS : Sorry for the late response. University projects kept me occupied :(

> I have created a new review request for the renaming patch. Should I wait for 
> that patch to get shipped?

It already is. :-) 

> PS: Sorry for the late response. University projects kept me occupied :(

No problem, glad to know you weren't hit by a bus. Actual review below.


- Matěj


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


On April 13, 2013, 4:14 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 13, 2013, 4:14 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
> -
> 
>   ChangeLog a278be3 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 28fb256 
>   src/dialogs/EqualizerDialog.cpp f42a033 
>   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-04-14 Thread Matěj Laitl 

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



src/EngineController.cpp


This is actually superfuous - you add the entry for preamp in the foreach 
loop below.



src/EngineController.cpp


This actually warrants a simplification:
foreach( ... parameter, equalizerParameters )
{
if( parameter.name().contains( rx ) )
{
int hertz = rx.cap( 0 ).toInt();
if( hertz < 1000 )
...
else
...
} else {
bandFrequencies << parameter.name()
}
}



src/dialogs/EqualizerDialog.cpp


code style: no space between if and (


- Matěj Laitl


On April 13, 2013, 4:14 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 13, 2013, 4:14 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
> -
> 
>   ChangeLog a278be3 
>   src/EngineController.h 5de4beb 
>   src/EngineController.cpp 28fb256 
>   src/dialogs/EqualizerDialog.cpp f42a033 
>   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-04-13 Thread Harsh Gupta 

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

(Updated April 13, 2013, 9:44 p.m.)


Review request for Amarok.


Changes
---

Second patch :  Bug fixed.


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

  ChangeLog a278be3 
  src/EngineController.h 5de4beb 
  src/EngineController.cpp 28fb256 
  src/dialogs/EqualizerDialog.cpp f42a033 
  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-04-12 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 ?
> 
> Harsh Gupta wrote:
> Oops... sorry for poor tagging !!! (first time  :P)
> Line no. is actually 791.
> 
> Matěj Laitl wrote:
> `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.
> 
> Harsh Gupta wrote:
> Should I submit both the patches in this review request one by one ?
> PS: Please ignore my latest patch. I was expecting two different patches 
> (one as a parent diff) but review board merged the two patches.
> 
> Matěj Laitl wrote:
> > Should I submit both the patches in this review request one by one?
> 
> Please submit the renaming patch as new review request here. Indicate in 
> this review request that it depends on the renaming one. (and optionally 
> update it so that it applied on top of the renaming patch - but this seems 
> already done)
> 
> > I was just wondering what should I write in the Changelog file?
> 
> BUGFIXES:
>  * Pre-aplifier in equalizer is now only enabled if it is actually 
> supported; patch by Harsh Gupta. (BR 301311)

I have created a new review request for the renaming patch. Should I wait for 
that patch to get shipped ? 

PS : Sorry for the late response. University projects kept me occupied :(


- Harsh


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


On April 1, 2013, 5:10 a.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 1, 2013, 5:10 a.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.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-04-11 Thread Matěj Laitl 

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


Ping.. While it may seem confusing, remarks for the latest version of the patch 
are in my comments for the version *before* the last one, sorry.

- Matěj Laitl


On March 31, 2013, 11:40 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated March 31, 2013, 11:40 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.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-04-02 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.
> 
> Matěj Laitl wrote:
> `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.
> 
> Harsh Gupta wrote:
> Should I submit both the patches in this review request one by one ?
> PS: Please ignore my latest patch. I was expecting two different patches 
> (one as a parent diff) but review board merged the two patches.

> Should I submit both the patches in this review request one by one?

Please submit the renaming patch as new review request here. Indicate in this 
review request that it depends on the renaming one. (and optionally update it 
so that it applied on top of the renaming patch - but this seems already done)

> I was just wondering what should I write in the Changelog file?

BUGFIXES:
 * Pre-aplifier in equalizer is now only enabled if it is actually supported; 
patch by Harsh Gupta. (BR 301311)


- Matěj


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


On March 31, 2013, 11:40 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated March 31, 2013, 11:40 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.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-31 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 ?
> 
> Harsh Gupta wrote:
> Oops... sorry for poor tagging !!! (first time  :P)
> Line no. is actually 791.
> 
> Matěj Laitl wrote:
> `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.

Should I submit both the patches in this review request one by one ?
PS: Please ignore my latest patch. I was expecting two different patches (one 
as a parent diff) but review board merged the two patches.


- Harsh


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


On April 1, 2013, 5:10 a.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated April 1, 2013, 5:10 a.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.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-31 Thread Harsh Gupta 

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

(Updated April 1, 2013, 5:10 a.m.)


Review request for Amarok.


Changes
---

Created two patches:
1. First patch : Fixed variable names.
2. Second patch : Fixed the bug.
Both the patches build.

I was just wondering what should I write in the Changelog file?


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

  src/EngineController.h 5de4beb 
  src/EngineController.cpp 58d7360 
  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 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 108995: FIX Pre-amplifier in equalizer doesn't work

2013-03-27 Thread Matěj Laitl 

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


Looks good, thanks! Just a remark or 2 below. Thanks also for the variable 
naming fixes.

We also encourage commits to be focused on just one thing. Could you please 
split this patch into 2, first one just changing the variable names of existing 
variables, second implementing the actual pre-amp change? I suggest you use git 
add -p (interactive adding files to git index) along with its s: split function 
and e: editing hunks to be added by hand.


src/EngineController.h


Hmm, please call it EQUALIZER_BANDS_COUNT and set it to 10, rather than 11. 
Then adapt the code.

Or rather, instead of macros, in C++ const variables are preferred. You 
should be able to achieve the same with:
static const s_equalizerBandsCount = 10;



src/EngineController.cpp


The equalizerParameters.isEmpty() is not needed here, the code would be 
no-op even if it is empty.


- Matěj Laitl


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-14 Thread Harsh Gupta 

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


Changes
---

Hard-coded preamp label in UI.
Renamed attribute names to match Amarok coding style.
Added a " #define NUM_EQ_PARAM 11 " in EngineController.h . Please suggest a 
better name for it.


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

  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-02-26 Thread Harsh Gupta 


> On Feb. 26, 2013, 4:25 p.m., Matěj Laitl wrote:
> > src/dialogs/EqualizerDialog.cpp, lines 99-119
> > 
> >
> > Ugly. Instead please:
> > a) check whether meqBandFrq (horrible variable name btw, not your 
> > fault) has 10 items or 11.
> > b) don't append preamp to mBands, mBandsValues, mBandsLabels in case it 
> > is not available.

Reason for appending preamp : Since number of sliders are hard coded in UI ( 11 
sliders ) I thought each slider should have some label.
Anyway I will remove preamp label and make first 10 sliders active while last 
slider is disabled.


- Harsh


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


On Feb. 18, 2013, 11:38 a.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated Feb. 18, 2013, 11:38 a.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/dialogs/EqualizerDialog.ui 43b0187 
>   src/EngineController.cpp 3577acf 
>   src/dialogs/EqualizerDialog.h fd9032b 
>   src/dialogs/EqualizerDialog.cpp 63d8209 
> 
> 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-02-26 Thread Matěj Laitl 

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


Hi, thanks for the patch and sorry for delays while reviewing it, I was 
preoccupied by other things lately. (Edward, Ralf, Teo, Bart, Sam, Sven: I'm 
not the only one who can review requests!)

Please resolve the remarks below before the patch can be merged.


src/EngineController.cpp


I think it is better to check whether mEqPar has 10 or 11 items.

This is not issue of this patch, but I would also welcome if some of the 
variables got renamed, for example mEqPar should be equalizerParameters etc, no 
need for cryptic abbreviations and "m" prefix.



src/EngineController.cpp


The assignment to scaledVal has no effect here, just the mEqParNewIt.next() 
has.



src/dialogs/EqualizerDialog.h


While you're at it, please rename attribute names to match Amarok coding 
style, e.g.:

m_valueScale
m_bands
m_bandValues  (note the removed s to make them more english)
m_bandLabels



src/dialogs/EqualizerDialog.cpp


Ugly. Instead please:
a) check whether meqBandFrq (horrible variable name btw, not your fault) 
has 10 items or 11.
b) don't append preamp to mBands, mBandsValues, mBandsLabels in case it is 
not available.


- Matěj Laitl


On Feb. 18, 2013, 6:08 a.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated Feb. 18, 2013, 6:08 a.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/dialogs/EqualizerDialog.ui 43b0187 
>   src/EngineController.cpp 3577acf 
>   src/dialogs/EqualizerDialog.h fd9032b 
>   src/dialogs/EqualizerDialog.cpp 63d8209 
> 
> 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-02-26 Thread Myriam Schweingruber 

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


Could a developer please look at this?

- Myriam Schweingruber


On Feb. 18, 2013, 6:08 a.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> ---
> 
> (Updated Feb. 18, 2013, 6:08 a.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/dialogs/EqualizerDialog.ui 43b0187 
>   src/EngineController.cpp 3577acf 
>   src/dialogs/EqualizerDialog.h fd9032b 
>   src/dialogs/EqualizerDialog.cpp 63d8209 
> 
> 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-02-17 Thread Harsh Gupta 

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

(Updated Feb. 18, 2013, 11:38 a.m.)


Review request for Amarok.


Description (updated)
---

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/dialogs/EqualizerDialog.ui 43b0187 
  src/EngineController.cpp 3577acf 
  src/dialogs/EqualizerDialog.h fd9032b 
  src/dialogs/EqualizerDialog.cpp 63d8209 

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

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

2013-02-17 Thread Harsh Gupta 

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

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 .


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


Diffs
-

  src/dialogs/EqualizerDialog.ui 43b0187 
  src/EngineController.cpp 3577acf 
  src/dialogs/EqualizerDialog.h fd9032b 
  src/dialogs/EqualizerDialog.cpp 63d8209 

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