<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/124773/">https://git.reviewboard.kde.org/r/124773/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 13th, 2015, 6:59 p.m. CEST, <b>Olivier Churlaud</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding the code, everything seems fine. Now the real question is about the shortcuts. Are they really good? Logical?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not the one to judge on this. But a Shift+E for instance might have bad repercussions (if you type Editors in the search field, what happens? Is the slot triggered?)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If Mamarok says it's ok, then ship it.</p></pre>
</blockquote>
<p>On September 13th, 2015, 8:45 p.m. CEST, <b>Aditya Dev Sharma</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well yes, there could be bad repurcussions for having same shortcuts. But that seems to be different for different people having different systems. For eg. I could use a shortcut already used above, for launching say yakuake. The user could still change these shortcuts from the GUI. So making absolutely unique shortcuts is difficult. I just took care that the shortcuts did not match any previous Global Shortcuts or even the ones used before.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll wait for Mamarok :)</p></pre>
</blockquote>
<p>On September 14th, 2015, 7:39 p.m. CEST, <b>Myriam Schweingruber</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Did you check the KDE Human Interface Guidelines here as well? -> https://techbase.kde.org/Projects/Usability/HIG/Keyboard_Shortcuts</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am still a bit puzzled that you use Shift+Ctrl+E to export a playlist (when Ctrl+E triggers Track editing) and then Shift+E to show the Equalizer, doesn't seem that logical to me...
With logical I mean that the same shortcuts in variation should be useds to enable and disable the same things, so if the letter E is used for an Edit function, it should not be used for something totally different in another combination. If the shortcuts are not logical I am not sure those are really useful. A combination with S to show something might be a good path to explore.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On the other hand our existing shortcuts don't seem to be very logical either <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">sigh</em>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">About different people having different systems: we don't cater for other desktop systems, for us what is relevant is what is in use in KDE and Amarok. Currenlty GNOME for example uses different shortcuts already, so we really can't take this into consideration anyway. If our shortcuts conflict with other people's private selections, it is again not something we should care, as they can change their setting quite easily.</p></pre>
</blockquote>
<p>On September 14th, 2015, 7:44 p.m. CEST, <b>Olivier Churlaud</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm wondering why we add this shortcuts? Every one has their own preference, as you said Myriam, and one can change it. So why don't we let it empty and the users choose directly what fits the best?</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The idea was to make Amarok completely navigable by keyboard: https://bugs.kde.org/show_bug.cgi?id=300979 as a Junior Job for newcomers to get familiar with our code. I personally think this is not really useful to implement and a waste of ressources, but I didn't create those Junior Jobs</p></pre>
<br />
<p>- Myriam</p>
<br />
<p>On September 6th, 2015, 7:30 p.m. CEST, Aditya Dev Sharma wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Amarok, Myriam Schweingruber and Bart Cerneels.</div>
<div>By Aditya Dev Sharma.</div>
<p style="color: grey;"><i>Updated Sept. 6, 2015, 7:30 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=300979">300979</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
amarok
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">JJ : Bug - 300979 : Patch to add some shortcuts for various actions in createActions() in MainWindow</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Built and tested.
Removed the shortcuts that matched either the Amarok Global Shortcuts or the Standard Shortcuts of KDE.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Clear Playlist : CTRL + SHIFT + C ---------------> SLOT(clear() )
Add Stream : CTRL + SHIFT + J ---------------> SLOT(slotAddStream() )
Export Playlits : CTRL + SHIFT + E ---------------> SLOT(exportPlaylist() )
Bookmarks Manager : CTRL + I ---------------> SLOT(slotShowBookmarkManager() )
Equalizer : SHIFT + E ---------------> SLOT(slotShowEqualizer() )
Cover Manager : CTRL + ALT + C ---------------> SLOT(slotShowCoverManager() )
Synchronize Statistics : SHIFT + S ---------------> SLOT(synchronize() )</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Bookmark Media Sources View : SHIFT + B ----------> SLOT(bookmarkCurrentBrowserView() )
Bookmark Playlist Setup : ALT + B ---------------> bookmarkCurrentPlaylistView() )
Bookmark Context Applets : CTRL + SHIFT + B ------> bookmarkCurrentContextView() )
Disable Dynamic Playlist : ALT + SHIFT + D -------> connected inside dynamic playlist category</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">About Amarok : ALT + F10 -----------------> SLOT(showAbout())
Diagnostics : ALT + F8 -----------------> SLOT(slotShowDiagnosticsDialog())
Report bug : ALT + F9 -----------------> SLOT(showReportBug())</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/MainWindow.cpp <span style="color: grey">(22fb8ce)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124773/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>