<table><tr><td style="">nicolasfella requested changes to this revision.<br />nicolasfella added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D16279">View Revision</a></tr></table><br /><div><div><p>Great work!</p>
<p>I have a few cosmetic suggestions: Instead of saying "Sending file over KDE Connect" I'd say "Sending file to <devicename>". Then the second line should be the filename. The Jobtracker seems to pick that automatically from the info. If you remove the "To" line from the info the filename is shown as secon line. Don't ask me how exactly this works. The end result is:<br />
<a href="https://phabricator.kde.org/F6335741" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F6335741: Screenshot_20181017_211737.png</a></p>
<p>A few code nitpicks: <br />
You should remove the commented code and debug output. <br />
Please follow the coding style from the rest of the file. Opening { for methods go on the next line, for everything inside methods on the same line. I'll leave a few more comments as inline comments</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16279#inline-88532">View Inline</a><span style="color: #4b4d51; font-weight: bold;">uploadjob.cpp:63</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span> <span class="n">i18nc</span><span class="p">(</span><span style="color: #766510">"File transfer origin"</span><span class="p">,</span> <span style="color: #766510">"From"</span><span class="p">),</span> <span class="n">m_input</span><span class="p">.</span><span class="n">staticCast</span><span style="color: #aa2211"><</span><span class="n">QFile</span><span style="color: #aa2211">></span><span class="p">().</span><span class="n">data</span><span class="p">()</span><span style="color: #aa2211">-></span><span class="n">fileName</span><span class="p">()</span> <span class="p">},</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span> <span class="n">i18nc</span><span class="p">(</span><span style="color: #766510">"File transfer destination"</span><span class="p">,</span> <span style="color: #766510">"To"</span><span class="p">),</span> <span class="n">m_deviceName</span> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Use Daemon::instance()->getDevice(this->m_deviceId)->name() directly and remove m_deviceName</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16279#inline-88533">View Inline</a><span style="color: #4b4d51; font-weight: bold;">uploadjob.cpp:103</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">m_timer</span><span class="p">.</span><span class="n">isValid</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">m_timer</span><span class="p">.</span><span class="n">start</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Looks like too much indentation to me</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R224 KDE Connect</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D16279">https://phabricator.kde.org/D16279</a></div></div><br /><div><strong>To: </strong>eduisters, KDE Connect, nicolasfella<br /><strong>Cc: </strong>nicolasfella, kdeconnect, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, timothyc, jdvr, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, tctara, apol<br /></div>