Expose slideshow to MPRIS controllers
ClosedPublic

Authored by kossebau on Mar 2 2018, 3:32 PM.

Details

Summary

Taking the abstraction "Media" in "Media Player Remote Interfacing
Specification" (MPRIS) into use, a plain slide in a slideshow can be seen
to be the same as e.g. a still picture in a movie without any sound.
Following that, a slideshow with pictures and videos as in the UI model
of Gwenview can be roughly mapped onto the concept of a list of tracks as
with in the data model of MPRIS.

This patch exposes the list of images/movies in the current folder and
the related slideshow feature as MPRIS D-Bus object, implementing for a
start the interfaces

  • "org.mpris.MediaPlayer2"
  • "org.mpris.MediaPlayer2.Player"

trying to map those to the Gwenview UX closely.

This allows the slideshow and some other navigation to be controlled
by any MPRIS controllers, which includes e.g.

  • keyboard mediakeys (Play/Pause, Stop, Next, Previous), as handled by Plasma MPRIS dataengine
  • KDE Connect media player controller plugin

Additionally the MRPIS D-Bus object is unregistered while the workspace
lockscreen is activated. Because at least the Plasma lockscreen has
the feature to show controls for any currently running MPRIS players,
which for one does not make sense currently for an image player as all
displays are locked, and then also can be surprising for some users and
result in data leaks via the image metadata used.

Future:
The great plan is to enhance the MPRIS spec to also work well for
presentation-like media shows. So there can and will be cross-app
rich (remote) controllers also for classical presentation application
instead of only per-app ones.
Supporting the existing MPRIS interfaces, even if slightly bending the
semantics, is a first step into that direction and already adds value
as it allows reuse of existing MPRIS controllers e.g. for remote control.

So follow-up work will be to also implement the other MPRIS interfaces
like org.mpris.MediaPlayer2.TrackList, for exposing the whole set of
"slides". This will enable MPRIS controllers to show the user a complete
visual list and e.g. navigate directly to a given slide or give a preview
for the next/previous slides.

CCBUG: 359381

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes

I love the idea of this feature! Would this allow your keyboard's media keys to play/pause and skip through slides in the slideshow? If so that would take a bit of the pressure off or even resolve https://bugs.kde.org/show_bug.cgi?id=359381

I love the idea of this feature! Would this allow your keyboard's media keys to play/pause and skip through slides in the slideshow? If so that would take a bit of the pressure off or even resolve https://bugs.kde.org/show_bug.cgi?id=359381

Yes, it does

Really nice idea! From my KDE Connect perspective it would be cool to have those things:

Thanks for feedback from that side, very welcome. Actually I plan to go your direction once the service side is done, as I would like to enhance the KDE Connect client side here as well.
My idea would be that it could be extended to support tracklist/playlst features of existing MPRIS2 spec, and then serve as testing ground for new specs
So in the end one is possible to do all things one currently can do with the app-specific LO Impress Remote app: https://wiki.documentfoundation.org/Impress_Remote

And all by keeping to the abstractions as possible, and only expose features in the UI if the MPRIS service supports them. So simple audio player -> same UI as currently. Complex media show client -> UI similar to Impress Remote.

Bonus points would be that players might inspired to support more features known from presentation software.
Seeing a movie and want to point your friends/students to a spot in a scene? Just rewind to the scene and use the image marker UI (as known from presentation tools) to highlight the area etc.

  • the pause methode stoppes/pauses the slideshow. It seems natural and fits better into KDE Connect than stop

Any chance KDE Connect can instead extend its support of the MediaPlayer2.Player interface? There Stop & Pause are two distinct different operations (Reset vs. Hold), and the property CanPause is an official flag to declare the ability to also do Pause.
Why I would rather like to see KDE Connect extended:
Right now the existing implementation of Gwenview only supports the concept of Stop (i.e. cancel show and reset to "begin" of current track/slide). While it might be an option to work on this and help the curently abilitites of KDE Connect, a real "Pause" does not make that much sense in the domain of a still image slide show (only when it's an video, which I have not yet really tested).
Then there can be players which do live streams (think webcam or babymonitor) without any buffering, so where the concept of Pause also does not apply. Yet one might want to turn on & off those using KDE Connect.

  • sadly mpris doesn't seem to have a canControlVolume slider, so we can't disable the slider in KDE Connect when its useless. But it would be nice if we could control the video volume from it.

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

@nicolasfella And I just saw a youtube video about an initial sailfishos client for KDE connect? That might pull me into KDE Connect development even more soon :)

I love the idea of this feature! Would this allow your keyboard's media keys to play/pause and skip through slides in the slideshow? If so that would take a bit of the pressure off or even resolve https://bugs.kde.org/show_bug.cgi?id=359381

That depends who has registered for those media keys and processes them. If that is done by the media controller applet, then yes. I see that somehow at least the NextMedia/PreviousMedia keys have an effect when on another virtual screen, so chance is indeed the media controller applet uses them. But Play/Pause & Stop do not have any effect, thus unsure myself.

ngraham added a comment.EditedMar 2 2018, 11:52 PM

Pause" does not make that much sense in the domain of a still image slide show

I think it does, actually. Slideshows can auto-advance after n number of seconds, and this can use pause and play features to control that.

Pause" does not make that much sense in the domain of a still image slide show

I think it does, actually. Slideshows can auto-advance after n number of seconds, and this can use pause and play features to control that.

Given that time has no relationship to a still image, if one pauses on the current image (e.g. to look longer at it or do something else), why would one then want to wait the exactly remaining time again until the next image appears?
What use-case would you be thinking of?

I suppose you could auto-advance on resume. But let's not overthink things here: it's just pausing and resuming in a timed image slideshow. I don't think it's the worst thing in the world if, say, you have a 4 second time per slide, pause on second 2 to explain the picture to your bored audience who's tired of looking at your bad vacation photos, and then after you resume, your already-bored guests have to sit through a further 2 seconds on the current slide before seeing the next one.

Thanks for feedback from that side, very welcome. Actually I plan to go your direction once the service side is done, as I would like to enhance the KDE Connect client side here as well.
My idea would be that it could be extended to support tracklist/playlst features of existing MPRIS2 spec, and then serve as testing ground for new specs
So in the end one is possible to do all things one currently can do with the app-specific LO Impress Remote app: https://wiki.documentfoundation.org/Impress_Remote

