<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/106042/">http://git.reviewboard.kde.org/r/106042/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 20th, 2012, 10:46 p.m., <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106042/diff/4/?file=79206#file79206line71" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">private:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">71</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QHash</span><span class="o"><</span><span class="n">QUrl</span><span class="p">,</span> <span class="n">Meta</span><span class="o">::</span><span class="n">TrackPtr</span><span class="o">></span> <span class="n">m_trackHash</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is an important design issue:
You either need MemoryCollection or all these hashes, but not both. I'm still not convinced that you cannot avoid MemoryCollection at all, but if you use it, don't duplicate MapChanger.</pre>
</blockquote>
<p>On August 22nd, 2012, 9:36 a.m., <b>Edward Hades Toroshchin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There are two issues here.
First, it would be much better to have implemented an own QueryMaker that would translate Amarok's queries to Nepomuk's queries.
This would, of course, render MemoryCollection unneeded. However, we did not go that way in this project, and you can ask me (or phalgun) personally, if you want to learn the reasons.
Second, we tried to rely on MemoryCollection, and it didn't go very well. Main reason is that it mostly checks uniqueness by the uniqueness of titles (or title+artist for albums). However, it doesn't do this very reliably (or I didn't understand how to use it correctly), so some quirks did creep out here and there. So, we switched to nepomuk-uid-uniqueness, handled by NepomukCollection.
TL;DR: this is quasi-temporary solution as it is, as we will move to NepomukQueryMaker eventually. So let's leave these hashes where they are.</pre>
</blockquote>
<p>On August 22nd, 2012, 10:05 a.m., <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> First, it would be much better to have implemented an own QueryMaker that would translate Amarok's queries to Nepomuk's queries.
Total agreement.
> Second, we tried to rely on MemoryCollection, and it didn't go very well. Main reason is that it mostly checks uniqueness by the uniqueness of titles (or title+artist for albums).
This is how MemoryCollection works. It is used for "dumb" back-ends that don't know tracks of an album etc.
> However, it doesn't do this very reliably (or I didn't understand how to use it correctly),
Well, it works as it is supposed to work. E.g. you should NOT find existing album (etc.) entities for new tracks you add to MemoryColleciton through MapChanger. You should return new instance every time (see IpodMeta::Track), MapChanger does the matching. Any matching you do yourself is completly unused.
> so some quirks did creep out here and there. So, we switched to nepomuk-uid-uniqueness, handled by NepomukCollection.
Has *no* effect at all, because the world sees the MemoryCollection maps, and it uses name-uniqueness. *No one* (apart from MapChanger for a short while) sees NepomukAlbum etc., and NepomukAlbum::tracks() is *never* called when using MapChanger. NepomukTrack is hidden behind MemoryMeta::Track. Test it!
> TL;DR: this is quasi-temporary solution as it is, as we will move to NepomukQueryMaker eventually. So let's leave these hashes where they are.
Understand that MemoryCollection is rather good temporary solution, but it is used incorrectly. (e.g. there's a lot of completely useless code)</pre>
</blockquote>
<p>On August 22nd, 2012, 10:17 a.m., <b>Edward Hades Toroshchin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Again: we had used MemoryCollection as you describe and it didn't work well. I didn't want to dig into it (and I don't want to now).
(if you don't take my word for it, or want to dig into it, please checkout some older commits from phalgun's branch).
I understand how MemoryCollection works, and I understand that there is a certain effort duplication, as well as (some) wasted memory.
So, to summarize, there are two options: either accept this uncool temporary solution, or switch it back to a cool temporary solution that works worse. I don't see much sense in the latter, because it's still temporary solution, and (if we believe phalgun) promises to be quite a short-lived one.
</pre>
</blockquote>
<p>On August 22nd, 2012, 10:26 a.m., <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> So, to summarize, there are two options: either accept this uncool temporary solution, or switch it back to a cool temporary solution that works worse.
The "cool temporary solution" *cannot*, by definition, work worse. Read my above comment again, all that additional code has *no visible effect*. How can I convince you? (do you see that all these hashes are used only for short while when the NepomukConstructMetaJob runs and then they're discarded?)
> I don't see much sense in the latter, because it's still temporary solution, and (if we believe phalgun) promises to be quite a short-lived one.
The "cool temporary solution" would be a couple of hundreds code lines less, witch nearly no effort. Worth it IMO.</pre>
</blockquote>
<p>On August 22nd, 2012, 10:58 a.m., <b>Edward Hades Toroshchin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You're talking theory, I'm talking practice. Been there, done it, gone away.
I'm just saying we shouldn't waste our time perfecting a _temporary_ solution. And it will amount to some time wasted, because we already wasted it once, trying to use MemoryCollection as you propose. Of course, we could have done this wrong.
If you still insist on reworking this, go ahead. I suggest you help phalgun on it then :)</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Also, you are abusing the term "by definition" :)</pre>
<br />
<p>- Edward Hades</p>
<br />
<p>On August 20th, 2012, 11:17 a.m., Phalgun Guduthur wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok, Edward Hades Toroshchin, Vishesh Handa, and Matěj Laitl.</div>
<div>By Phalgun Guduthur.</div>
<p style="color: grey;"><i>Updated Aug. 20, 2012, 11:17 a.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;">Nepomuk plugin for Amarok.
Almost all of the code changes can be found in src/core-impl/collections/nepomukcollection/*
And a minor change in src/core-impl/collections/support/MemoryMeta.cpp
Code builds and after Nepomuk plugin is activated in the "Settings" dialog, Nepomuk Plugin comes into play and queries all the tracks in your machine. The query is not 'that' fast and might take several seconds depending on the number of tracks in your box.
IMPORTANT : Make sure Nepomuk is enabled if you want to give the plugin a spin. </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;">Minimal. Plan to spend the remaining time on testing the plugin. </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>src/core-impl/collections/CMakeLists.txt <span style="color: grey">(c78b920)</span></li>
<li>src/core-impl/collections/nepomukcollection/CMakeLists.txt <span style="color: grey">(7cfd4b0)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukAlbum.h <span style="color: grey">(185c25a)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukAlbum.cpp <span style="color: grey">(6a09a1b)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukArtist.h <span style="color: grey">(6fcedf3)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukArtist.cpp <span style="color: grey">(13ddf01)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukCollection.h <span style="color: grey">(928b145)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukCollection.cpp <span style="color: grey">(cb185e8)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukComposer.h <span style="color: grey">(1b11325)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukComposer.cpp <span style="color: grey">(f21251e)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukGenre.h <span style="color: grey">(ce0e3b7)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukGenre.cpp <span style="color: grey">(945074c)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukQueryMaker.h <span style="color: grey">(50067de)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp <span style="color: grey">(33163ea)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukRegistry.h <span style="color: grey">(a21347e)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukRegistry.cpp <span style="color: grey">(8afa199)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukTrack.h <span style="color: grey">(77dd8c7)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukTrack.cpp <span style="color: grey">(7db01cf)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukYear.h <span style="color: grey">(504cbe2)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukYear.cpp <span style="color: grey">(1f13de0)</span></li>
<li>src/core-impl/collections/nepomukcollection/amarok_collection-nepomukcollection.desktop <span style="color: grey">(1ac9f02)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukArtist.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukArtist.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukComposer.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukComposer.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukGenre.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukGenre.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukLabel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukLabel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukYear.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukYear.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106042/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>