Re: Review Request 108995: FIX Pre-amplifier in equalizer doesn't work
--- 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
On April 14, 2013, 7:08 p.m., Matěj Laitl wrote: src/dialogs/EqualizerDialog.cpp, lines 100-102 http://git.reviewboard.kde.org/r/108995/diff/4/?file=138625#file138625line100 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
On March 29, 2013, 12:03 a.m., Harsh Gupta wrote: src/EngineController.cpp, line 793 http://git.reviewboard.kde.org/r/108995/diff/2/?file=119751#file119751line793 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
On April 14, 2013, 1:38 p.m., Matěj Laitl wrote: src/dialogs/EqualizerDialog.cpp, lines 100-102 http://git.reviewboard.kde.org/r/108995/diff/4/?file=138625#file138625line100 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
On April 14, 2013, 7:08 p.m., Matěj Laitl wrote: src/dialogs/EqualizerDialog.cpp, lines 100-102 http://git.reviewboard.kde.org/r/108995/diff/4/?file=138625#file138625line100 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
--- 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
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108995/#review31016
---
src/EngineController.cpp
http://git.reviewboard.kde.org/r/108995/#comment23055
This is actually superfuous - you add the entry for preamp in the foreach
loop below.
src/EngineController.cpp
http://git.reviewboard.kde.org/r/108995/#comment23056
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
http://git.reviewboard.kde.org/r/108995/#comment23057
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
On March 28, 2013, 6:33 p.m., Harsh Gupta wrote: src/EngineController.cpp, line 793 http://git.reviewboard.kde.org/r/108995/diff/2/?file=119751#file119751line793 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
--- 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
--- 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
On March 29, 2013, 12:03 a.m., Harsh Gupta wrote: src/EngineController.cpp, line 793 http://git.reviewboard.kde.org/r/108995/diff/2/?file=119751#file119751line793 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
On March 28, 2013, 6:33 p.m., Harsh Gupta wrote: src/EngineController.cpp, line 793 http://git.reviewboard.kde.org/r/108995/diff/2/?file=119751#file119751line793 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
--- 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 http://git.reviewboard.kde.org/r/108995/#comment22340 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 http://git.reviewboard.kde.org/r/108995/#comment22339 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
--- 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
--- 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
--- 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 http://git.reviewboard.kde.org/r/108995/#comment21035 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 http://git.reviewboard.kde.org/r/108995/#comment21036 The assignment to scaledVal has no effect here, just the mEqParNewIt.next() has. src/dialogs/EqualizerDialog.h http://git.reviewboard.kde.org/r/108995/#comment21039 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 http://git.reviewboard.kde.org/r/108995/#comment21038 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
Review Request 108995: FIX Pre-amplifier in equalizer doesn't work
--- 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
Re: Review Request 108995: FIX Pre-amplifier in equalizer doesn't work
--- 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
