A whole load of random patches

[N.C. Wilson]

On Thursday 03 January 2008 1:33 am Leo Franchi wrote:
> Just an FYI, it is not good to save the return value of Amarok::config
> (IIRC, due to threading isues).
> So the first "fix" doesn't really work :)
> Otherwise, this patch looks good.

Oops.  2 lines of code to change here then. Easily sorted.
>
> i'll be honest, i can't really see the difference with the greying
> out. i tried checking and unchecking stuff, but it seemed pretty
> logical before and after.  but the patch doesn't mess anything up...2
>
>
> the specified page fix looks good too.
>
The problem was storing the wrong page object; tring to dynamic cast to the 
right one would then always give 0, so it thought the page does not exist.
>
> > 5. Added some little features for altering the sort order. Old order
> > would have been eg.:
> > - Symphony 1
> > - Symphony 10
> > - Symphony 2
> > Now the three are shown correctly in order.
>
>  you definitely need to comment your code here.
> CollectionSortFilterProxyModel::lessThanString needs some doc lovin'
> to bo honest a relatively quick readthrough shows me that it would take me
> quite a significant amount of time to parse the code, so i'm going to wait
> for some comments.

Point taken.  V. sensible.
>
> 6. I also made a customisable thing for ignoring prefixes on entries. (I
>
> > find this very neat - give it a whirl and you'll see what I mean! 
> > Putting QRegExp support into everything is dead easy too if it would be 
> > useful.) This is primarily useful for French or German names, where you 
> > want "von Richthofen" to appear under R, but still display as "von 
> > Richthofen", not "Richthofen, von" or something stupid like that.
>
> i don't like how there is now a new config pane. IMHO we need to cut 
> down on options wherever possible, and to be honest i think 99% of users 
> does not care about custom prefix settings. we can easily specify a 
> default that will work for virtually everyone and save it in a config 
> file, users that really need to remove <insert 'the' in random language 
> here> as a prefix can manually edit that.
>
> what i think may be a better place for the "select how to sort by" 
> options is this: maybe under Group By in the collectionbrowser the last 
> entry could be "custom", which opens a small dialog (with just the 
> customizable settings). this would remove the need for the config pane 
> completely (the one checkbox could go in the Collection pane).
>
> anyway, thats my opinion :)

This is sort of what I was thinking when I said it was a placeholder (in 
another e-mail I think). I just put it there to test stuff quickly because 
the KConfigPage does everything for you.
>
> 7. Probably a few other little one-liners.
>
> > I noticed that in the code there was a commented out line saying that 
> > showing "Artist / Year - Album" was far harder than "Artist / Year / 
> > Album". The only way I could think of fixing this would be to get rid 
> > of the idea of Meta::Album and Meta::AlbumPtr items, and instead have 
> > some sort of a Meta::Field and Meta::FieldPtr class which can hold a 
> > custom field, constucted with a regex from the tag fields in Collection 
> > Manager. This seems quite a large change, so I assume it is not 
> > something people want done? Being able to completely control the 
> > presentation would be nice, but probably not worth it (laying out your 
> > collection by customised fields
> >
> > with regexes would be very cool though, especially for people like me
> > with hundreds of GBs of music).
>
> you're going to have to talk to maxx_k about this, the Meta:: stuff is 
> his. but i'm going to side with maxx_k simply because the architecture is 
> new from the ground-up and (i believe) designed to be simple and 
> extensible... and last us a long time. a Meta::Field constructed from a 
> regex from the tags in collection manager is far from simple.
>
> either way, maxx_k is the authority on this stuff, unfortunately he is out
> of irc/coding for a while, but should check amarok-devel@ (i'm cc'ing the
> devel list).
>
I guessed this would be tough, hence the comment I saw in the code. I was 
not really suggesting you do that, just asking whether it really was the 
easiest way.

Thanks for looking at it.

Nicholas