Awesome, your contribution would be very welcome!

Any chance KDE Connect can instead extend its support of the MediaPlayer2.Player interface? There Stop & Pause are two distinct different operations (Reset vs. Hold), and the property CanPause is an official flag to declare the ability to also do Pause.
Why I would rather like to see KDE Connect extended:
Right now the existing implementation of Gwenview only supports the concept of Stop (i.e. cancel show and reset to "begin" of current track/slide). While it might be an option to work on this and help the curently abilitites of KDE Connect, a real "Pause" does not make that much sense in the domain of a still image slide show (only when it's an video, which I have not yet really tested).
Then there can be players which do live streams (think webcam or babymonitor) without any buffering, so where the concept of Pause also does not apply. Yet one might want to turn on & off those using KDE Connect.

A use case for pause might be: Your're watching a slideshow with auto-advance but need to go to the bathroom and pause it for x minutes.
I think it would be best to support both stop and pause and adapting KDE Connect should be fairly easy

  • sadly mpris doesn't seem to have a canControlVolume slider, so we can't disable the slider in KDE Connect when its useless. But it would be nice if we could control the video volume from it.

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

I have an idea how to avoid a useless volume slider but still have it when needed, though it would be outside of the specification. When no volume slider is desired (e.g. when displaying an image) we could report -1 as volume and KDE Connect (and other KDE Mpris consumers) could take this as a hint to disable the slider.

kossebau added a comment.EditedMar 3 2018, 4:35 AM

I love the idea of this feature! Would this allow your keyboard's media keys to play/pause and skip through slides in the slideshow? If so that would take a bit of the pressure off or even resolve https://bugs.kde.org/show_bug.cgi?id=359381

That depends who has registered for those media keys and processes them. If that is done by the media controller applet, then yes. I see that somehow at least the NextMedia/PreviousMedia keys have an effect when on another virtual screen, so chance is indeed the media controller applet uses them. But Play/Pause & Stop do not have any effect, thus unsure myself.

Seems indeed the media control keys (Play/Pause, Stop, Next, Previous) are registered via KGlobalAccel by the Mediacontroller applet by asking the MultiplexedService from the MPRIS2 dataengine to do that. Cmp.
https://cgit.kde.org/plasma-workspace.git/tree/applets/mediacontroller/contents/ui/main.qml#n187
https://cgit.kde.org/plasma-workspace.git/tree/dataengines/mpris2/multiplexedservice.cpp#n78

Though again we hit code which assumes CanPause=true, that's why the Play/Pause key did not work for me before when I tested with the patched Gwenview :) Did, as I have a patch now, soon up with some more MPRIS support fixes. Edit: now up as D10991

mtijink added a subscriber: mtijink.Mar 3 2018, 2:16 PM

Your feature looks awesome! I'd also like to weigh in on the pause/stop discussing though.

  • the pause methode stoppes/pauses the slideshow. It seems natural and fits better into KDE Connect than stop

Any chance KDE Connect can instead extend its support of the MediaPlayer2.Player interface? There Stop & Pause are two distinct different operations (Reset vs. Hold), and the property CanPause is an official flag to declare the ability to also do Pause.
Why I would rather like to see KDE Connect extended:
Right now the existing implementation of Gwenview only supports the concept of Stop (i.e. cancel show and reset to "begin" of current track/slide). While it might be an option to work on this and help the curently abilitites of KDE Connect, a real "Pause" does not make that much sense in the domain of a still image slide show (only when it's an video, which I have not yet really tested).
Then there can be players which do live streams (think webcam or babymonitor) without any buffering, so where the concept of Pause also does not apply. Yet one might want to turn on & off those using KDE Connect.

I think "Pause" certainly makes sense in a slideshow. Stop should reset to the beginning of the slideshow (as required by the mpris specification). Pausing should allow you to continue playing the slideshow where you left off. "Stop" certainly is a useful feature but play/pause should be far more common. If you think about the analogy to video/music, that seems the logical behaviour to me.

kossebau added a comment.EditedMar 3 2018, 4:34 PM

Your feature looks awesome!

Thanks, so by all that feedback it seems it was not just a wild idea :)

I think "Pause" certainly makes sense in a slideshow. Stop should reset to the beginning of the slideshow (as required by the mpris specification). Pausing should allow you to continue playing the slideshow where you left off. "Stop" certainly is a useful feature but play/pause should be far more common. If you think about the analogy to video/music, that seems the logical behaviour to me.

This proposal maps a slide to a "track" in MPRIS terms, and the complete slideshow to a tracklist. Given the properties of a track match that of a slide, and the same for a tracklist matching that of a slide set. Edit: Also consider that some slides, as with Gwenview, can be movies (like animated GIFs & Co), so that mapping makes even more sense (to me).
And MPRIS for the Stop method defines this (as you said): "Calling Play after this should cause playback to start again from the beginning of the track. "

So with that in mind, would you reconsider what you think is logical? :)

This proposal maps a slide to a "track" in MPRIS terms, and the complete slideshow to a tracklist. Given the properties of a track match that of a slide, and the same for a tracklist matching that of a slide set. Edit: Also consider that some slides, as with Gwenview, can be movies (like animated GIFs & Co), so that mapping makes even more sense (to me).
And MPRIS for the Stop method defines this (as you said): "Calling Play after this should cause playback to start again from the beginning of the track. "

So with that in mind, would you reconsider what you think is logical? :)

Ah, with that in mind it makes sense. I guess the analogies break down a bit though, since you can't really "play" an image. Would it make sense to use "pause" as "stop" too, if it's not a video?

kossebau added a comment.EditedMar 3 2018, 6:11 PM

This proposal maps a slide to a "track" in MPRIS terms, and the complete slideshow to a tracklist. Given the properties of a track match that of a slide, and the same for a tracklist matching that of a slide set. Edit: Also consider that some slides, as with Gwenview, can be movies (like animated GIFs & Co), so that mapping makes even more sense (to me).
And MPRIS for the Stop method defines this (as you said): "Calling Play after this should cause playback to start again from the beginning of the track. "

