Review Request: Improve SqlMeta thread safety

[Ralf Engels]

> On 2010-10-28 18:52:07, Ian Monroe wrote:
> > src/core-impl/collections/sqlcollection/SqlMeta.cpp, line 198
> > <http://git.reviewboard.kde.org/r/100098/diff/3/?file=2522#file2522line198>
> >
> >     Could you document why you added these checks? I'm guessing its just a sanity check, but just say so here.

Actually it's not only sanity.
It's just being defensive. The only part of the code that guarantees that those ids are set is the SqlRegistry and the ScanResultProcessor. If one of those pieces is changed then you will start getting in trouble.

However a short "// sanity" shouldn't be so bad.


> On 2010-10-28 18:52:07, Ian Monroe wrote:
> > src/core-impl/collections/sqlcollection/SqlRegistry.cpp, line 327
> > <http://git.reviewboard.kde.org/r/100098/diff/3/?file=2524#file2524line327>
> >
> >     This should maybe be a QLatin1String instead. I say maybe because obviously sql queries that include end-user strings should *not* be a qlatin1string. So I can't say that sqlregistry using just QString is a bad idea.

id is an int.
No problem there.


- Ralf


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100098/#review206
-----------------------------------------------------------


On 2010-10-25 16:55:56, Ralf Engels wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100098/
> -----------------------------------------------------------
> 
> (Updated 2010-10-25 16:55:56)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> -------
> 
> This patch improves the SqlMeta thread safety.
> It has several parts:
> - Use QReadWriteLock for SqlTrack.
>  For this I also needed to pull the read accessor functions into the .cpp
> - SqlCollection is a const pointer.
>  All metas are directly dependent on the SqlCollection and it could and should never change.
>  Using weak pointers here is not needed.
>  Note that now the access to m_collection is also thread safe as it's const
> - SqlAlbum::setCompilation did modify the old album
>  This was changed an a new album will be created.
>  The positive side effect is that the UI will now update directly if setting a compilation
> - SqlAlbum::setImage will now copy embedded images directly when set
>  If the track-uid is set (maybe from another thread) the cover image will still be found.
>  Note that the image cache is and was not connected to the track uid.
> 
> 
> This addresses bug 254631.
>     https://bugs.kde.org/show_bug.cgi?id=254631
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/sqlcollection/SqlCollection.cpp b88d257 
>   src/core-impl/collections/sqlcollection/SqlMeta.h b92e351 
>   src/core-impl/collections/sqlcollection/SqlMeta.cpp 6b29b6e 
>   src/core-impl/collections/sqlcollection/SqlRegistry.h 66ba632 
>   src/core-impl/collections/sqlcollection/SqlRegistry.cpp e1b3571 
> 
> Diff: http://git.reviewboard.kde.org/r/100098/diff
> 
> 
> Testing
> -------
> 
> setting and unsetting compilation
> changing album name
> scanning and fully scanning collection
> 
> 
> Thanks,
> 
> Ralf
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101029/1ff08660/attachment-0001.htm