D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W. H. Kossebau]

kossebau abandoned this revision.
kossebau added a comment.
Herald added a subscriber: okular-devel.


  No personal need anymore, and seemingly no-one else interested, thus 
discarding.

REPOSITORY
  R223 Okular

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

To: kossebau, #okular
Cc: okular-devel, aacid, pino, rkflx, #kde_connect, tfella, ngraham, darcyshen

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 29945.
kossebau added a comment.


  extend MprisThumbnailStore to support updates of thumbnails

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11250?vs=29681&id=29945

BRANCH
  addmprisservice

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

AFFECTED FILES
  CMakeLists.txt
  mpris2/dbusabstractadaptor.cpp
  mpris2/dbusabstractadaptor.h
  mpris2/mpris2service.cpp
  mpris2/mpris2service.h
  mpris2/mprismediaplayer2.cpp
  mpris2/mprismediaplayer2.h
  mpris2/mprismediaplayer2player.cpp
  mpris2/mprismediaplayer2player.h
  ui/presentationwidget.cpp
  ui/presentationwidget.h

To: kossebau, #okular
Cc: aacid, pino, rkflx, #kde_connect, michaelweghorn, ngraham

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> aacid wrote in dbusabstractadaptor.h:32
> This seems like random glue code, wouldn't this make more sense in Qt or in 
> KF5?

Possibly. Similar code working around the missing Qt feature is found across 
code. Actually one would expect this indeed with Qt, but the one known related 
bug report https://bugreports.qt.io/browse/QTBUG-48008 got for whatever reason 
a harsh wontfix.
Not my  playing ground, so I escaped into manual workaround like found 
elsewhere. Will be happy to have something in Qt or KF5, but do not have 
resources for that.

> aacid wrote in mprismediaplayer2player.cpp:58
> No, you can not, file can change at any moment.

Also during presentation? Okay. Any change signal you would recommend to catch 
here? Would encode some thumbnail versioning then to ensure uniquness.

> aacid wrote in mprismediaplayer2player.cpp:304
> Why do you need that?

To avoid creating repeated requests for the same pixmap.

Challenge I see: I need to get a thumbnail per page. To speed-up things, it 
will be done only for pages currently visited. But given the request is 
processed async, when a user switches pages while the requests are still 
on-going, when entering the same page multiple times multiple requests would be 
created, one per visit.

Is there some internal mechanism to squash such requests?

> aacid wrote in mprismediaplayer2player.h:99
> are this ugly uppercase tyied to the spec? Isn't there like a magic way in Qt 
> to create the api from the xml spec?

Yes, needed due to using `sessionBus.registerObject(mediaPlayer2ObjectPath(), 
this, QDBusConnection::ExportAdaptors);`, and D-Bus methods should be Uppercase 
and so does MPRIS.
 I only know `qt5_add_dbus_interface()` for creating nice-Qt-style API for 
interfaces to be used with remote objects. For local actual implementations of 
interfaces I am not aware how to avoid that, the other code I saw doing MPRIS 
implementations did things the same way as here. Might have missed something, 
but done to my best knowledge.

REPOSITORY
  R223 Okular

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

To: kossebau, #okular
Cc: aacid, pino, rkflx, #kde_connect, michaelweghorn, ngraham

D11250: Expose presentation via MPRIS to rich remote controllers

[Albert Astals Cid]

aacid added inline comments.

INLINE COMMENTS

> dbusabstractadaptor.h:32
> + */
> +class DBusAbstractAdaptor : public QDBusAbstractAdaptor
> +{

This seems like random glue code, wouldn't this make more sense in Qt or in KF5?

> mprismediaplayer2player.cpp:50
> +if (flags & Pixmap) {
> +// TODO: can we be sure about this?
> +emit pixmapAvailable(page);

Should be about right

> mprismediaplayer2player.cpp:58
> +: mThumbnailCacheDir(new QTemporaryDir)
> +// TODO: can we assume document is unchanged while in presentation?
> +, mThumbnailForPageCreated(document->pages())

No, you can not, file can change at any moment.

> mprismediaplayer2player.cpp:304
> +
> +// TODO: how to find out if there still is another pixmap request 
> ongoing?
> +// not a QObject, so cannot connect to destroyed signal

Why do you need that?

> mprismediaplayer2player.h:99
> +public Q_SLOTS: // D-Bus API
> +void Next();
> +void Previous();

are this ugly uppercase tyied to the spec? Isn't there like a magic way in Qt 
to create the api from the xml spec?

REPOSITORY
  R223 Okular

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

To: kossebau, #okular
Cc: aacid, pino, rkflx, #kde_connect, michaelweghorn, ngraham

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau added a comment.


  So far got a few comments about this being a nice idea in general, and one 
already about something in the code. So far all positive. Now, where are the 
negative ones, there must be some :)
  If not or even then, I hope for some good people for some further detailed 
review of the code and/or feature in the next days, please :)
  
  I know this came late in as new feature for 18.04, but still would be glad if 