So with that in mind, would you reconsider what you think is logical? :)

Ah, with that in mind it makes sense.

Okay, good getting us to some common grounds.

I guess the analogies break down a bit though, since you can't really "play" an image. Would it make sense to use "pause" as "stop" too, if it's not a video?

Not sure, It might really depend what mental model we will see with users in general: the slideshow being the actual object that is "played" (like you instinctivly saw it) or the individual images/movies, simply organized as playlist/tracklist. Though in the first case users then might expect Stop to also jump to begin of slideshow.
Looking at the current menu entry in Gwenview itself and its behaviour: it just says "Stop slideshow" (so does not use the term "Pause", though uses an icon with Pause symbol, not Stop symbol). By the behaviour it stops the slideshow at the current image and keeps that selected., also resets the display timeout to 0, so on activation of "Start slideshow" again it hehaves as also specified for the MPRIS Stop action.

But I can see how people might be confused about seeing the "Pause" media key on their keyboard not work, once they can use the media keys due to MPRIS and e.g. the Plasma media controller applet. The difference between Stop and Pause for most users of image slideshows might be rather academical for what I assume the typical use-cases. Possibly the best escape from this challenge is to add an implementation of Pause to Gwenview itself :)
But IIMHO that is slightly orthogonal to this patch, unless it will conflict with the needs to match the MPRIS spec. And possibly might gain from collecting some usage experience first.

My developer's brain used to abstracting things :) sees it like this:
a "track" is a media object which can have multiple parallel subtracks of types like typically sound and image-frames (not wide-spread but possible would be physical object control like for puppets moving, fountains shooting, pipes of street organ blowing, light spots glowing, or, he, odor spraying ;) ).
And the player then goes and "renders" the data from the tracks. The data themselves would either allow random access because coming as fixed object from some storage like filesystem or full database or coming from some deterministic data generator. Or the data would not allow random access, because it is generated non-deterministically e.g. from sensors on the physical world (like microphone, camera) without any buffering.

So with that abstract thinking a simple static slide with some timeout is the same as a short video just showing only the same image. And a simple static slide with no timeout is the same as a video livestream showing only the same image. And thus "Stop" and "Pause" with their different concepts at least by design should be applied the same.

If we think further in that grand plan as sketched initially, slides in a presentation show also can have a let's-call-it sub-slideshow, where a slide can reach several states by items appearing, changing or disappearing (and just thinking about linear organized shows :) ). Once we get there and try to create model concpets for the needed new MPRIS interfaces, perhaps the right now proposed mapping has to be rethought indeed. But for what I drafted some time ago, for now mapping a single (main) slide, which is usually shown for some minutes, to a track, which usually is some 3-minutes pop music, should work out with what exists in MPRIS.
Multi-hierarchy track notation might be also interesting for non-3-minutes-pop-music tracks. Think movies separated in story chapters, classical Western music (operas, synfonies) being composed of units of units. So the same structuring as known e.g. from books might be useful to have, to allow navigation using the same interaction patterns where sane.

And all that while trying to be simple by default, but powerful when needed. Some road ahead. Hopefully not too bumpy or being a dead end :)

I think it would make sense to add pause support to Gwenview, but I agree that it could be done in another patch. KDE Connect relies on PlayPause, so it doesn't work really good in the current state. We could change KDE Connect, but keep in mind that there might be other controllers that might behave similar.

but keep in mind that there might be other controllers that might behave similar.

Indeed. As I meanwhile discovered all the MPRIS controllers across the Plasma universe (media controller applet, task manager applet, global media keys handler in the mpris dataengine(!?)) are not cooperative with Pause-less players, meh.
Just, by that they effectively do not follow the MPRIS MediaPlayer2.Player specification, which clearly states for PlayPause:

If CanPause is false, attempting to call this method should have no effect and raise an error.

IMHO interoperability is key to successful (Free/Libre) software, and thus specifications should be valued and followed closely. So we should see to fix the controllers which do not comply to the spec.
And given this is FLOSS, we have easy options to do so :) For those Plasma-related controllers I should soon have at least patches proposed for all of them, so there hopefully soon a flaw of the past.

Plasma >= 5.12.3 (released today) should now be prepared for the MPRIS features as exposed/used by this patch. Patches needed to make the features work with

  • global media keys (on keyboard)
  • Plasma mediacontroller applet
  • mediaplayer support in the Plasma taskmanager (tooltip & context menu)

have been pushed just in time for 5.12.3 tagging.

For those interested in enhancements of the MPRIS specs, I also created https://community.kde.org/MPRIS as a collection/dumping ground for material for now, to be massaged into more proper structure & content by the time, so at least the ideas are not lost if people move elsewhere :)

So for this patch I would be back to the "Questions:" as in the patch's summary text at the begin. All ready for your answers/comments, Gwenview team :)
Ideally we can sort out things in time for the feature freeze of KDE Applications 18.04, which is March 22th?

rkflx added a comment.Mar 7 2018, 1:04 AM

@kossebau Slowly making progress with my review queue… Did not read all of the (lengthy) discussion yet, but for a head-start, could you give me a short summary on why I see a disabled pause button instead of a stop** button in the mediacontroller applet? (I discovered the context menu to stop the slideshow again only by accident, i.e. not something normal users would find.) – Thanks ;)

**) In the sense of "make it not move anymore", I'll have to think about whether pause or stop would be right.

@kossebau Slowly making progress with my review queue…

Okay, looking forward to you reaching this one :)

Did not read all of the (lengthy) discussion yet, but for a head-start, could you give me a short summary on why I see a disabled pause button instead of a stop** button in the mediacontroller applet? (I discovered the context menu to stop the slideshow again only by accident, i.e. not something normal users would find.) – Thanks ;)

**) In the sense of "make it not move anymore", I'll have to think about whether pause or stop would be right.

The (old) MPRIS specification has the two concepts "Pause" (as in hold on current position in track) and "Stop" (as in reset to begin of track). With "Pause" being an optionally supported feature per track, which can be queried by a property CanPause.
The maintainers of the Plasma mediacontroller designed the UI though with rather continious (music) players in mind, where one would only toggle between Pause and Play, and thus only need one button for that (both media controller as well as media controller overlay for taskmanager tooltip).

