GSoC application review - reimplementing personal metadata importers

[Matěj Laitl]

On 1. 5. 2013 Konrad Zemek wrote:
> Matěj Laitl wrote:
> > Kondad wrote:
> > > To ensure that metadata from current version of Amarok can always be
> > > synchronized, Collections::SqlCollection class will be used
> > 
> > I don't think this is a good idea at all. It is like using a cannon to
> > smash a fly, and it would add great deal of complexity and open a can of
> > worms.
>
> I find this a bit puzzling. Ideally it would be made that importing from 
> Amarok 2.x Just Works (tm), without introducing additional logic and 
> instead reusing what's already implemented. Some changes to 
> SqlCollection might be in order (e.g. we wouldn't want a migration 
> dialog for imported database), and it may even be broken up in two 
> layers - the basics (the layer used by importers) and the rest (the 
> layer used for "normal collection). Of course, SqlCollection has some 
> more complicated logic inside, like using a buffer, but from the 
> perspective of Provider it would just be a convenient interface for 
> MySqlStorage / MySqlEmbeddedStorage. So from my viewpoint it's both 
> conceptually clearer and more maintainable.
> 
> That being said, I have much (much) less experience with Amarok than you 
> do, so I probably am just not seeing something here. If you would kindly 
> supply me with any major-ish point speaking against using SqlCollection, 
> I would gladly go along with just using MySqlStorage / MySqlEmbeddedStorage.

My bet that support code for actually using SqlCollection would be much 
heavier than implementing the A 2.x importer without it.

Technical reasons not to use SqlCollection:
 1. you'll be instantiating the beast just to use 2% of its functionality. 
SqlCollection is huge, including SqlRegistry, gui classes that are 
unconditionally instantiated, playlist support, you don't need albums that 
read their images from disc, etc. etc.
 2. SqlCollection has much more assumptions on the environment than you'd 
need. For example it needs MountPointManager instantiated connected to the 
right database etc.
 3. By using SqlCollection, you actually make it harder for you to support 
syncing against older/future Amarok versions because various (unrelated) 
methods assume the latest database schema.
 4. In future, some users may not enable/build SqlCollection in favour of 
let's say NepomukCollection. But they still should be able to sync against it.

While you'll be able to make changes to SqlCollection to get rid of some of 
the limitations, please be prepared that I'll be against any changes to 
SqlCollection that make it more complicated (with regards to its primary 
purpose; cleanups are still welcome).

	Matěj