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

[Matěj Laitl]

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

Ship it!


Go for it then.

- Matěj Laitl


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
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

[Matěj Laitl]

> On June 14, 2012, 11:06 a.m., Matěj Laitl wrote:
> > src/MountPointManager.cpp, line 331
> > 
> >
> > 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.
> 
> Edward Hades Toroshchin wrote:
> 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.

> 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.

Oh, I see, ScanManager seems to be the only real user:
 * SqlCollectionLocation uses it in actualLocation(), but that is only called 
internally when copying tracks to collection. (I'd like to see actualLocation() 
removed from Collections::CollectionLocation)
 * SqlCollection uses it is collectionFolders() which is never called -> I'd 
like to see it removed


- Matěj


---
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
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: 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
> > 
> >
> > 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
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

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

[Matěj Laitl]

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



src/MountPointManager.cpp


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.


- Matěj Laitl


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
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel