Re: Review Request 100321: Fix Inconsistencies with Organize Files Dialog when canceling Dialog

[Matěj Laitl]

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


Ralf, do you want to incorporate some of the suggestions into the new organize 
collection dialog? I agree that pressing "Update preset" should be permanent 
even if you hit the Cancel button afterwards. Additionally, there are some 
minor bugs with the new dialog (bad place to mention them here, but this is 
minor):
 * switching between advanced and basic breaks curly braces in the advanced 
preset and and also the preview (I use the curly braces a lot)
 * the "Update button" doesn't get enabled if you edit a preset in advanced mode

- Matěj Laitl


On Jan. 8, 2011, 12:12 p.m., Philipp Schmidt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100321/
> ---
> 
> (Updated Jan. 8, 2011, 12:12 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> Fixes two errors:
> 
> First: Presets are being saved explicitely, meaning they should persist even 
> when the Dialog is aborted/canceled.
> Second: The state of the Current Collection Directory is saved regardless of 
> whether the Dialog was accepted or canceled. IMO it should only be saved like 
> all other values when it is accepted.
> 
> 
> Diffs
> -
> 
>   ChangeLog 3c337d1 
>   src/dialogs/OrganizeCollectionDialog.cpp b7d7850 
> 
> Diff: http://git.reviewboard.kde.org/r/100321/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Philipp Schmidt
> 
>

___
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

[Matěj Laitl]

> On Feb. 9, 2013, 12:06 p.m., Matěj Laitl wrote:
> > src/MainWindow.cpp, line 836
> > 
> >
> > 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)
> 
> Harsh Gupta wrote:
> 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.
> 
> Matěj Laitl wrote:
> It looks like you've uploaded the difference between the second and third 
> patch rather than the difference between origin/master and v3 of your work. 
> If you work in git (good), it probably means that you created extra commit 
> for the third version of your patch. I strongly recommend you use a tool like 
> gitk to visualize changes in your git repository. All your work on this 
> feature should be squelched into a single patch - a comand like `git rebase 
> -i ` (where  is the upstream commit/branch 
> (most probably just origin/master)) can help you edit and merge existing 
> commits.
> 
> `git format patch orgin/master` is the right command to export commits 
> you have on top of the upstream code.

Also note there are nice tools to work with reviewboard from git, the package 
is called rbtools and contains command named post-review. A command to update 
this review request could be then:

post-review --guess-description --username= --parent=$(git 
merge-base HEAD origin/master) --open -r 108716


- Matěj


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


On Feb. 9, 2013, 8:51 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108716/
> ---
> 
> (Updated Feb. 9, 2013, 8:51 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
> -
> 
>   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

[Matěj Laitl]

> On Feb. 9, 2013, 12:06 p.m., Matěj Laitl wrote:
> > src/MainWindow.cpp, line 836
> > 
> >
> > 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)
> 
> Harsh Gupta wrote:
> 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.

It looks like you've uploaded the difference between the second and third patch 
rather than the difference between origin/master and v3 of your work. If you 
work in git (good), it probably means that you created extra commit for the 
third version of your patch. I strongly recommend you use a tool like gitk to 
visualize changes in your git repository. All your work on this feature should 
be squelched into a single patch - a comand like `git rebase -i 
` (where  is the upstream commit/branch (most 
probably just origin/master)) can help you edit and merge existing commits.

`git format patch orgin/master` is the right command to export commits you have 
on top of the upstream code.


- Matěj


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


On Feb. 9, 2013, 8:51 p.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108716/
> ---
> 
> (Updated Feb. 9, 2013, 8:51 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
> -
> 
>   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

[Harsh Gupta]

> On Feb. 9, 2013, 5:36 p.m., Matěj Laitl wrote:
> > src/MainWindow.cpp, line 836
> > 
> >
> > 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

[Harsh Gupta]

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

[Matěj Laitl]

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


The patch looks good now, just some last trivial things. (sorry about being 
this picky)

(optional) If you want to help even more, please add:
 * BUG: XX commit tag (see 
http://techbase.kde.org/Development/Git/Configuration#Commit_Template)
 * REVIEW: YY commit tag (see above)
 * ChangeLog entry (on top, under FEATURES, ensure there is (BR XX) tag)


src/MainWindow.cpp


Nitpick: Amarok coding style says:

m_playlistDock.data()->sortWidget()->addLevel( QString( "Shuffle" ) );



src/MainWindow.cpp


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)


- Matěj Laitl


On Feb. 9, 2013, 7:03 a.m., Harsh Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108716/
> ---
> 
> (Updated Feb. 9, 2013, 7:03 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
> 
> 
> Diffs
> -
> 
>   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