My initial patches proposed to make that toggle button switch between Pause & Play or Stop & Play, depending on current value of CanPause property. Which was not winning over the minds of the maintainer(s). Too still get at least some version of the patches in in time for Plasma 5.12.3, so people can test & use the given patch here for Gwenview already without having to patch Plasma themselves, I removed that part for the middle button in the media controller(s) again and just made the patches fix the usage of the wrong unconditional PlayPause action everywhere.

So this is not the final solution yet and needs more thinking/discussions/design. But at least progress to the old state.

rkflx added a comment.Mar 7 2018, 10:32 PM

Played around a bit with Gwenview, JuK, Elisa, Amarok and VLC. I could not find any app which worked flawlessly in Plasma. IMO shipping MPRIS support in a half-broken state does not align all that well with Gwenview's vision of polish (from a user's point of view when they hit "Pause" on their keyboard and in the mediacontroller applet – I do recognize it's important to follow a spec in the longer term, though).

I pondered for quite a bit, and I think mapping slideshows to media players and then assigning buttons is fundamentally flawed. The long discussion above is an indicator for this. We should think in terms of use cases and UI, not in terms of conforming to a spec (which is important, but an implementation detail in the end). In case there is only space for a single button, the question to ask is "What is the most important function which should be controlled by this button?". Stating "Let's disable the button too, because the app does not claim support for one particular function!" should be avoided. Brainstorming:

# of ButtonsController UIGwenviewImpressJuKLive video stream
1Basic (1 primary button)Hold/Resume slideshowHold/Resume slideshowHold/Resume playbackStart/Stop stream
3Simple (+2 navigational buttons)+previous/next image+previous/next slide+previous/next track+switch channels
5+xAdvanced (+more regular and navigational buttons)+start/resume videos in slideshow, skip in videos+blank screen, advance on bullet level+stop playlist, seek through track+mute audio, switch filters
4 / default caseKeyboard (prev, next, stop, play/pause)"simple" + x"simple" + x"simple" + x"simple" + x

This way we avoid any awkward mapping to concepts like "canPause", every app can decide on their own what navigation means and what primary/secondary etc. buttons should do. Think DWD, but much more restricted to a specific type of usage and thus implementable.

Obviously, for this to work the app would have to provide icon and text for the controller to put on the requested type of button and in the context menu, perhaps with falling back to the selection of icons/texts we currently have. After all, in Impress you do not "Play" a presentation, but you "Present" a slideshow. This kind of polish is important, but not really achievable with the current spec.


Now, what to do with your patch for Gwenview? Obviously the current generation of controllers often only has a pause button, and keyboards have a permanent pause button. As "holding" the slideshow is the primary use case, that's what Gwenview should do right now in reaction to this button. Of course this can be refined later on when better ways to express functionality become available.

Please tell me where I'm wrong in my thinking, and I'll admit defeat in my mission to keep Gwenview's quality up ;)

Once we cleared up the behavioural part, it will be much easier to take a decision on some of your questions.


TL;DR: In general mapping images to tracks seems sane, but could you make it so that "Pause" as well as "Stop" in the controller or on the keyboard both map to the single corresponding "hold" action in Gwenview for the time being?

rkflx added a comment.Mar 7 2018, 10:49 PM

In terms of testing with the mediacontroller applet, apart from the Pause problem I found those issues:

  • Thumbnails for some pictures have incorrect rotation
  • Rotating images does not reflect in the preview, even after saving
  • Skipping through pictures with different aspect ratios results in the thumbnail extending over the bounds of the applet

@kossebau How expensive is this feature in terms of CPU and DBus traffic for navigating in non-fullscreen browse mode if it were enabled by default for a default Plasma install, i.e. only the systray icon shown?

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

I pondered for quite a bit, and I think mapping slideshows to media players and then assigning buttons is fundamentally flawed.

You mean "music player" or "movie player", right? Because ",media" by itself is an abstraction, and slides/pictures are a type of media, no? :)

But let me make clear where I come from to show up with this initial patch:
over the last years I observed myself and others in quite some situations where it would have improved life style if one was able to remotely control the device which ran a presentation (e.g. giving a talk/lecture while walking across the room/stage, or giving a photo/videosnaps report and resting with your fellows in comfortable furniture).

Now one could design the perfect suited software/hardware solution for those generalized use-cases. Chance to get it done rather 0 %.
Or one could see to make reuse of existing infrastructure & tools which are not perfect, but get the job done for a start. And ideally that initial setip advertises that there is a solution to the problem, so more people no longer avoid the problem but enjoy the use-case, and with more users there is more chance of resources to improve things into a perfect solution.

I totally agree that this patch will not provide the perfect solution. And I also agree the MPRIS spec at v2.0 is not yet the golden one, as initially said the goal is also to collect more experience by using it in the field for slideshows to improve it. And the current implementation of MRPIS-UIs in Plasma are IMHO music-player centric, possibly because that was the only use-case and thus their driver so far. Same with UI in KDE Connect.

But to restate (because I am tired right now to write things short, pardon me): the idea of this patch is to start off from something that works now with the things that are available. It's enabling new things without waiting for the perfect solution to be done first.. And not blocking the perfect solution, rather providing more feedback by the use of this first prototype.

We should think in terms of use cases and UI, not in terms of conforming to a spec (which is important, but an implementation detail in the end).

Agreed. And it is where I started from :) But missed to point that out and the fact that I already had went shopping for things that can help get a first implementation/solution now and this is what I returned with in my bags. By just writing this patch (well, a variant where CanPause=true and just mapping MPRIS Pause to GwenView's Stop), I could give a trip photo report using Gwenview on the computer and leaning back onto the couch with a mobile and controlling things with legs-up via KDE-Connect (just had to switch to Remote Input when needing to go to previous slide ;) ).

Your proposals here for the UIs make sense to me, and I will add them as further material for the MPRIS 3.0 collection on the above linked wiki page. Just, next to finding agreement on that new protocol/spec they also need (re)implemantion on the complete chain first.

Now, what to do with your patch for Gwenview? Obviously the current generation of controllers often only has a pause button, and keyboards have a permanent pause button. As "holding" the slideshow is the primary use case, that's what Gwenview should do right now in reaction to this button. Of course this can be refined later on when better ways to express functionality become available.

The inital patch was done as proof of concept & to inspire possibilities, adapting to the existing behaviour and code of gwenview (which does "reset", not "hold" on the "Stop presentation" action). I did not want to challenge that on first contact with you the maintainers :) Or invest more into the patch first only to find out you do not want this at all.

