Review Request: Fix crash when closing Amarok with running CoverFetcher

[Matěj Laitl]

> On Aug. 9, 2012, 10:45 a.m., Matěj Laitl wrote:
> > Looks good, just a few small things. Could you also please describe the steps that lead to the crash in the commit msg?
> 
> Ralf Engels wrote:
>     Really? It's in the bug report pretty clearly. No need to copy it in my opinion.

Well, I don't understand _how_ adding the mutex solved the bug, but maybe I'm a bit dumb here.


> On Aug. 9, 2012, 10:45 a.m., Matěj Laitl wrote:
> > src/statusbar/CompoundProgressBar.cpp, line 46
> > <http://git.reviewboard.kde.org/r/105942/diff/1/?file=76720#file76720line46>
> >
> >     Nitpicky: I would personally strip all the added newlines
> 
> Ralf Engels wrote:
>     Actually I think that newlines are a good device to structure code.
>     I use it to split different functional blocks inside a function.
>     
>     Often I use them to split up between the initialization, the actual function and the cleaning up inside a function.
>     That's why there is a newline between the Locker and the rest of the code.
> 
> Bart Cerneels wrote:
>     Single line comments for those blocks (ex. //Initialization) are even better then. newlines are just newlines for anyone not aware of your intended structuring.
>     
>     Some structure in cpp files is actually a good thing. I usually keep it limited to having the methods in the same order as in the header file.

Okay, not a big deal.


> On Aug. 9, 2012, 10:45 a.m., Matěj Laitl wrote:
> > src/statusbar/CompoundProgressBar.h, line 32
> > <http://git.reviewboard.kde.org/r/105942/diff/1/?file=76719#file76719line32>
> >
> >     Please document the fact that this class is now completely thread-safe. (or, if not, document that it is reentrant and some methods are thread-safe, then document every thread-safe method)
> 
> Ralf Engels wrote:
>     I have no idea if it's now completely thread-save. I just fixed the most obvious problems.

Hmm, could you have a look at it, please? Documentation of thread-safety is something that is desperately needed in current Amarok code-base IMO. The lack of it opens gates for subtle bugs such as this one.

It looks like you've added mutexes to all methods that work with class data, thus making the whole class thread-safe.


- Matěj


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


On Aug. 9, 2012, 10:36 a.m., Ralf Engels wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105942/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2012, 10:36 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> childBarFinished was called with invalid m_progressDetailsWidget
> However also the m_progressMap needs protection. The CompoundProgressBar needs to be thread safe.
> 
> 
> This addresses bug 292553.
>     https://bugs.kde.org/show_bug.cgi?id=292553
> 
> 
> Diffs
> -----
> 
>   src/statusbar/CompoundProgressBar.h baaab94 
>   src/statusbar/CompoundProgressBar.cpp 371a534 
> 
> Diff: http://git.reviewboard.kde.org/r/105942/diff/
> 
> 
> Testing
> -------
> 
> Change is verified by me to fix the problem.
> 
> 
> Thanks,
> 
> Ralf Engels
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120809/e2154fd7/attachment.html>