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

[Matěj Laitl]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130414/025d8853/attachment.html>