[Patch] "No grouping" checkbox in playlist layout

[Tim Bocek]

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).

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.

Thanks again for your help!

-Tim

On Sat, Aug 29, 2009 at 12:36 PM, Nikolaj Hald Nielsen <
nhnfreespirit at gmail.com> wrote:

> 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
> _______________________________________________
> Amarok mailing list
> Amarok at kde.org
> https://mail.kde.org/mailman/listinfo/amarok
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok/attachments/20090829/fd7be6d0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Added-an-option-to-disable-grouping-in-the-playlist.patch
Type: text/x-diff
Size: 11428 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/amarok/attachments/20090829/fd7be6d0/attachment.bin>