[prev in list] [next in list] [prev in thread] [next in thread]
List: amarok-devel
Subject: Re: Review Request 111655: Reimplement RecentlyPlayedListWidget
From: "Mark Kretschmann" <kretschmann () kde ! org>
Date: 2013-07-26 9:00:33
Message-ID: 20130726090033.29743.69956 () 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/111655/#review36526
-----------------------------------------------------------
Ship it!
Ship It!
- Mark Kretschmann
On July 24, 2013, 2:55 p.m., Konrad Zemek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111655/
> -----------------------------------------------------------
>
> (Updated July 24, 2013, 2:55 p.m.)
>
>
> Review request for Amarok.
>
>
> Description
> -------
>
> Reimplement RecentlyPlayedListWidget
>
> Rewrote RecentlyPlayedListWidget from the basics. There was a major
> inconsistency in this widget, where tracks were added to it if they were
> recently played at all, but the time shown was the "Last Played"
> statistics which is only updated after whole song is played. This
> caused gravely incorrect data to be displayed (see bug 302485).
>
> There were two different methods of solving the inconsistency: focusing
> on the "Last Played" time, and adding songs to the widget only if the
> "Last Played" changed; and focusing on recent plays completely
> disregarding "Last Played". I opted for the latter as I felt it fits the
> widget's nature better.
>
> Because we can't rely on already available data, the widget needs to do
> its own housekeeping. It saves its data on shutdown, to be restored
> on next startup. One other major benefit of this approach is that
> widget's data remains correct even if collections change, while
> previously tracks from removed collections would disappear.
>
> Finally, I added the feature to add tracks to playlist directly from the
> widget, provided that the track exists. Adding to playlist works
> consistently with other parts of Amarok, with standard double-click and
> middle-click behavior.
>
> BUG: 302485
> FEATURE: 296090
> FEATURE: 279263
> REVIEW: 111655
> FIXED-IN: 2.8
>
>
> Diffs
> -----
>
> ChangeLog 87e4708a6330e7a612db59052a50eef053294b06
> src/context/widgets/RecentlyPlayedListWidget.h caa78039b891511ca36ada8f61cd4f776d1f3c6d
> src/context/widgets/RecentlyPlayedListWidget.cpp 6ea501affcf6e61d237002e15f7ed4e26989b91b
>
> Diff: http://git.reviewboard.kde.org/r/111655/diff/
>
>
> Testing
> -------
>
> Manual.
>
>
> Thanks,
>
> Konrad Zemek
>
>
[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/111655/">http://git.reviewboard.kde.org/r/111655/</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;">Ship It!</pre> <br />
<p>- Mark</p>
<br />
<p>On July 24th, 2013, 2:55 p.m. UTC, Konrad Zemek 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 Konrad Zemek.</div>
<p style="color: grey;"><i>Updated July 24, 2013, 2:55 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;">Reimplement RecentlyPlayedListWidget
Rewrote RecentlyPlayedListWidget from the basics. There was a major
inconsistency in this widget, where tracks were added to it if they were
recently played at all, but the time shown was the "Last Played"
statistics which is only updated after whole song is played. This
caused gravely incorrect data to be displayed (see bug 302485).
There were two different methods of solving the inconsistency: focusing
on the "Last Played" time, and adding songs to the widget only if the
"Last Played" changed; and focusing on recent plays completely
disregarding "Last Played". I opted for the latter as I felt it fits the
widget's nature better.
Because we can't rely on already available data, the widget needs to do
its own housekeeping. It saves its data on shutdown, to be restored
on next startup. One other major benefit of this approach is that
widget's data remains correct even if collections change, while
previously tracks from removed collections would disappear.
Finally, I added the feature to add tracks to playlist directly from the
widget, provided that the track exists. Adding to playlist works
consistently with other parts of Amarok, with standard double-click and
middle-click behavior.
BUG: 302485
FEATURE: 296090
FEATURE: 279263
REVIEW: 111655
FIXED-IN: 2.8</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;">Manual.</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">(87e4708a6330e7a612db59052a50eef053294b06)</span></li>
<li>src/context/widgets/RecentlyPlayedListWidget.h <span style="color: \
grey">(caa78039b891511ca36ada8f61cd4f776d1f3c6d)</span></li>
<li>src/context/widgets/RecentlyPlayedListWidget.cpp <span style="color: \
grey">(6ea501affcf6e61d237002e15f7ed4e26989b91b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111655/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