we could manage to decide about it just in time (5 days left still until 
feature freeze next Thursday).
  
  Some more motivational info about this patch:
  While the current state of this feature is yet missing a lot over e.g. what 
is possible with https://wiki.documentfoundation.org/Impress_Remote , it still 
already adds value for people wanting to remote control their presentation, but 
who do not have a special wireless remote control input device.
  Older projects like https://code.google.com/archive/p/remuco/ or (k)anyRemote 
also supporting Okular show there are at least some people who like to have 
such functionality for making use of their existing mobile device instead.
  
  Also having this now in Okular, next to Gwenview (which already got a similar 
feature a few days ago), allows to keep Okular presentation feature in the loop 
and getting feedback from users already when working on enhancing the MPRIS 
spec for a future version for improved support for non-music media (see also 
https://community.kde.org/MPRIS#Features_for_MPRIS_3.0 for planning).
  
  For your inspiration about the proposed feature, here another thing which 
gets possible this way: in a meeting everyone could have their mobile connected 
with KDE Connect to the presentation device running Okular, and in discussion 
then everybody could navigate through the current presentation thanks to KDE 
Connect & MPRIS :) No more need to having to tell the current 
presentation-doing person which slide you want to see again or pass the remote 
control around or stand up and take over position next to presentation device 
when it is your turn. Just keep leaned back in your chair with legs up and 
touch your mobile :)
  
  And some note:
  when testing with KDE Connect for Android, make sure to use latest version 
1.8, there had been some glitches with previous/next action buttons before.

REPOSITORY
  R223 Okular

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

To: kossebau, #okular
Cc: pino, rkflx, #kde_connect, michaelweghorn, ngraham, aacid

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 29681.
kossebau added a comment.


  - always create mpris service, not only with Q_OS_LINUX
  - minor code clean-ups

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11250?vs=29333&id=29681

BRANCH
  addmprisservice

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

AFFECTED FILES
  CMakeLists.txt
  mpris2/dbusabstractadaptor.cpp
  mpris2/dbusabstractadaptor.h
  mpris2/mpris2service.cpp
  mpris2/mpris2service.h
  mpris2/mprismediaplayer2.cpp
  mpris2/mprismediaplayer2.h
  mpris2/mprismediaplayer2player.cpp
  mpris2/mprismediaplayer2player.h
  ui/presentationwidget.cpp
  ui/presentationwidget.h

To: kossebau, #okular
Cc: pino, rkflx, #kde_connect, michaelweghorn, ngraham, aacid

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau added a comment.


  In D11250#227259 , @pino wrote:
  
  > Please do not restrict it to `Q_OS_LINUX`, as there is no Linux-only code 
(nor the feature is specific to Linux in any way).
  
  
  Good hint, possibly was misguided by the special case for 
QDBusUnixFileDescriptor. But QtDBus is a required build dep unconditionally, 
and then what you said. Fixed in incoming update.

REPOSITORY
  R223 Okular

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

To: kossebau, #okular
Cc: pino, rkflx, #kde_connect, michaelweghorn, ngraham, aacid

D11250: Expose presentation via MPRIS to rich remote controllers

[Pino Toscano]

pino added a comment.


  Please do not restrict it to `Q_OS_LINUX`, as there is no Linux-only code 
(nor the feature is specific to Linux in any way).

REPOSITORY
  R223 Okular

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

To: kossebau, #okular
Cc: pino, rkflx, #kde_connect, michaelweghorn, ngraham, aacid

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 29333.
kossebau added a comment.


  Register objects before registering the service name, this should help Plasma
  MPRIS controllers & others to use the service instantly when the name is
  registered

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11250?vs=29326&id=29333

BRANCH
  addmprisservice

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

AFFECTED FILES
  CMakeLists.txt
  mpris2/dbusabstractadaptor.cpp
  mpris2/dbusabstractadaptor.h
  mpris2/mpris2service.cpp
  mpris2/mpris2service.h
  mpris2/mprismediaplayer2.cpp
  mpris2/mprismediaplayer2.h
  mpris2/mprismediaplayer2player.cpp
  mpris2/mprismediaplayer2player.h
  ui/presentationwidget.cpp
  ui/presentationwidget.h

To: kossebau, #okular
Cc: rkflx, #kde_connect, michaelweghorn, ngraham, aacid

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau added a comment.


  Two issues yet, one in code and one possibly in Plasma:
  a) the same okular process can have multiple documents open and thus run 
