[amarok] /: CollectionLocation: make associated collection pointer non-const

[Kevin Funk]

On Saturday 21 January 2012, 01:39, Matěj Laitl wrote:
> Git commit 99a7d0b5bb541ed6400213d81cc7683935666514 by Matěj Laitl.
> Committed on 20/01/2012 at 15:21.
> Pushed by laitl into branch 'master'.
> 
> CollectionLocation: make associated collection pointer non-const
> 
> CollectionLocation is used to copy/move/delete tracks from collection,
> it is irrational that it would point to _constant_ parentCollection.
> 
> Also make Collection::location() non const, same rationale. This allows
> to get rid of a fair bunch (well, 7) of const_casts.
> 
> _All_ places where there methods/arguments/attributes are updated to
> reflect the change (even ones that would compile without changing).
> 
> Furthermore {DatabaseCollection,SqlCollection}::is{Writable,Organizable}
> can use the default implementation, which is tweaked to be null pointer
> safe.
> 
> Many classes that subclass CollectionLocation called its default
> constructor, this is now fixed to call its
> CollectionLocation( Collection *parentCollection ) constructor so that
> default implementation of CollectionLocation::collection() works.
> 
> (UmsCollectionLocation even introduced its own ::umsCollection() method
> probably because generic collection() did not work (surprise), it is
> removed in favor of default ::collection() implementation)
> 
> (snip)

Hey there,

Disclaimer: I don't really know the code, so forgive me when I'm wrong.

I'm curious. Why does Collection::location() need to be non-const? From what I 
can see in your diff you can make it const, after all. (=> no const_cast 
needed at all)

Secondly, it should be named createLocation() since it is in fact allocating 
memory on the heap on every call, isn't it? That applies both to 
Collection::queryMaker and Collection::location, actually. They should be 
named createQueryMaker and createLocation respectively and made const to 
reflect their behavior (I refer to the factory pattern here).

(SqlCollection, for example, internally calls "QueryFactor::createQueryMaker() 
const" which is what I would expect).

Just my two cents. :)

Greets

-- 
Kevin Funk