My expectations have been you could answer my questions, then I would add the MPRIS adapter code, and then we would together extend things to make the slideshow feature slightly better, also with the new use cases of remote controlling, and iterativly shape things into sanity (too bad no managers around who could be bought into by saying "agile!" ;) ).

To be frank, the whole interaction pattern around Gwenview's slideshows confuses me. E.g. when entering the slideshow state from a non-fullscreen window, when leaving it (e.g. by ESC key) I end up in fullscreen state still. And I can enter slideshow from normal view, but not full-screen in folder-browse mode. And when in full-screen single-file view mode, the actual "slideshow" being on actually only means automatic advancing to another image/video to show, after timeout/video-done. Where my concept of slideshows before was not involving only automatic advancing, but included any (fullscreen) display of the individual "slide" items also those with manual advancing only. So Gwenview's fullscreen single-file view mode alone already means slideshow to me, as I can give the slideshow by manually walking through the images and videos. "Autoplay" or "AutoProceed" or a more fitting term might be a more matching name for the given UI concept perhaps than "slideshow"?

Then there is the duplication of the concept "Start" when it comes to a video being the current item. For one videos are autostarted if switched to. But when having paused a video as current item, calling "Start Slideshow" does not unpause the video, other than perhaps expected.

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.

Next to that, but perhaps independently I also would like a distinction between "Start slideshow from begin" and "Start slideshow, but jump straight to currently selected image". The latter is what happens now, and it is both unexpected most of the time, but also needs additional activity to make sure one really has the first image selected when wanting to show all the files.

So.... IMHO independent of exposing things to remote via MPRIS, Gwenview things themselves could see some more twisting please :)

BTW, https://en.wikipedia.org/wiki/Media_controls taught me there even are some ISO/IEC specs for the concepts of Play, Pause, Stop & Co :)

TL;DR: In general mapping images to tracks seems sane, but could you make it so that "Pause" as well as "Stop" in the controller or on the keyboard both map to the single corresponding "hold" action in Gwenview for the time being?

Sure. Patch update coming in, there now Pause and Stop MPRIS actions both SlideShow::stop. And the MPRIS states "Playing" & "Paused" now correspond to SlideShow->isRunning(), while "Stopped" is when no slideshow is possible (i.e. action is disabled).
The bends things a little, as the MPRIS spec demands that "Stop()" "Stops playback", so instead going for "Paused" state would be wrong, but we could just claim someone else directly afterwards called "Play()" and then "Pause()",
From what I tested, Plasma MPRIS controllers (UI and keyboad media key handler) nicely dealt with that.

Late in the night, but might not get back to computer tomorrow, so to keep things a little rolling this braindump here, hopefully consumable.

kossebau updated this revision to Diff 29200.Mar 11 2018, 2:55 AM

Make Pause API working, map to slideshow stopped

In terms of testing with the mediacontroller applet, apart from the Pause problem I found those issues:

  • Thumbnails for some pictures have incorrect rotation

Yes, that is because currently the raw file url is only passed in the metadata. Please see initial question "how to provide thumbnails to the mpris controller via temp files whose url then can be passed in the metadata?" which would allow to handle that, by using instead a properly preprocessed thumbnail. Sadly we can only pass a url in the metadata, so it would need to be some file on the filesystem where the thumbnail is made available. Does Gwenview store the thumbnails in some cache files which could be reused for that?
AFAIK the mpris:artUrl we can use here in the metadata per slide sadly does not specify format or recommended image size, so anything that is fine for Gwenview as preview should be fine with MPRIS as well. One of the things a future MPRIS spec should have better care for.

  • Rotating images does not reflect in the preview, even after saving

See above. And for after saving, possibly the media controller UI caches things in memory, or just ignores any rotation settings on loading.
We should assume the average MPRIS controller is not very clever about image formats, so best gets some simple PNG file for the artUrl.

  • Skipping through pictures with different aspect ratios results in the thumbnail extending over the bounds of the applet

That seems some Plasma Media controller decision how to do the scaling into the square space used for the "track art". Not specified in MPRIS, so controller decision.

@kossebau How expensive is this feature in terms of CPU and DBus traffic for navigating in non-fullscreen browse mode if it were enabled by default for a default Plasma install, i.e. only the systray icon shown?

I have no clue about the costs or how to measure that even. But I would not suspect this being expensive, unless people change images and play/slideshow states 1000x per second.

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

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

lib/mpris2/mprismediaplayer2.cpp
47–50

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

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

52–58

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.

lib/mpris2/mprismediaplayer2.h
21–22

MPRISMEDIAPLAYER2_H?

lib/mpris2/mprismediaplayer2player.cpp
130

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.

271

Would it be possible to include the folder name somewhere?

lib/mpris2/mprismediaplayer2player.h
111

Unused?

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

Actually dbus is (in theory) available on Windows (https://www.freedesktop.org/wiki/Software/dbus/#Windowsport), but I don't know whether it will be of any use

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.

You might also want to look into how the filenames of the thumbnail cache are generated, because those already seem to have a hash or ID which filenames can be mapped onto.

kossebau updated this revision to Diff 29337.Mar 12 2018, 4:30 PM
kossebau marked 4 inline comments as done.

update to in-code feedback/comments

lib/mpris2/mprismediaplayer2.cpp
47–50

More a TODO, had not yet started to think about how and where this could be useful. Also not yet seen how existing controllers make use of this feature.

To call this on "Play" would be a decision of the controller. From my reading of the spec this seems more about bringing any specific media app controller UI onto the screen, not the actually rendered media (though usually apps have the bundled in their UI). But room for interpretation. Then in Wayland apps might not be able to autoraise their windows.

Added an explicit todo comment now, so future code reader can continue from here.

52–58

It was connected to things as by my initial mind model of gwenview states :)

