<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/129695/">https://git.reviewboard.kde.org/r/129695/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Декември 23rd, 2016, 2:07 след обяд EET, <b>Harald Sitter</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;">Please run <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">rbt post -d</code> to try and get some debug information as to why reviewboard keeps rejecting your diffs. I'd like to take this to the sysadmins to get it fixed. Not being able to use the review software defeats the point of having review software. For the time being you can post your diffs on phabricator manually, so we can still discuss the diffs easily. https://phabricator.kde.org/differential/diff/create/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As for the diff itself:
Awesome change!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Functional problems:
- Play a video -> go to play media -> switch to an audio file -> preview widget still has a black background in the loadView (or white sometimes). I am not entirely sure why that is happening now and did not happen before, I am guessing this is actually a left over problem from the kf5 port. As an easy fix I think you can call setThumbnail on the loadView with a nullptr when entering stopped state.
- When you play a file -> go to play media -> now stop playback (s shortcut or menu) -> you'll now get toggled into the view where nothing happens. What it should do is stay on the loadview.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Style problems:
- <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">m_currentWidget = nullptr;</code> please remove that and use the init list of the constructor to init the pointer to null. no point creating a qwidget and replacing it again with a nullptr
- the connect statements have excess spaces
- the connect statement in videowindow.cpp should be changed to use c++11 addresss-of style rather than SIGNAL()
- the ifs in stateChange.cpp need spaces before the opening bracket</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;">Style is mixing, old used style if( x ), one line statement without brasses {}, does not match new style i prefer to <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">adjust</em> patches to that style. About me it's better than mixing it.</p></pre>
<br />
<p>- Anthony</p>
<br />
<p>On Декември 23rd, 2016, 8:16 преди обяд EET, Anthony Fieroni 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 KDE Multimedia and Harald Sitter.</div>
<div>By Anthony Fieroni.</div>
<p style="color: grey;"><i>Updated Дек. 23, 2016, 8:16 преди обяд</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
dragon
</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;">Patch was rejected from reviewboard, see attached file and link to pastebin for pretty format https://paste.kde.org/pm1yzkjdl</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;">Fixed one frame blinking audioview before video starts, it's because information (hasVideo) is not resolved immediately after a media object gets a new source.
Tested scenarios:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Play video file -> videoview -> wait to auto end -> loadview
Play video file -> videoview -> press stop -> loadview
Play video file -> videoview -> press Play media (preview) -> wait to end -> loadview
Play video file -> videoview -> press Play media (preview) -> press stop -> loadview</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Play audio file -> audioview -> wait to auto end -> loadview
Play audio file -> audioview -> press stop -> loadview
Play audio file -> audioview -> press Play media -> wait to end -> loadview
Play audio file -> audioview -> press Play media -> press stop -> loadview</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/app/recentlyPlayedList.cpp <span style="color: grey">(2c25e7f)</span></li>
<li>src/app/stateChange.cpp <span style="color: grey">(7bf4038)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129695/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/12/23/87b20eea-30b9-4a6c-8915-bfbc0f4f3cd8__simple.patch">real patch</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>