[Patch] "No grouping" checkbox in playlist layout

[Nikolaj Hald Nielsen]

So, I'm back, a bit earlier than expected. Here are some comments on your patch.

> The patch adds a checkbox to the playlist layout that allows disabling of
> the grouping feature.  Unless I'm missing something, previously users who
> wanted no grouping needed to create an empty header, and duplicate the body
> and single layouts.  Now, they just check this box.

Something similar to this has been requested a few times already, so I
think this makes sense.

> 1. This is enforced in the UI code currently.  Is this OK, or would it be
> better to enforce somehow in PlaylistLayout.

I am not sure exactly what you mean by this.

> 2. Right now, if grouping is disabled and re-enabled, the playlist will have
> the empty header and duplicated body/single layouts.  This might not be
> desireable, e.g. if a user creates a layout with grouping, turns it off, and
> turns it on again.  In this case, would it be more desireable to restore the
> original layouts, or to have the head/body layouts reflect the settings to
> turn off grouping?

I think this is very destructive and that it can easily be avoided,
see my comments bellow.

> 3. If the previous questions or any other issues require fixes, can we at
> least get the checkbox and associated string in there so this can ship with
> 2.2 (that is, if the patch is even wanted)?

Not sure what you mean by this as having the checkbox alone does not
do us much good, and there should be no issue adding it to 2.2.1 if we
cannot get it ready by 2.2.0.

> 4. Since I'm pretty new to FOSS development, anything I should have done
> differently? (in other words, "hey guys im a n00b am i doin this rite?")

The patch applied fine and works as advertised, so you are already
doing quite well in that regard! :-)

So, the main issue I have with this patch right now, beyond your point
2 above, is that, even though the layout will work as expected when
Amarok is restarted, the value of the "allow grouping" checkbox is not
persistent. So what I propose is this:

You already made the "allowGrouping()" function to the layout classes.
How about modifying the rendering code in the playlist delegate so it
just paints the "single" layout for both single and body items and
ignores the head, even if it is there, when allowGrouping() == false.
This way you don't really have to delete the head item and copy the
single item to the body item in the layout, and thus, the layout will
be as it was if you later recheck the "allow grouping" button. In this
case you also need to save and load the "allowGrouping" value to/from
xml, but this should be easilly done (make sure it defaults to "true"
when loading, that way existing layouts will work as expected)

Oh, as another small nitpick, I think you should inverse the checkbox
to "disallow grouping" as grouping is the most used case (and the
default).

Did this make sense?

- Nikolaj