https://specifications.freedesktop.org/mpris-spec/latest/Media_Player.html#Property:Fullscreen does not specify whether requesting a change of fullscreen state should result also in playbackmodus change, so should be fine to do as you prefer.

Not aware of an controllers which make use of this, so only tested via qtdbusviewer.

lib/mpris2/mprismediaplayer2.h
21–22

Was some last trace of this feature patch some years ago initially drated for Calligra Stage (which uses KPR from former KPresenter name as prefix :) )

lib/mpris2/mprismediaplayer2player.cpp
271

The folder name could be mapped onto "xesam:album" semantics instead of the "Gwenview Slideshow". What would be a good source to read the folder name from?

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

Okay, patch updated to that.

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

Hm, good question. After some initial thoughts, IMHO having access to the current playlist has a similar risk of privacy leaking (no-one should know I listen to unpopular music XYZ or audio book ABC or watching movies from insane series 123 ;) ). For now I would just opt for: say No to screen locker media controls in general. But no decided opinion yet, as I do not use it. I can see how this being a new feature can be surprising to some, even if consistent.

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.

When we later want to support MPRIS tracklist/playlist, ideally that mapping can be done in both directions, without overhead. Any hashing would also need bookkeeping for allowing the inverse (and then there is the minimal risk of collisions).
I guess for now we could simply encode the url/path and use that.

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). Controllers should just access that cache, or be told by Gwenview about the actual file location in that cache.

That's a good idea with the standard thumbnail cache. Just, MPRIS does not specify that the controllers should go for those thumbnails, so to be on the safe side we better point them ourselves directly to a matching thumbnail. Any pointers in the Gwenview code where one could read-out the matching paths to the thumbnail in the cache?
Though some thumbnailers might only generate thumbnails on the fly (official option, e.g. when there are already thumbnails embedded in the file itself). And in case of unsaved changes (e.g. rotated images) ideally we would still create a matching custom thumbnail ourselves.

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.

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.

I see. FTR I would not subscribe to ESC being a key shortcut not for normal users, but your call, I will not be another cook to add more ingredients to the design soup :)

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.

Okay.

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

I saw that "Auto-Advance" seems to be a term that can be found in image/slides related software, perhaps worth a consideration.

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

Thanks a lot for the quick and detailed reply, got things some way. As by comments above, thumbnails and unique track ids still need more thinking, will play some more with the code tonight.

kossebau updated this revision to Diff 29341.Mar 12 2018, 5:44 PM

build MPRIS support only if Qt5DBus found

kossebau updated this revision to Diff 29357.Mar 12 2018, 10:00 PM
  • use base64 encoding for creating unique ids per image/video url
  • drop support for thumbnails urls in the metadata for now

While I have some ideas how to hook-up into the thumbnail cache system, I would consider the display of a thumbnail of the current image/movie via the mpris:arturl metadata an additional feature which currently does not add that much value, as one usually sees the current image straight on the screen. So I would like to postpone this to a future patch.

Instead I want to focus this first version of MPRIS support on enabling remote control of the "slideshow"/image browsing by the navigation and play/pause/stop commands.
For what I tested, the current version works reliably with the existing Plasma MPRIS controller as well as KDEConnect.

Thanks for the updates, works even neater than before ;) Changes to the code look good to me (even though you sneaked in more changes than announced, making me wonder whether some parts could be shared with other users like Okular in the future…).

For now I would just opt for: say No to screen locker media controls in general.

On Tumbleweed this worked (ignoring the fact that normal users will have a hard time digging for the option), but on Neon and Bionic that config setting was missing. I'm pretty sure that's not a custom openSUSE patch, so probably an issue in the theming support on the other distros?

I guess my question was more about the how, not about the if. I cannot see any reason to show Gwenview on the lockscreen, why would you want to navigate images which are hidden behind the lockscreen anyway? I anticipate we'll get bug reports about this, because simply having Gwenview open (regardless of fullscreen state, and that's good!) will trigger this behaviour.

Note that Gwenview inhibits power management (in theory, did not test), so even the use case of stopping the slideshow from the lockscreen when you've fallen asleep does not apply.

Any pointers in the Gwenview code where one could read-out the matching paths to the thumbnail in the cache?

Grepping for 256 I quickly ended up in generateThumbnailPath.

Though some thumbnailers might only generate thumbnails on the fly (official option, e.g. when there are already thumbnails embedded in the file itself).

Did not read that part of the codebase yet, but I'd assume there are three sources for thumbnails: embedded, cached and computed; and there's also some smoothing going on in a delayed step (if you observe the rendering carefully). Then we have the problem that per FDO spec the cache maxes out at 256px, which clashes with HiDPI requirements.

Perhaps we should stretch the spec a bit by caching thumbnails regardless of type and in higher resolution, so that the cache would always contain a matching entry? This needs more investigation at a later time…

And in case of unsaved changes (e.g. rotated images) ideally we would still create a matching custom thumbnail ourselves.

I believe there are also some bugs where Gwenview gets confused and displays incorrect orientations itself.

When we later want to support MPRIS tracklist/playlist, ideally that mapping can be done in both directions, without overhead.

I see. If I interpret the DBus spec right, the ID can be of arbitrary length, so your base64 approach is probably fine.

I saw that "Auto-Advance" seems to be a term that can be found in image/slides related software, perhaps worth a consideration.

The only place the string is used besides tooltips is the menu entry which also triggers fullscreen, i.e. "Start Slideshow". Renaming to "Activate Auto-Advance and switch to Fullscreen" is a bit unwieldy.

The folder name could be mapped onto "xesam:album" semantics instead of the "Gwenview Slideshow". What would be a good source to read the folder name from?

I'm not aware of any special handler for that, so your best bet might be adjusting the URL.


drop support for thumbnails urls in the metadata for now

Good call.

I want to focus this first version of MPRIS support on enabling remote control of the "slideshow"/image browsing by the navigation and play/pause/stop commands.

I found one more niggle, but this one could also be fixed later: The stop action should be disabled once the slideshow is stopped, i.e. in terms of our current behaviour when Gwenview is in windowed mode. VLC and JuK seem to support this when testing the mediacontroller applet.


To summarize: Patch seems to be able to make it into the Beta in time. Folder name and stop button are not really important, but I'm still feeling a bit uneasy about the lockscreen issue.

