<table><tr><td style="">kossebau added a comment.
</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/D10972">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D10972#226088" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D10972#226088</a>, <a href="https://phabricator.kde.org/p/rkflx/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@rkflx</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Thanks so much, Friedrich. Latest changes work great, only one more small thing found (but solvable, I guess).</p>
<p>I know I have been asking for a lot in this review, but I think users will appreciate the polish. After editing the commit message, let's land this ;)</p></div>
</blockquote>
<p>I also appreciated your feedback (incl. the extra effort to get this done in time for freeze), as it gave me the feeling this is a welcome feature and at least for now seems useful also to you :)</p>
<p>Will edit the commit message into something, do you want to review the text first? Would text follwing the one of <a href="https://phabricator.kde.org/D11250" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D11250</a> work for you?</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>There's now also <a href="https://phabricator.kde.org/D10972" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D10972: [RFC] Exposing slideshow to MPRIS controllers</a> where I added some of the things which could be solved later, so they are not forgotten and interested contributors can jump right in. Feel free to add to this (but please keep it concise) or add subtasks.</p></blockquote>
<p>Okay. Might make sense to copy the phab T numbers then in the respective code comments, so people can find things both ways?</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/D10972#inline-55643">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rkflx</span> wrote in <span style="color: #4b4d51; font-weight: bold;">mprismediaplayer2.cpp:82</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This is the only part which prevents compiling with Qt 5.6 as found on Leap 42.3 and elsewhere, which would be kinda nice.</p>
<p style="padding: 0; margin: 8px;">Are there any alternatives? Or could we return <tt style="background: #ebebeb; font-size: 13px;">QString()</tt> for older Qts? The basic functionality still seemed fine to me when testing, so perhaps this is not strictly needed?</p>
<p style="padding: 0; margin: 8px;">Otherwise we'd have to bite the bullet and bump the Qt version to 5.7.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ah, missed the Qt 5.6 min dep, right.</p>
<p style="padding: 0; margin: 8px;"><a href="https://specifications.freedesktop.org/mpris-spec/latest/Media_Player.html#Property:DesktopEntry" class="remarkup-link" target="_blank" rel="noreferrer">https://specifications.freedesktop.org/mpris-spec/latest/Media_Player.html#Property:DesktopEntry</a> says that "[t]This property is optional.".</p>
<p style="padding: 0; margin: 8px;">So I would simply "if version > 5.6" this part, okay?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R260 Gwenview</div></div></div><br /><div><strong>BRANCH</strong><div><div>addmprisservice</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D10972">https://phabricator.kde.org/D10972</a></div></div><br /><div><strong>To: </strong>kossebau, Gwenview, rkflx<br /><strong>Cc: </strong>mtijink, ngraham, nicolasfella, KDE Connect, rkflx, broulik<br /></div>