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
> 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, 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
--- 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
--- 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
> 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) 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
> 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. 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
--- 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 109585: FIX UI doesn't say I have to run Moodbar generator manually. (BR 289483)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109585/ --- (Updated March 29, 2013, 1:01 a.m.) Review request for Amarok and Myriam Schweingruber. Changes --- Fixed all issues. Description --- Added a label in moodbar options saying moodbar files must be generated manually. This addresses bug 289483. https://bugs.kde.org/show_bug.cgi?id=289483 Diffs (updated) - ChangeLog 284f188 src/configdialog/dialogs/GeneralConfig.ui 4e33f64 Diff: http://git.reviewboard.kde.org/r/109585/diff/ Testing --- All test case passed. File Attachments Screenshot http://git.reviewboard.kde.org/media/uploaded/files/2013/03/19/moodbar.png Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
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
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108995/#review30021 --- src/EngineController.cpp <http://git.reviewboard.kde.org/r/108995/#comment22381> 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 109585: FIX UI doesn't say I have to run Moodbar generator manually. (BR 289483)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109585/ --- (Updated March 19, 2013, 11:28 p.m.) Review request for Amarok and Myriam Schweingruber. Description --- Added a label in moodbar options saying moodbar files must be generated manually. This addresses bug 289483. https://bugs.kde.org/show_bug.cgi?id=289483 Diffs - ChangeLog 284f188 src/configdialog/dialogs/GeneralConfig.ui 4e33f64 Diff: http://git.reviewboard.kde.org/r/109585/diff/ Testing --- All test case passed. File Attachments (updated) Screenshot http://git.reviewboard.kde.org/media/uploaded/files/2013/03/19/moodbar.png Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request 109585: FIX UI doesn't say I have to run Moodbar generator manually. (BR 289483)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109585/ --- Review request for Amarok and Myriam Schweingruber. Description --- Added a label in moodbar options saying moodbar files must be generated manually. This addresses bug 289483. https://bugs.kde.org/show_bug.cgi?id=289483 Diffs - ChangeLog 284f188 src/configdialog/dialogs/GeneralConfig.ui 4e33f64 Diff: http://git.reviewboard.kde.org/r/109585/diff/ Testing --- All test case passed. 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
> On Feb. 26, 2013, 4:25 p.m., Matěj Laitl wrote: > > src/dialogs/EqualizerDialog.cpp, lines 99-119 > > <http://git.reviewboard.kde.org/r/108995/diff/1/?file=114229#file114229line99> > > > > 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
--- 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
--- 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 108716: FIX Randomize playlist with Ctrl+H
> On Feb. 10, 2013, 7:34 p.m., Commit Hook wrote: > > This review has been submitted with commit > > 62ce5195cfe30b78ec3e234f1d1f5dbed7534a4b by Mat?j Laitl on behalf of Harsh > > Gupta to branch master. Thanks :) - Harsh --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/#review27124 --- On Feb. 10, 2013, 7:16 p.m., Harsh Gupta wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/108716/ > --- > > (Updated Feb. 10, 2013, 7:16 p.m.) > > > Review request for Amarok. > > > Description > --- > > Added a shortcut button for shuffle with default shortcut as Ctrl + H. > Created a slot shuffleTrack() in mainWindow.cpp which is triggered on > pressing Ctrl + H > > > This addresses bug 208061. > https://bugs.kde.org/show_bug.cgi?id=208061 > > > Diffs > - > > ChangeLog 139ffa5 > src/MainWindow.h 4b23679 > src/MainWindow.cpp 8587784 > > Diff: http://git.reviewboard.kde.org/r/108716/diff/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Harsh Gupta > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108716: FIX Randomize playlist with Ctrl+H
> On Feb. 10, 2013, 4:40 p.m., Matěj Laitl wrote: > > The ChangeLog entry from v3 seems to be absent in v4... Oops ! I will fix it. - Harsh --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/#review27117 --- On Feb. 10, 2013, 7:16 p.m., Harsh Gupta wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/108716/ > --- > > (Updated Feb. 10, 2013, 7:16 p.m.) > > > Review request for Amarok. > > > Description > --- > > Added a shortcut button for shuffle with default shortcut as Ctrl + H. > Created a slot shuffleTrack() in mainWindow.cpp which is triggered on > pressing Ctrl + H > > > This addresses bug 208061. > https://bugs.kde.org/show_bug.cgi?id=208061 > > > Diffs > - > > ChangeLog 139ffa5 > src/MainWindow.h 4b23679 > src/MainWindow.cpp 8587784 > > Diff: http://git.reviewboard.kde.org/r/108716/diff/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Harsh Gupta > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108716: FIX Randomize playlist with Ctrl+H
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/ --- (Updated Feb. 10, 2013, 7:16 p.m.) Review request for Amarok. Changes --- Added Changelog entry. Description --- Added a shortcut button for shuffle with default shortcut as Ctrl + H. Created a slot shuffleTrack() in mainWindow.cpp which is triggered on pressing Ctrl + H This addresses bug 208061. https://bugs.kde.org/show_bug.cgi?id=208061 Diffs (updated) - ChangeLog 139ffa5 src/MainWindow.h 4b23679 src/MainWindow.cpp 8587784 Diff: http://git.reviewboard.kde.org/r/108716/diff/ Testing --- All tests passed. Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108716: FIX Randomize playlist with Ctrl+H
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/ --- (Updated Feb. 10, 2013, 1:44 p.m.) Review request for Amarok. Changes --- Fixed almost everything. Please let me know if I have missed anything. Description --- Added a shortcut button for shuffle with default shortcut as Ctrl + H. Created a slot shuffleTrack() in mainWindow.cpp which is triggered on pressing Ctrl + H This addresses bug 208061. https://bugs.kde.org/show_bug.cgi?id=208061 Diffs (updated) - src/MainWindow.h 4b23679 src/MainWindow.cpp 8587784 Diff: http://git.reviewboard.kde.org/r/108716/diff/ Testing --- All tests passed. Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108716: FIX Randomize playlist with Ctrl+H
> On Feb. 9, 2013, 5:36 p.m., Matěj Laitl wrote: > > src/MainWindow.cpp, line 836 > > <http://git.reviewboard.kde.org/r/108716/diff/2/?file=113315#file113315line836> > > > > Amarok coding style says: > > > > connect( action, SIGNAL(triggered(bool)), this, > > SLOT(slotShufflePlaylist()) ); > > > > (note that SIGNAL/SLOT macros are the only exception when there are no > > spaces around arguments) Thanks of pointing out minor details. I have fixed all the above things but in the process I some how deleted one patch file. So I tried to create patch using "git format-patch commit-tag --stdout > patchfile" but it appears that there is something wrong with the patch. Please help. Thanks again. - Harsh --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/#review27059 ------- On Feb. 10, 2013, 2:21 a.m., Harsh Gupta wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/108716/ > --- > > (Updated Feb. 10, 2013, 2:21 a.m.) > > > Review request for Amarok. > > > Description > --- > > Added a shortcut button for shuffle with default shortcut as Ctrl + H. > Created a slot shuffleTrack() in mainWindow.cpp which is triggered on > pressing Ctrl + H > > > This addresses bug 208061. > https://bugs.kde.org/show_bug.cgi?id=208061 > > > Diffs > - > > src/MainWindow.cpp 8587784 > src/MainWindow.cpp 8587784 > src/MainWindow.h 4b23679 > ChangeLog 139ffa5 > ChangeLog 749ed6d > > Diff: http://git.reviewboard.kde.org/r/108716/diff/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Harsh Gupta > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108716: FIX Randomize playlist with Ctrl+H
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/ --- (Updated Feb. 10, 2013, 2:21 a.m.) Review request for Amarok. Changes --- Looks like I messed up my diffs. Someone please help me ! ( I have made all the required changes successfully ) Description --- Added a shortcut button for shuffle with default shortcut as Ctrl + H. Created a slot shuffleTrack() in mainWindow.cpp which is triggered on pressing Ctrl + H This addresses bug 208061. https://bugs.kde.org/show_bug.cgi?id=208061 Diffs (updated) - src/MainWindow.cpp 8587784 src/MainWindow.cpp 8587784 src/MainWindow.h 4b23679 ChangeLog 139ffa5 ChangeLog 749ed6d Diff: http://git.reviewboard.kde.org/r/108716/diff/ Testing --- All tests passed. Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108716: FIX Randomize playlist with Ctrl+H
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/ --- (Updated Feb. 9, 2013, 12:33 p.m.) Review request for Amarok. Changes --- Fixed bad name. Description --- Added a shortcut button for shuffle with default shortcut as Ctrl + H. Created a slot shuffleTrack() in mainWindow.cpp which is triggered on pressing Ctrl + H Diffs (updated) - src/MainWindow.h 4b23679 src/MainWindow.cpp 8587784 Diff: http://git.reviewboard.kde.org/r/108716/diff/ Testing --- All tests passed. Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108716: FIX Randomize playlist with Ctrl+H
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/ --- (Updated Feb. 3, 2013, 10:04 p.m.) Review request for Amarok. Description --- Added a shortcut button for shuffle with default shortcut as Ctrl + H. Created a slot shuffleTrack() in mainWindow.cpp which is triggered on pressing Ctrl + H Diffs - src/MainWindow.h 4b23679 src/MainWindow.cpp 8587784 Diff: http://git.reviewboard.kde.org/r/108716/diff/ Testing (updated) --- All tests passed. File Attachments http://git.reviewboard.kde.org/media/uploaded/files/2013/02/02/0001-Randomize-playlist-with-Ctrl-H.patch Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request 108716: FIX Randomize playlist with Ctrl+H
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/ --- Review request for Amarok. Description --- Added a shortcut button for shuffle with default shortcut as Ctrl + H. Created a slot shuffleTrack() in mainWindow.cpp which is triggered on pressing Ctrl + H Diffs - src/MainWindow.h 4b23679 src/MainWindow.cpp 8587784 Diff: http://git.reviewboard.kde.org/r/108716/diff/ Testing --- File Attachments http://git.reviewboard.kde.org/media/uploaded/files/2013/02/02/0001-Randomize-playlist-with-Ctrl-H.patch Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