multiple presentations at the same time. The current code will only expose the 
first presentation to mpris. No idea yet how to handle that given MPRIS only 
expects one MPRIS object per D-Bus service. But then might also not be a 
problem for 99% of the usecases, so dp npt see this as real showstopper.
  
  b) sometimes Plasma stuff (seems mpris dataengine mainly) misses to detect 
the new MPRIS D-Bus service. As KDEConnect does not have the problem and 
qdbusviewer also shows DBus service & object as expected, for now I suspect it 
is an issue in the Plasma mpris dataengine. Yet to be investigated.

REPOSITORY
  R223 Okular

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

To: kossebau, #okular
Cc: rkflx, #kde_connect, michaelweghorn, ngraham, aacid

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 29326.
kossebau added a comment.


  - added thumbnail support
  - fixed unregistering from DBus when presentationwidget is only closed, not 
deleted

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11250?vs=29286&id=29326

BRANCH
  addmprisservice

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

AFFECTED FILES
  CMakeLists.txt
  mpris2/dbusabstractadaptor.cpp
  mpris2/dbusabstractadaptor.h
  mpris2/mpris2service.cpp
  mpris2/mpris2service.h
  mpris2/mprismediaplayer2.cpp
  mpris2/mprismediaplayer2.h
  mpris2/mprismediaplayer2player.cpp
  mpris2/mprismediaplayer2player.h
  ui/presentationwidget.cpp
  ui/presentationwidget.h

To: kossebau, #okular
Cc: rkflx, #kde_connect, michaelweghorn, ngraham, aacid

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau added a comment.


  Right now trying to also get preview of the current page implemented, by 
storing the PixmapRequest result in a temporary file and passing that in the 
metadata as mpris:artUrl.
  But not sure if I make it tonight, and already useful as is, so this upload 
for first round of feedback.

REPOSITORY
  R223 Okular

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

To: kossebau, #okular
Cc: #kde_connect, michaelweghorn, ngraham, aacid

D11250: Expose presentation via MPRIS to rich remote controllers

[Friedrich W . H . Kossebau]

kossebau created this revision.
kossebau added a reviewer: Okular.
Restricted Application added a project: Okular.
kossebau requested review of this revision.

REVISION SUMMARY
  When going into presentation mode, a D-Bus object is created that
  implements MPRIS spec and exposes control to the presentation.
  By that any existing MPRIS controllers can be used next to the existing
  direct control by Okular UI itself.
  This includes e.g.
  
  - keyboard mediakeys (Play/Pause, Stop, Next, Previous), as handled by Plasma 
mpris dataengine
  - KDE Connect MPris plugin
  
  Motivation:
  For giving a presentation and not having to stand next to the computer
  which runs Okular, one can use wireless input controllers.
  Sometimes though one would like more rich remote controllers, e.g.
  with a display showing notes for the current page, giving a preview
  of the next/previous page, or allowing to jump to a page directly
  without everyone following on the big screen the search in the pages list.
  Such a rich remote controller could be done e.g. by an app for a mobile,
  which then connects to Okular.
  Instead of writing a custom controller app and designing a custom protocol,
  another option is to use the MPRIS spec and map the concepts of the
  Okular document and its pages onto the MPRIS tracklist and tracks.
  After all the "media player" in MPRIS is an abstraction, and to a good
  degree it is possible to make the Okular presentation mode an
  instantiation of it.
  While the current version 2 of MPRIS seems done rather with typical
  music players or movie players in mind, a future version shall also
  take presentation shows more into the design.
  This initial patch already enables simple Play/Pause/Next/Previous
  remote control, which is handy for remote controlling via KDE Connect.
  
  Future patches should also implement tracklist support, so a controller
  is e.g. able to show the complete list of tracks^Wpages as thumbnails.

TEST PLAN
  Load a document, enter presentation mode and see Okular appear in the
  KDE Connect Multimedia Remote Control, following commands or updating
  the states.

REPOSITORY
  R223 Okular

BRANCH
  addmprisservice

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

AFFECTED FILES
  CMakeLists.txt
  mpris2/dbusabstractadaptor.cpp
  mpris2/dbusabstractadaptor.h
  mpris2/mpris2service.cpp
  mpris2/mpris2service.h
  mpris2/mprismediaplayer2.cpp
  mpris2/mprismediaplayer2.h
  mpris2/mprismediaplayer2player.cpp
  mpris2/mprismediaplayer2player.h
  ui/presentationwidget.cpp
  ui/presentationwidget.h

To: kossebau, #okular
Cc: #kde_connect, michaelweghorn, ngraham, aacid