How about this?<br><br>I didn't know where to add the Mutex, and I didn't think ResourceData would be appropriate. So, I implemented the Resource::Private class and added it over there. This should not break binary compatibility as it already existed. Or is there a better way?<br>
<br>- Vishesh Handa<br><br><div class="gmail_quote">On Sat, May 29, 2010 at 2:25 PM, Vishesh Handa <span dir="ltr"><<a href="mailto:handa.vish@gmail.com">handa.vish@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hey Sebastian<br><br>I'm sorry for the late reply I was busy with college stuff.<br><br>I looked at the modified patch, and I like the fact that it works, but I don't like the fact that we have to remember additional stuff. DetermineUri() must not me called inside ResourceData, and that you'll probably have to check if the ResourceData is stored() and its uri has been determined, before calling a method. These small things then to add up, and clutter the Resource Code.<br>
<br>Ideally I would have liked ResourceData to do all this checking itself, so that when altering the Resource class we don't have to worry about it. ( But then we already had to worry about store() ) I don't have a solution, but I'm trying to figure out one.<br>
<br>I never even thought about multi-threading before this, and, yes, I too think it will crash. ( Time to add more tests? Can we add multi-threading tests? ) I was thinking of some way of checking if the determineUri lock exists, and then accordingly calling determineUri() from Resource, but I think your method of mutex locking the Resource should cover it.<br>
<br>I'll try to implement it (It pretty simple, right?)<br><font color="#888888"><br>- Vishesh Handa</font><div><div></div><div class="h5"><br><br><br><br><div class="gmail_quote">On Thu, May 27, 2010 at 5:29 PM, Sebastian Trüg <span dir="ltr"><<a href="mailto:trueg@kde.org" target="_blank">trueg@kde.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">Hi Vishesh,<br>
<br>
I finally looked at the remove-proxy patch again and fixed it. Now the<br>
only problem left is the design weirdness of ResourceData instances<br>
deleting themselves.<br>
<br>
The main thing I did was removing all calls to determineUri from<br>
ResourceData and moving them into Resource. I did the same with load()<br>
and store() which is actually not necessary. Maybe it would be cleaner<br>
to move those back...<br>
<br>
I did not look into the merging of load and determineUri yet. I think we<br>
should do that in a separate patch only after we cleaned this one up.<br>
Otherwise it is impossible to debug.<br>
<br>
Cheers,<br>
<font color="#888888">Sebastian<br>
</font><div><br>
On 05/26/2010 12:52 PM, Vishesh Handa wrote:<br>
><br>
><br>
> On Wed, May 26, 2010 at 3:13 PM, Sebastian Trüg <<a href="mailto:trueg@kde.org" target="_blank">trueg@kde.org</a><br>
</div><div><div></div><div>> <mailto:<a href="mailto:trueg@kde.org" target="_blank">trueg@kde.org</a>>> wrote:<br>
><br>
> On 05/26/2010 10:08 AM, Vishesh Handa wrote:<br>
> > > 3. When checking if the the m_kickoffUri is a nie:url for a<br>
> resource.<br>
> > > The nieUrl is assigned to be equal to the uri. This however<br>
> gets fixed<br>
> > > in the load(). And nieUrl isn't used anywhere, so it doesn't<br>
> > matter that<br>
> > > much.<br>
> ><br>
> > I suppose you mean this part:<br>
> ><br>
> > if( it.next() ) {<br>
> > QUrl uri = it["r"].uri();<br>
> > if( uri.isEmpty() ) {<br>
> > m_uri = m_kickoffUri;<br>
> > }<br>
> > else {<br>
> > m_uri = uri;<br>
> > m_nieUrl = uri;<br>
> > }<br>
> ><br>
> > The last two lines. AFAICT this is perfectly fine since in the<br>
> latter<br>
> > case both nie:url and resource URI are equal.<br>
> ><br>
> ><br>
> > I respectfully disagree. :)<br>
> ><br>
> > The query used is this "select distinct ?r ?o where { { ?r nie:url<br>
> <uri><br>
> > . } UNION { <uri> ?p ?o . } } LIMIT 1". The case where ?r isn't empty<br>
> > is when the <uri> contains the nie:url and therefore ?r will<br>
> contain the<br>
> > resource uri.<br>
><br>
> You are of course correct. It should be "m_nieUrl = m_kickoffUri"<br>
> instead. Agreed?<br>
><br>
><br>
> Yup. :)<br>
><br>
> If you get the time could you please look at my horrible merge patch?<br>
><br>
> Thanks<br>
> - Vishesh Handa<br>
><br>
> Cheers,<br>
> Sebastian<br>
><br>
><br>
</div></div></blockquote></div><br>
</div></div></blockquote></div><br>