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
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/#review27117 --- The ChangeLog entry from v3 seems to be absent in v4... - Matěj Laitl On Feb. 10, 2013, 8:14 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, 8:14 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.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
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/#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
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/#review27047 --- Hi, thanks for the patch, looks good - just one badly chosen name. src/MainWindow.h http://git.reviewboard.kde.org/r/108716/#comment20375 This should be caller more desctiptively, perhaps slotShufflePlaylist() P.S.: there's no need to attach the copy of the patch as a file. - Matěj Laitl On Feb. 3, 2013, 4:34 p.m., Harsh Gupta wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108716/ --- (Updated Feb. 3, 2013, 4:34 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 --- 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
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