Thanks for the suggestions.  I've attached a revised patch that I hope addresses your comments.  It's a bit more extensive, but it is more robust too!  The patch I've attached is not incremental, so revert the last patch I set first (I have an incremental one to send if you want).<br>

<br>One issue I can't track down is it doesn't play nice with the inline editor.  I'll go after this in a bit, but I wanted to get the new patch out for review.<br><br>Thanks again for your help!  <br><br>-Tim<br>

<br><div class="gmail_quote">On Sat, Aug 29, 2009 at 12:36 PM, Nikolaj Hald Nielsen <span dir="ltr"><<a href="mailto:nhnfreespirit@gmail.com">nhnfreespirit@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

So, I'm back, a bit earlier than expected. Here are some comments on your patch.<br>
<div class="im"><br>
> The patch adds a checkbox to the playlist layout that allows disabling of<br>
> the grouping feature.  Unless I'm missing something, previously users who<br>
> wanted no grouping needed to create an empty header, and duplicate the body<br>
> and single layouts.  Now, they just check this box.<br>
<br>
</div>Something similar to this has been requested a few times already, so I<br>
think this makes sense.<br>
<div class="im"><br>
> 1. This is enforced in the UI code currently.  Is this OK, or would it be<br>
> better to enforce somehow in PlaylistLayout.<br>
<br>
</div>I am not sure exactly what you mean by this.<br>
<div class="im"><br>
> 2. Right now, if grouping is disabled and re-enabled, the playlist will have<br>
> the empty header and duplicated body/single layouts.  This might not be<br>
> desireable, e.g. if a user creates a layout with grouping, turns it off, and<br>
> turns it on again.  In this case, would it be more desireable to restore the<br>
> original layouts, or to have the head/body layouts reflect the settings to<br>
> turn off grouping?<br>
<br>
</div>I think this is very destructive and that it can easily be avoided,<br>
see my comments bellow.<br>
<div class="im"><br>
> 3. If the previous questions or any other issues require fixes, can we at<br>
> least get the checkbox and associated string in there so this can ship with<br>
> 2.2 (that is, if the patch is even wanted)?<br>
<br>
</div>Not sure what you mean by this as having the checkbox alone does not<br>
do us much good, and there should be no issue adding it to 2.2.1 if we<br>
cannot get it ready by 2.2.0.<br>
<div class="im"><br>
> 4. Since I'm pretty new to FOSS development, anything I should have done<br>
> differently? (in other words, "hey guys im a n00b am i doin this rite?")<br>
<br>
</div>The patch applied fine and works as advertised, so you are already<br>
doing quite well in that regard! :-)<br>
<br>
So, the main issue I have with this patch right now, beyond your point<br>
2 above, is that, even though the layout will work as expected when<br>
Amarok is restarted, the value of the "allow grouping" checkbox is not<br>
persistent. So what I propose is this:<br>
<br>
You already made the "allowGrouping()" function to the layout classes.<br>
How about modifying the rendering code in the playlist delegate so it<br>
just paints the "single" layout for both single and body items and<br>
ignores the head, even if it is there, when allowGrouping() == false.<br>
This way you don't really have to delete the head item and copy the<br>
single item to the body item in the layout, and thus, the layout will<br>
be as it was if you later recheck the "allow grouping" button. In this<br>
case you also need to save and load the "allowGrouping" value to/from<br>
xml, but this should be easilly done (make sure it defaults to "true"<br>
when loading, that way existing layouts will work as expected)<br>
<br>
Oh, as another small nitpick, I think you should inverse the checkbox<br>
to "disallow grouping" as grouping is the most used case (and the<br>
default).<br>
<br>
Did this make sense?<br>
<font color="#888888"><br>
- Nikolaj<br>
</font><div><div></div><div class="h5">_______________________________________________<br>
Amarok mailing list<br>
<a href="mailto:Amarok@kde.org">Amarok@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/amarok" target="_blank">https://mail.kde.org/mailman/listinfo/amarok</a><br>
</div></div></blockquote></div><br>