Haha, I should not have scrolled to the end of the Diff again…

lib/slideshow.cpp
290–294

Not needed anymore?

kossebau marked an inline comment as done.EditedMar 14 2018, 12:54 AM
(even though you sneaked in more changes than announced, making me wonder whether some parts could be shared with other users like Okular in the future…).

You mean D-Bus registration code? Indeed, forgot to mention that, sorry. This was feedback from issues found with the parallel patch for Okular, where the Plasma MPRIS dataengine sometimes ignored the appeared service. Reason is that as I now learned since Qt 5.6 D-Bus registration is done in an own thread, so not waiting on main event loop. Thus in the moment the service appears on the D-Bus with the well-known name, any other process which sees that can already make requests to it. Just if the code has not yet advanced to to the registration of the D-Bus object, the other process' request will fail with error, and it will then tend to ignore the service. So the order is now important and needs to be: first prepare objects, then announce service by name :)

For now I would just opt for: say No to screen locker media controls in general.

On Tumbleweed this worked (ignoring the fact that normal users will have a hard time digging for the option), but on Neon and Bionic that config setting was missing. I'm pretty sure that's not a custom openSUSE patch, so probably an issue in the theming support on the other distros?

I guess my question was more about the how, not about the if. I cannot see any reason to show Gwenview on the lockscreen, why would you want to navigate images which are hidden behind the lockscreen anyway? I anticipate we'll get bug reports about this, because simply having Gwenview open (regardless of fullscreen state, and that's good!) will trigger this behaviour.

Note that Gwenview inhibits power management (in theory, did not test), so even the use case of stopping the slideshow from the lockscreen when you've fallen asleep does not apply.

In case of video playing (with sound) this could be even wanted ;) (have some old music videos directories which I sometimes play just for the music).

Will ask tomorrow the Plasma people what could be done here.

Any pointers in the Gwenview code where one could read-out the matching paths to the thumbnail in the cache?

Grepping for 256 I quickly ended up in generateThumbnailPath.

Yes, thanks, meanwhile also have some first experimental code which hooks into this. Though plan to only work more on this when I get to "playlist"/"tracklist" support and for that also work on extending kdeconnect mpris plugin to allow scanning through the list, as then the thumbnail views will be mainly interesting (same with Okular for the pages). Planned for somewhen in the summer, when it is again feature development time.

Though some thumbnailers might only generate thumbnails on the fly (official option, e.g. when there are already thumbnails embedded in the file itself).

Did not read that part of the codebase yet, but I'd assume there are three sources for thumbnails: embedded, cached and computed; and there's also some smoothing going on in a delayed step (if you observe the rendering carefully). Then we have the problem that per FDO spec the cache maxes out at 256px, which clashes with HiDPI requirements.

Perhaps we should stretch the spec a bit by caching thumbnails regardless of type and in higher resolution, so that the cache would always contain a matching entry? This needs more investigation at a later time…

Yes. Possibly this is something to do in sync with FDO & Co., given HiDPI is also interesting for thumbnails in other file managers/browsers. Big note made.

I saw that "Auto-Advance" seems to be a term that can be found in image/slides related software, perhaps worth a consideration.

The only place the string is used besides tooltips is the menu entry which also triggers fullscreen, i.e. "Start Slideshow". Renaming to "Activate Auto-Advance and switch to Fullscreen" is a bit unwieldy.

:) Just my 2 cents.

The folder name could be mapped onto "xesam:album" semantics instead of the "Gwenview Slideshow". What would be a good source to read the folder name from?

I'm not aware of any special handler for that, so your best bet might be adjusting the URL.

Given Gwenview thankfully supports KIO protocols, there might be url schemes where the "path" can not be simply decomposed into hierarchy patterns from the file url itself. Though for now we could simply first test for localFile, then do normal filesystem query, otherwise just do a good guess with taking xxx/(FOLDER)/filename.foo.bar match from the path. Ah, or just see if the semantical dir model has a folder property? Will have a look into that.


drop support for thumbnails urls in the metadata for now

Good call.

Okay, glad you agree.

I want to focus this first version of MPRIS support on enabling remote control of the "slideshow"/image browsing by the navigation and play/pause/stop commands.

I found one more niggle, but this one could also be fixed later: The stop action should be disabled once the slideshow is stopped, i.e. in terms of our current behaviour when Gwenview is in windowed mode. VLC and JuK seem to support this when testing the mediacontroller applet.

Okay, no problem, will change.
In the MPRIS patch for Okular even I initially propose to simply remove the MPRIS service completely when not in presentation mode. Based on the idea that in normal reading mode one does not want any remote mediaplayer-like navigation, but instead interacts using the normal custom UI of Okular. Only once the presentation mode is activated and entered on, the MPRIS service is registered.

Perhaps this model might make sense for Gwenview as well? So no exposure as MPRIS in windowed mode?


To summarize: Patch seems to be able to make it into the Beta in time. Folder name and stop button are not really important, but I'm still feeling a bit uneasy about the lockscreen issue.

Sounds good. Will see to get more info about the lockscreen thing tomorrow. Hoping you are around again the night again, so you can hopefully give the final blessing to get this patch in for the Dependency Freeze on Thursday, given this adds the (optional) dependency on QtDBus coming with it :)

rkflx added a comment.Mar 14 2018, 1:15 AM

have some old music videos directories which I sometimes play just for the music

Not sure Gwenview is the best app for that ;)

no exposure as MPRIS in windowed mode?

This would hide it from the taskbar's context menu, making it less discoverable, and remove my favourite show-off feature: Open Gwenview, and then press "Play" on the remote without having to switch to View and also to Fullscreen mode. Both of those feature are also quite useful for Okular. You are right though, that having the mediacontroller applet showing thumbnails when reading a paper in Okular would be annoying. Ideally in windowed mode only the "Start Presentation/Slideshow" button would be announced.

On the other hand, it's an option to mitigate the lockscreen issue. Hm, maybe we should wait what users think about it.

Gwenview, now is your chance to chime in ;)

kossebau updated this revision to Diff 29448.Mar 14 2018, 1:49 AM
  • remove no longer needed indexOfUrl method
  • if !fullscreen, be in stopped mode

