search

gnome-prototypes-team / gnome-music

Gnome Music
https://live.gnome.org/Music
Other
5 stars 13 forks source link

create a playlist iter before appending to playlist #185

Closed sumansai14 closed 11 years ago

kyoushuu commented 11 years ago

Sorry but I'll reject this for merging.

I believe that the code that this commit changes is the correct one and the patch to make it work is already accepted for commit in http://bugzilla.gnome.org/show_bug.cgi?id=707642 because it is actually an annotations issue. I'm pretty sure that it is correct because of the following:

  1. TotemPlParser works the same way as GtkListStore/GtkTreeModel:
commit 01ded1197b0553ecb0685ef180e35c053a504b8e
Author: Carlos Garnacho <carlos@lanedo.com>
Date:   Fri Dec 18 14:25:57 2009 +0100

    Remove GTK+ dependency.

    TotemPlPlaylist has been added to replace GtkTreeModel usage in the
    write API, internal playlist implementations now use this to get
    songs/titles in order to write the playlist. Older write API has been
    replaced by totem_pl_parser_save().

The annotations there use in and out for these types of functions. The current code matches the way a new row is appended to a GtkListStore.

  1. The fix for this issue is already approved by Bastien Nocera (a totem-pl-parser developer). The patch is ready for commit, this code will break things once TotemPlParser is released for 3.10 cycle.
  2. I did a similar thing before in http://bugzilla.gnome.org/show_bug.cgi?id=703861 with all my patches rejected. This is simply wrong, scripted languages don't have pointers so trying to pass variables to functions won't get them modified. If it works, this behavior is undefined and might cause issues (depending on programming languages and how gobject-introspection works there; this might work in Python, but definitely not in JavaScript). It might work in Python but... (see 2.).

Anyway, thanks for trying to fix this issue. I'm looking forward for more pull request in the playlists branch in the future :D

kyoushuu commented 11 years ago

I'm changing my decision on this one, see the totem-pl-parser bug for more info. Please rebase, and fix other instances of this too. I'll merge this one after a few hours. (Or maybe not, because I hate merges, which make rebasing difficult, I'll just create a mailbox patch and apply it. This way, only the hash will change, and there's no "Merged ...." commit.)

kyoushuu commented 11 years ago

Merged.