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/#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 http://git.reviewboard.kde.org/r/108716/#comment20380 Nitpick: Amarok coding style says: m_playlistDock.data()-sortWidget()-addLevel( QString( Shuffle ) ); src/MainWindow.cpp http://git.reviewboard.kde.org/r/108716/#comment20381 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
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
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
On Feb. 9, 2013, 12:06 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) 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 starting-point` (where starting point 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
On Feb. 9, 2013, 12:06 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) 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 starting-point` (where starting point 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=your 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