Yet to fix: when in fullscreen, but browse mode, that should also be only
Stopped mode, not Playing/Paused

kossebau updated this revision to Diff 29487.Mar 14 2018, 11:09 AM
  • use folder name as "album title"
  • set also xesam:userRating
  • set also xesam:userRating

Quick feedback:

  • does not compile OOTB
  • crash for GoUp (or just usage of KUrlNavigator)
  • rating does not appear in DBus metadata

Does git diff happen to show something like this for you?

1diff --git a/lib/mpris2/mprismediaplayer2player.cpp b/lib/mpris2/mprismediaplayer2player.cpp
2index bd2d668f..9d360e69 100644
3--- a/lib/mpris2/mprismediaplayer2player.cpp
4+++ b/lib/mpris2/mprismediaplayer2player.cpp
5@@ -22,6 +22,8 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
6 #include <config-gwenview.h>
7
8 // lib
9+#include <semanticinfo/semanticinfodirmodel.h>
10+#include <semanticinfo/sorteddirmodel.h>
11 #include <slideshow.h>
12 #include <mimetypeutils.h>
13 #include <contextmanager.h>
14@@ -298,9 +300,9 @@ void MprisMediaPlayer2Player::onMetaInfoUpdated()
15
16 #ifndef GWENVIEW_SEMANTICINFO_BACKEND_NONE
17 const QModelIndex index = mContextManager->dirModel()->indexForUrl(url);
18- if (!index.isValid()) {
19- const float rating = index.data(SemanticInfoDirModel::RatingRole).toInt() / 10.0;
20- updatedMetaData.insert(QStringLiteral("xesam:userRating", rating);
21+ if (index.isValid()) {
22+ const qreal rating = index.data(SemanticInfoDirModel::RatingRole).toInt() / 10.0;
23+ updatedMetaData.insert(QStringLiteral("xesam:userRating"), rating);
24 }
25 #endif
26 // consider export of other metadata where mapping works

  • set also xesam:userRating

Quick feedback:

  • does not compile OOTB

My bad. sorry for wasting your time. I forgot that I yesterday had only added the code draft to the commit, and when before lunch I was to quickly show off the current metadata mapping, I just arc diffed after stashing my current code for disabling MPRIS service during lockscreen time., without remembering it also contained the fixes to make the code compile. Will see to fix the current patch here qickly, before completing the lockscreen code (coming up soon, so at least some good news here :) ).

  • crash for GoUp (or just usage of KUrlNavigator)

Had not yet experienced that. If you can, please give a backtrace (best wait for updated patch, though your diff pasted should be similar to mine)

  • rating does not appear in DBus metadata

After the fix of the inverted !index.valid() it should, no?

kossebau updated this revision to Diff 29495.Mar 14 2018, 12:53 PM

fix last code update into compilable/working version

rkflx added a comment.Mar 14 2018, 2:36 PM
  • crash for GoUp (or just usage of KUrlNavigator)

Had not yet experienced that. If you can, please give a backtrace (best wait for updated patch, though your diff pasted should be similar to mine)

No worries, it was just Qt DBus choking on the float.

  • rating does not appear in DBus metadata

After the fix of the inverted !index.valid() it should, no?

Yup, my patch (as well as your nearly identical update) fixed all the problems I mentioned.

kossebau updated this revision to Diff 29510.Mar 14 2018, 5:19 PM

add listener for screenlock, based on kwin's ScreenLockerWatcher, and
unregister MPRIS if in locked screen mode

kossebau updated this revision to Diff 29512.Mar 14 2018, 5:21 PM

add missing init of mScreenSaverInterface

rkflx accepted this revision.EditedMar 15 2018, 12:36 AM

Thanks so much, Friedrich. Latest changes work great, only one more small thing found (but solvable, I guess).

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


There's now also T8222: Improve MPRIS support 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.

lib/mpris2/mprismediaplayer2.cpp
83

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.

Are there any alternatives? Or could we return QString() for older Qts? The basic functionality still seemed fine to me when testing, so perhaps this is not strictly needed?

Otherwise we'd have to bite the bullet and bump the Qt version to 5.7.

This revision is now accepted and ready to land.Mar 15 2018, 12:37 AM

Thanks so much, Friedrich. Latest changes work great, only one more small thing found (but solvable, I guess).

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

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

Will edit the commit message into something, do you want to review the text first? Would text follwing the one of D11250 work for you?

There's now also D10972: Expose slideshow to MPRIS controllers 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.

Okay. Might make sense to copy the phab T numbers then in the respective code comments, so people can find things both ways?

lib/mpris2/mprismediaplayer2.cpp
83

Ah, missed the Qt 5.6 min dep, right.

https://specifications.freedesktop.org/mpris-spec/latest/Media_Player.html#Property:DesktopEntry says that "[t]This property is optional.".

So I would simply "if version > 5.6" this part, okay?

Will edit the commit message into something, do you want to review the text first? Would text follwing the one of D11250 work for you?

Oh, I was just thinking about cleaning up "RFC", "Questions" etc., nothing fancy ;)

There's now also T8222: Improve MPRIS support 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.

Okay. Might make sense to copy the phab T numbers then in the respective code comments, so people can find things both ways?

Your call, but why not. Gwenview's workboard is quite new, so there's not really a precedence.

lib/mpris2/mprismediaplayer2.cpp
83

Yup.

kossebau updated this revision to Diff 29549.Mar 15 2018, 1:14 AM

DesktopEntry property only with Qt >=5.7

kossebau retitled this revision from [RFC] Exposing slideshow to MPRIS controllers to Expose slideshow to MPRIS controllers.Mar 15 2018, 1:54 AM
kossebau edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
kossebau marked an inline comment as done.
kossebau added a comment.EditedMar 15 2018, 5:54 PM

Follow-up remark: I just found some flaw in KDE Connect's MPRIS controller plugin, on discovering a new MPRIS service it seems to not properly init to existing Can* flags of the player, only handles change signals. Which existing media players seem to send a lot :) Hopefully can get a fix done and in for kdeconnect-kde 1.2.2, whenever that will appear. Seems those issues were rather in KDE Connect for Android. Latest 1.8 works flawlessly, so make sure to update (not only due to that :) ).