Implementing Seek() is useful both for short seeking (+/-10s) and for
positioning, since the kde connect daemon implements setPosition() from
the android client via Seek().
Also, emit the Seeked() signal, for better synchronization.
Details
Details
- Reviewers
broulik
TODO
Diff Detail
Diff Detail
- Repository
- R856 Plasma Browser Integration
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Thanks for your patch! There's a minor issue but other than that it's good.
Can you also verify this doesn't cause a loop when using Plasma's media controller applet?
| extension/content-script.js | ||
|---|---|---|
| 177 | The seekable property isn't an int but a TimeRanges object that tells you which portions of the player are seekable so this will never be false | |
| host/mprisplugin.cpp | ||
| 205 | Can't you just do emit Seeked(position)? | |
Comment Actions
Regarding loops with the Media Player widget, I haven't noticed any (using KDE Neon, 5.13.1). What test would catch this?
| extension/content-script.js | ||
|---|---|---|
| 177 | Good catch. I uploaded a fix for this. | |
| host/mprisplugin.cpp | ||
| 205 | MPrisPlugin doesn't define Seeked, so no. But it's fine with me if you think adding it to the parent and using the signal auto-relaying would be better. Regarding macros vs. keywords, I just figured macros would be more compatible, but it could be switched to using emit as well. | |
| extension/content-script.js | ||
|---|---|---|
| 177 | Please avoid negating a longer statement, this makes the code hard to understand at a glance: if (activePlayer.seekable.length === 0) | |
| host/mprisplugin.cpp | ||
| 205 | Doesn't it work if you define the signal in mprisplugin.h and emit there? The generated DBus code from the XML should auto-relay them. Agreed, it's somewhat magic. | |
Comment Actions
Clean things up a bit:
- use signal auto-relaying to clean up mprisplugin logic
- use more readable test in content-script.js
| extension/content-script.js | ||
|---|---|---|
| 177 | Good point. Fixed. | |