D10972: [RFC] Exposing slideshow to MPRIS controllers

Mon Mar 12 00:20:50 UTC 2018

rkflx added a comment.

  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.

  > make this an optional build feature?

  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.)

  > make this an optional feature toggable in the settings, where/how best?

  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 #gwenview <https://phabricator.kde.org/tag/gwenview/> members think differently!?). We can revisit this decision later, if need be.

  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.

  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.)

  > any idea how to create unique track ids based on the media url?

  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.

  > how to provide thumbnails to the mpris controller via temp files whose url then can be passed in the metadata?

  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 Browse or View 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 D9078 <https://phabricator.kde.org/D9078>). Controllers should just access that cache, or be told by Gwenview about the actual file location in that cache.

  > 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 :)

  Yup, playing videos with sounds should work. However, see comment below, full video support via MPRIS is probably material for later.

  In D10972#221146 <https://phabricator.kde.org/D10972#221146>, @rkflx wrote:

  > @ngraham 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?

  No reaction so far, so I'll just open a Diff for that.

  ---

  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:

  - 2 basic modes: Browse and View, switching between them is done by [Enter] and [⎋] or toolbar buttons. (The Start Page is the odd poster child, but we are thinking about upgrading it, i.e. allowing fullscreen also there.)
  - Both modes have a windowed and a fullscreen variant, you switch via [F11] or the toolbar button.
  - Navigation is done by pressing a key or a toolbar button.
  - 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 View, switches to Fullscreen and activates the slideshow bot in one go. For consistency reasons [⎋] does not undo that, but activates its default action. Normal users are not expected to use keyboard shortcuts, for them the Leave Fullscreen button will work just fine.

  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.

  > And I can enter slideshow from normal view, but not full-screen in folder-browse mode.

  Good point, we should just make the left-hand side of the fullscreen toolbars the same for Browse and View.

  > "Autoplay" or "AutoProceed" or a more fitting term might be a more matching name for the given UI concept perhaps than "slideshow"?

  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.

  > 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.

  Could be added, but would need a config option in the fullscreen config widget.

  > I also would like a distinction between "Start slideshow from begin" and "Start slideshow [...]"

  Sorry, but no. This would turn UI and shortcuts into a spaceship cockpit. The extra Go > First is an acceptable and more flexible alternative. Note that when entering a folder the first image is automatically selected anyway.

  ---

  Hopefully all open questions are answered now, if not let me know.

  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.

  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.

INLINE COMMENTS

> mprismediaplayer2.cpp:46-49
> +bool MprisMediaPlayer2::canRaise() const
> +{
> +    return false;
> +}

Is there a fundamental reason for that, or is this just a `TODO`?

Also wouldn't it be good to call this upon "Play"?

> mprismediaplayer2.cpp:51-57
> +bool MprisMediaPlayer2::canSetFullscreen() const
> +{
> +    // Gwenview itself currently only does fullscreen slideshows,
> +    // so while in principle capable to support this by wrapping the fullscreen action,
> +    // normal Gwenview code does not expect a state with non-fullscreen slideshow running
> +    return false;
> +}

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 `true`. Switching tracks already works fine in windowed mode.

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).

Conversely, switching back to windowed mode from fullscreen should stop the slideshow, just like Gwenview does.

> mprismediaplayer2.h:20-21
> +
> +#ifndef KPRMPRISMEDIAPLAYER2_H
> +#define KPRMPRISMEDIAPLAYER2_H
> +

`MPRISMEDIAPLAYER2_H`?

> mprismediaplayer2player.cpp:129
> +{
> +    mSlideShow->stop();
> +}

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.

This would correspond nicely with the "Play" action entering fullscreen and starting playback.

> mprismediaplayer2player.cpp:270
> +        // slight bending of semantics :)
> +        updatedMetaData.insert(QStringLiteral("xesam:album"), i18n("Gwenview Slideshow"));
> +        // TODO: hack, will fail for movies, instead create thumbnails, but which size?

Would it be possible to include the folder name somewhere?

> mprismediaplayer2player.h:110
> +    SlideShow* mSlideShow;
> +    SortedDirModel* mDirModel;
> +    ContextManager* mContextManager;

Unused?

REPOSITORY
  R260 Gwenview

REVISION DETAIL
  https://phabricator.kde.org/D10972

To: kossebau, #gwenview
Cc: mtijink, ngraham, nicolasfella, #kde_connect, rkflx, broulik
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180312/f4c49ae3/attachment-0001.html>