Review Request: Do not return collectionFolders() unless ready.

[Edward Hades Toroshchin]

> On June 14, 2012, 11:06 a.m., Matěj Laitl wrote:
> > src/MountPointManager.cpp, line 331
> > <http://git.reviewboard.kde.org/r/105246/diff/1/?file=67594#file67594line331>
> >
> >     Hmmm, IMO this can be a source of subtle bugs where calling this important method is dependent on when it is called. I fear that callers of ::collectionFolders() cache the result so that could have long-ranging consequences.
> >     
> >     I'd rather see a fix that touches the _caller_ to ensure that MountPointManager is ready.

As far as I've seen, no one caches the result. There is even a TODO comment that suggests that such caching should be implemented in the collectionFolders() itself.

Also, the desired behavior of caller when MountPointManager is not ready is to do nothing, which is accomplished by returning empty list.


- Edward Hades


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


On June 13, 2012, 8:44 p.m., Edward Hades Toroshchin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105246/
> -----------------------------------------------------------
> 
> (Updated June 13, 2012, 8:44 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> If a collectionFolders() method of MountPointManager is called before it
> has completed identifying mount points, it adds the default path like
> $HOME/Music.
> 
> 
> This addresses bug 286219.
>     https://bugs.kde.org/show_bug.cgi?id=286219
> 
> 
> Diffs
> -----
> 
>   src/MountPointManager.h 910f777 
>   src/MountPointManager.cpp d3238c0 
> 
> Diff: http://git.reviewboard.kde.org/r/105246/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Edward Hades Toroshchin
> 
>

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