[prev in list] [next in list] [prev in thread] [next in thread]
List: amarok-devel
Subject: Re: Review Request 111512: MPRIS2: avoid updating Metadata when between tracks
From: "Mark Kretschmann" <kretschmann () kde ! org>
Date: 2013-07-15 17:02:15
Message-ID: 20130715170215.9599.99858 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111512/#review35986
-----------------------------------------------------------
Ship it!
Looking good, please push to master.
- Mark Kretschmann
On July 15, 2013, 1:18 p.m., Alex Merry wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111512/
> -----------------------------------------------------------
>
> (Updated July 15, 2013, 1:18 p.m.)
>
>
> Review request for Amarok.
>
>
> Description
> -------
>
> MPRIS2: avoid updating Metadata when between tracks
>
> When changing tracks, we would emit PropertiesChanged for Metadata
> twice, once with mpris:trackid set to /org/kde/amarok/PendingTrack and
> once with the actual new trackid (because the first time the playlist
> code had not yet updated the active track). If the track was changed
> manually (not just progressing to the next one) we would often also emit
> a PropertiesChanged with an empty Metadata before repopulating it.
>
> This solves the first issue by making the signal connection for
> trackChanged from EngineController queued, meaning that by the time the
> MPRIS2 code gets the signal, the playlist has updated the activeTrack
> and we can easily figure out the correct trackid.
>
> It solves the second issue by ignoring the trackLengthChanged signal
> when it has a meaningless value (<0), which seems to happen at some
> point during track changes that are not predictable.
>
> BUG: 321602
>
>
> Diffs
> -----
>
> ChangeLog 5fe5d2e64c771d722f3f90bf6c98d5013e56553c
> src/dbus/mpris2/MediaPlayer2Player.cpp a633756bf558a89ba2a3db2307c0ebbc373a759b
>
> Diff: http://git.reviewboard.kde.org/r/111512/diff/
>
>
> Testing
> -------
>
> Tested using a tool that listens to the PropertiesChanged signal of the MPRIS2 \
> interface and lists when the mpris:trackid changes.
> Without the patch, I get output like
> Track change: "/org/kde/amarok/Track/5739423209746661216" -> \
> "/org/kde/amarok/PendingTrack" Track change: "/org/kde/amarok/PendingTrack" -> \
> "/org/kde/amarok/Track/8264712350997591513" when the track progresses because the \
> previous track finished, and Track change: \
> "/org/kde/amarok/Track/5739423209746661216" -> "" Track change: "" -> \
> "/org/kde/amarok/Track/8264712350997591513" when I manually change the track (eg: \
> by clicking "next" or by double-clicking another track).
> With the patch, I only get things like
> Track change: "/org/kde/amarok/Track/5739423209746661216" -> \
> "/org/kde/amarok/Track/8264712350997591513"
>
> Thanks,
>
> Alex Merry
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/111512/">http://git.reviewboard.kde.org/r/111512/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looking good, please \
push to master.</pre> <br />
<p>- Mark</p>
<br />
<p>On July 15th, 2013, 1:18 p.m. UTC, Alex Merry wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Amarok.</div>
<div>By Alex Merry.</div>
<p style="color: grey;"><i>Updated July 15, 2013, 1:18 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">MPRIS2: avoid updating Metadata when between tracks
When changing tracks, we would emit PropertiesChanged for Metadata
twice, once with mpris:trackid set to /org/kde/amarok/PendingTrack and
once with the actual new trackid (because the first time the playlist
code had not yet updated the active track). If the track was changed
manually (not just progressing to the next one) we would often also emit
a PropertiesChanged with an empty Metadata before repopulating it.
This solves the first issue by making the signal connection for
trackChanged from EngineController queued, meaning that by the time the
MPRIS2 code gets the signal, the playlist has updated the activeTrack
and we can easily figure out the correct trackid.
It solves the second issue by ignoring the trackLengthChanged signal
when it has a meaningless value (<0), which seems to happen at some
point during track changes that are not predictable.
BUG: 321602</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Tested using a tool that listens to the PropertiesChanged signal of the \
MPRIS2 interface and lists when the mpris:trackid changes.
Without the patch, I get output like
Track change: "/org/kde/amarok/Track/5739423209746661216" -> \
"/org/kde/amarok/PendingTrack" Track change: \
"/org/kde/amarok/PendingTrack" -> \
"/org/kde/amarok/Track/8264712350997591513" when the track progresses \
because the previous track finished, and Track change: \
"/org/kde/amarok/Track/5739423209746661216" -> "" Track \
change: "" -> "/org/kde/amarok/Track/8264712350997591513" when \
I manually change the track (eg: by clicking "next" or by double-clicking \
another track).
With the patch, I only get things like
Track change: "/org/kde/amarok/Track/5739423209746661216" -> \
"/org/kde/amarok/Track/8264712350997591513"</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>ChangeLog <span style="color: \
grey">(5fe5d2e64c771d722f3f90bf6c98d5013e56553c)</span></li>
<li>src/dbus/mpris2/MediaPlayer2Player.cpp <span style="color: \
grey">(a633756bf558a89ba2a3db2307c0ebbc373a759b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111512/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic