<table><tr><td style="">rkflx 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><p>Okay great, I think we are on the same page regarding implementing "something" now, and making it perfect later. The only thing I was wondering about was that you wanted to make the abstract mapping of the "pause" concept "too perfect", breaking simple user expectations. Thankfully that's working great now.</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>make this an optional build feature?</p></blockquote>
<p>Not sure whether Gwenview currently runs on Windows, but it's a nice thought. I'd say handle it just like Elisa does: Make this dependent on whether DBus is available. (A separate feature toggle could always be added later on if people ask for it.)</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>make this an optional feature toggable in the settings, where/how best?</p></blockquote>
<p>We are wary of adding too many config options. Did not check, but if JuK or Elisa don't have that as an extra option, we should also go without one (unless other <a href="https://phabricator.kde.org/tag/gwenview/" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">#gwenview</a> members think differently!?). We can revisit this decision later, if need be.</p>
<p>However, there is a slight twist which I only discovered recently: The mediacontroller applet is also visible on the lockscreen. For Gwenview's use case that's pretty useless, because the lockscreen simply hides any picture from the user. Moreover, rightfully people will complain we are exposing their photo collections to the public if all they did was running Gwenview in the background.</p>
<p>What do you think about detecting the lockscreen and disabling the feature then (just like KWallet can close itself in that case)? That would be easier than separately signalling something to the controllers. (Maybe there is even already some code handling inhibition related things, you might want to check that.)</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>any idea how to create unique track ids based on the media url?</p></blockquote>
<p>Would hashing the path work? I'd assume the ID should be unique only per MPRIS "session", i.e. renaming and moving files could result in different IDs.</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>how to provide thumbnails to the mpris controller via temp files whose url then can be passed in the metadata?</p></blockquote>
<p>If that means that Gwenview should compute a temporary thumbnail and write it to disk multiple times per second in case someone holds done the arrow key in either <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Browse</span></span></span> or <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">View</span></span></span> mode, that's a no-go. There are already thumbnails in the standard thumbnails directory which are shared with Dolphin (at least for most part, because there is more in-memory thumbnail handling happening which I haven't looked too deeply into yet, see also discussion in <a href="https://phabricator.kde.org/D9078" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D9078</a>). Controllers should just access that cache, or be told by Gwenview about the actual file location in that cache.</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>Still need to learn how videos are supported (so if with sound), but will add then once I found out. Hints from Gwenview members welcome :)</p></blockquote>
<p>Yup, playing videos with sounds should work. However, see comment below, full video support via MPRIS is probably material for later.</p>
<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#221146" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D10972#221146</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><a href="https://phabricator.kde.org/p/ngraham/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@ngraham</a> I think we should change the previous/next buttons back. My intention was to be more similar to Dolphin, but now that we add so much UI which has the media player type icons and terminology, the old version would fit better. Also, perhaps change the text from "stop" to "pause", to match the icon. Thoughts?</p></div>
</blockquote>
<p>No reaction so far, so I'll just open a Diff for that.</p>
<hr class="remarkup-hr" />
<p>As for your comments about Gwenview in general: I'm only trying to make sense of it and helping fix things, I'm not the author or anything. Gwenview should be very simple, and IMO at least some of your suggestions clash with that. The fundamental paradigm (which I agree needs more polishing to shine through) is this:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">2 basic modes: <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Browse</span></span></span> and <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">View</span></span></span>, switching between them is done by <kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">Enter</kbd> and <kbd title="Escape" style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">⎋</kbd> or toolbar buttons. (The <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Start Page</span></span></span> is the odd poster child, but we are thinking about upgrading it, i.e. allowing fullscreen also there.)</li>
<li class="remarkup-list-item">Both modes have a windowed and a fullscreen variant, you switch via <kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">F11</kbd> or the toolbar button.</li>
<li class="remarkup-list-item">Navigation is done by pressing a key or a toolbar button.</li>
<li class="remarkup-list-item">The slideshow feature is nothing more than a cute little bot pressing the forward button once in a while for you. It's opt-in and understated. There is a shortcut in the menu which goes to <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">View</span></span></span>, switches to <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Fullscreen</span></span></span> and activates the slideshow bot in one go. For consistency reasons <kbd title="Escape" style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">⎋</kbd> does not undo that, but activates its default action. Normal users are not expected to use keyboard shortcuts, for them the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Leave Fullscreen</span></span></span> button will work just fine.</li>
</ul>
<p>As for videos and their relation to pause/forward etc.: Yeah, we know about that one (see endless discussions on Phab and Bugzilla). It's a tricky problem and we'll tackle it eventually, but for now let's not throw the baby out with the bath water. Until the regular video interaction/handling is defined properly, it does not make much sense to look at the MPRIS side.</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>And I can enter slideshow from normal view, but not full-screen in folder-browse mode.</p></blockquote>
<p>Good point, we should just make the left-hand side of the fullscreen toolbars the same for <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Browse</span></span></span> and <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">View</span></span></span>.</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>"Autoplay" or "AutoProceed" or a more fitting term might be a more matching name for the given UI concept perhaps than "slideshow"?</p></blockquote>
<p>Those are too technical. I tried to find something better, but always ended up with "Slideshow" in the end. Unless someone comes up with a better alternative, that's how it is.</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>Also is there no indication of the timeout on a image, so the change to the next image comes a little at a surprise time.</p></blockquote>
<p>Could be added, but would need a config option in the fullscreen config widget.</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>I also would like a distinction between "Start slideshow from begin" and "Start slideshow [...]"</p></blockquote>
<p>Sorry, but no. This would turn UI and shortcuts into a spaceship cockpit. The extra <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Go</span></span><span style="color: #92969D;"> → </span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">First</span></span></span> is an acceptable and more flexible alternative. Note that when entering a folder the first image is automatically selected anyway.</p>
<hr class="remarkup-hr" />
<p>Hopefully all open questions are answered now, if not let me know.</p>
<p>I also looked at the code, cannot find anything majorly wrong with it (not that I expected anything else with your level of experience). For the DBus side of things, it would be good if you could find someone with knowledge in that area to informally double check that code.</p>
<p>Anyway, thanks so much so far, this is really an exciting feature and you also make us rethink/refine other parts of Gwenview, which is great.</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-54773">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mprismediaplayer2.cpp:46-49</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">bool</span> <span class="n">MprisMediaPlayer2</span><span style="color: #aa2211">::</span><span class="n">canRaise</span><span class="p">()</span> <span style="color: #aa4000">const</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span style="color: #304a96">false</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;">Is there a fundamental reason for that, or is this just a <tt style="background: #ebebeb; font-size: 13px;">TODO</tt>?</p>
<p style="padding: 0; margin: 8px;">Also wouldn't it be good to call this upon "Play"?</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/D10972#inline-54774">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mprismediaplayer2.cpp:51-57</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">bool</span> <span class="n">MprisMediaPlayer2</span><span style="color: #aa2211">::</span><span class="n">canSetFullscreen</span><span class="p">()</span> <span style="color: #aa4000">const</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// Gwenview itself currently only does fullscreen slideshows,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// so while in principle capable to support this by wrapping the fullscreen action,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// normal Gwenview code does not expect a state with non-fullscreen slideshow running</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span style="color: #304a96">false</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;">Why is this connected to the notion of a (auto-advancing) slideshow? If this is only about a fullscreen toggle, this could be set to <tt style="background: #ebebeb; font-size: 13px;">true</tt>. Switching tracks already works fine in windowed mode.</p>
<p style="padding: 0; margin: 8px;">In windowed mode, it would probably still make sense to make the "Play" action enter fullscreen like the corresponding "Start Slideshow" action in Gwenview does (see other part of the reply).</p>
<p style="padding: 0; margin: 8px;">Conversely, switching back to windowed mode from fullscreen should stop the slideshow, just like Gwenview does.</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/D10972#inline-54768">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mprismediaplayer2.h:20-21</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: #304a96">#ifndef KPRMPRISMEDIAPLAYER2_H</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#define KPRMPRISMEDIAPLAYER2_H</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">MPRISMEDIAPLAYER2_H</tt>?</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/D10972#inline-54787">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mprismediaplayer2player.cpp:129</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>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">mSlideShow</span><span style="color: #aa2211">-></span><span class="n">stop</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;">Now that Play/Pause works great, I think "Stop" should be equivalent to "Leave Fullscreen", i.e. end the auto-advancing and switch to windowed mode.</p>
<p style="padding: 0; margin: 8px;">This would correspond nicely with the "Play" action entering fullscreen and starting playback.</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/D10972#inline-54785">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mprismediaplayer2player.cpp:270</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: #74777d">// slight bending of semantics :)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">updatedMetaData</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"xesam:album"</span><span class="p">),</span> <span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"Gwenview Slideshow"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// TODO: hack, will fail for movies, instead create thumbnails, but which size?</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Would it be possible to include the folder name somewhere?</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/D10972#inline-54790">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mprismediaplayer2player.h:110</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="n">SlideShow</span><span style="color: #aa2211">*</span> <span class="n">mSlideShow</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">SortedDirModel</span><span style="color: #aa2211">*</span> <span class="n">mDirModel</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">ContextManager</span><span style="color: #aa2211">*</span> <span class="n">mContextManager</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Unused?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R260 Gwenview</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<br /><strong>Cc: </strong>mtijink, ngraham, nicolasfella, KDE Connect, rkflx, broulik<br /></div>