Fwd: extragear/multimedia/amarok/src/servicebrowser/lastfm

[Nikolaj Hald Nielsen]

second one:


---------- Forwarded message ----------
From: Maximilian Kossick <maximilian.kossick at googlemail.com>
Date: Mon, Mar 3, 2008 at 2:40 PM
Subject: Re: extragear/multimedia/amarok/src/servicebrowser/lastfm
To: Nikolaj Hald Nielsen <nhnfreespirit at gmail.com>


OK, I just noticed that you did not send thi mail to amarok-devel. fine with me.


 On Mon, Mar 3, 2008 at 12:24 PM, Nikolaj Hald Nielsen
 <nhnfreespirit at gmail.com> wrote:
 > On Mon, Mar 3, 2008 at 9:52 AM, Maximilian Kossick
 >  <maximilian.kossick at googlemail.com> wrote:
 >  > The idea of the collection framework and in particular the Meta
 >  >  framework [1] is to hide the implementation details of the music from
 >  >  other core components like the playlist or the engine. I just had a
 >  >  look at the shoutcast service again, and it looks like the "expand
 >  >  track" capability that we talked about a while ago never actually was
 >  >  implemented as far as i can tell. So forget what I was saying about
 >  >  the shoutcast service.
 >
 >  Yes, it actually is, to the extend that each tracks url is checked for
 >  whether it is actually a playlist type. Really ugly hack imo. but at
 >  least it works. For now....

 As long as the "expand track" stuff is done in the component where the
 track originates from, i have no problem with that.
 I just think it's a bad idea to let the playlist do it. I tried to
 find the "expand track" code in the playlist, but I did not find it.
 So I'm not really sure if this is actually an issue, as mentioned in
 my last e-mail. As far as I know, shoutcast playlists are actually
 alternative urls for the same track (at least they seem to be every
 time i add a shoutcast stream in A1). Therefore we should only insert
 one track per stream in the playlist, which creates a problem because
 the tracks are actually ServiceTracks (or whatever the class is called
 exactly) and they don't support alternative urls (no class actually
 does yet, but, again, as mentioned before on IRC, StreamTrack should
 be extended to do). But that *is* an implementation detail and other
 components of A2 should not be aware of this (the exception is the
 Engine, because at some point we have to access the alternative urls
 if the primary one doe not work)


 >
 >  > That capability would create a problem though.
 >  >  What we would essentially be doing, if we'd actually use that or a
 >  >  similar capability, is to make other parts of Amarok aware of
 >  >  implementation details of that collection/Meta implementation, even if
 >  >  we slap another abstraction layer on top of those implementation
 >  >  details. Doing that would require special code to handle those
 >  >  details, which results in additional if/else statements (and I really
 >  >  dislike if/eslse statements if they can be replaced by a proper
 >  >  architecture). This can be avoided by keeping the implementation
 >  >  details internal to the component where the tracks are originating
 >  >  from.
 >
 >  As I see it, right now a huge bunch of if/else statements is exactly
 >  what you are getting. Adding playlists to the playlist mode currently
 >  requires a set of completely separate functions that basically just
 >  replicate the stuff that is in the 'normal' insert methods.

 I'm not quite sure why you are mentioning playlists here. I was
 talking about Capabilities, in particular the "expand track"
 capability. As mentioned above, I don't know right now what exactly
 happens when we add a shoutcast stream to the playlist, so is part is
 going to become very theoretical. As I wrote in my last e-mail,
 expanding a track to multiple tracks in the playlist brings an
 implementation detail of one component into another component. We have
 to check for each track that we want to add to the playlist and check
 if it's expandable. That's an unnecessary if/else statement, and it's
 not immediately apparent to a new developer why it's necessary. On the
 other hand, if we expand the track in the component where it
 originates from, it's ok, because a) we can easily add a comment, e.g.
 in the shoutcast service something like "shoutcast tracks are actually
 playlists, so expand it before adding it to the playlist" and b) we
 hopefully don't need an if/else statement because we have to expand
 all tracks in that component anyway. I hope you can see that I'd like
 everything nicely loosely couped and all implementation details
 contained in the actual implementation. Again, I have no idea where
 the expanding is doen in the shoutcast browser, so I should not have
 choosen it as an example in my original mail. I only thought we were
 expanding the tracks in the playlist itself.


 >
 >  >  If we ignore the question whether a tree view is really the right user
 >  >  interface for last.fm, making last.fm a collection and reusing the
 >  >  collection view create a few problems. I don't know what you layout
 >  >  you have in mind for that tree, but at the very least we'd have
 >  >  something similar to global tags in A1, so we'd need a Global Tags
 >  >  node which would have a child node for each global tag. The problem is
 >  >  that the Global Tag node is not a real node[2]. For example, adding
 >  >  all child nodes of the Global Tags node to the playlist does not
 >  >  really make sense from a UI POV, although it's technically possible.
 >
 >  Why shouldn't be allowed to add a group of streams to the playlist?
 >  For now, the last.fm "skip" and playlist "next" actions are separate
 >  anyway, so the user can both skip within a stream and forward to the
 >  next one. I agree that it is not something I would use, but I don't
 >  see why it conceptually does not make sense. To be honest, I never
 >  planned to take over the last.fm service, I jsut saw what Hydrogen had
 >  done and figure that it could be added to the treeview really easily (
 >  which I still think it can, even though we once again bang our head
 >  against the "this is a stream pretending to be a track" issue. If this
 >  is the correct user interface for the last.fm service I have no idea,
 >  and luckily his is not for me alone to decide! :-)

 I'll try to explain why I think you are mixing a few separate issues
 here. A "stream pretending to be a track" issue does not exist. Look
 at meta/stream, that's a class for handling
 streams that i wrote myself. A better example is the magnatune store.
 Each preview is a stream, but you won't hear me complaining about any
 of that. why? Because the collection model is a natural fit for
 magnatune. I personally don't think that a tree view is a good UI for
 the last.fm service, but I do not have a better proposal either. But
 as long as we all now that we have to find a good UI first, before
 going ahead and implementing it as a tree view, which is what I
 thought you were about to do, I think we can all agree.


 >  >  Removing that item from the context view would mean modifications to
 >  >  the collection view and/or the model, and we all know that these two
 >  >  classes unfortunately contain lots of black magic. Then there's the
 >  >  problem how we'd insert the Global Tags node into the collection view
 >  >  anyway? Using another Meta class to fake it, e.g. Meta::Genre, results
 >  >  in each track having the genre "Global Tags".
 >
 >  Yes, and why is that bad? It is as good as any other genre description
 >  we could come up with. In the shoutcast service, this means that a
 >  stream has the genre of the category it was in ( for instance,
 >  "Hardcore Rock", or "Albanian" ) which again imo is a good as any
 >  other genre we can give it. ( Technically this does not currently work
 >  because of having to generate a new playlist out of the track, thus
 >  loosing the genre data... )

 Again, this is not an issue. I was talking specifically about last.fm.
 As with the magnatune store,
 the collection model is a pretty good fit for shoutcast.

 I've come to realize that it does not matter for the rest of A2 as
 along as it is compartmentalized as outlined above. You might or might
 not run into trouble doing it that way (and I've too much else on my
 mind to think it through even more), but now that I know that you are
 not sure about the UI it, it does not really matter anymore.


 >
 >  >  And I am completely
 >  >  against extending the CollectionModel with support for further "fake"
 >  >  nodes. Adding Various Artists was bad enough, and we don't have to try
 >  >  and break it again.
 >
 >  Yes, you have gotten that point across loud and clear by now. If I
 >  were to put things on edge just a little bit, I would say that I get
 >  the feeling that you are strongly opposed to adding anything to the
 >  collection framework that was not part of your original grand scheme.
 >  The downside of this is that it leaves the rest of us who are doing
 >  things _slightly_ outside the bounds of the uses you had originally
 >  thought out to "go play in a corner somewhere" and basically
 >  reimplementing features from the collection framework just to get even
 >  fairly basic stuff like playlists working in a sane way.
 >
 >  Additionally, I don't think we need more fake nodes. Make streams and
 >  playlists proper members and most of these issues will go away. I see
 >  no reason that a stream or playlist should not be allowed to have a
 >  genre or other meta data the same as every other playable item.

 I'm not sure how streams are not proper members of the Meta framework
 yet. A Meta:.Track instance can easily represent a stream
 or a couple of alternative stream urls. I think I outlined my
 reasoning above, but feel free to ask for a clarification. As for
 playlists, I haven't actually seen any proper design yet. It would be
 nice if Bart would create a class diagram for his ideas, which would
 make it a lot easier to talk about it. We have to integrate playlists
 into the Meta framework somehow. I'm just not sure simply copying what
 I did for the other classes is the best idea.

 And no, I'm not considering the shoutcast playlists to be proper
 playlists. Some users agree with me, because I do remember seeing a
 bug report which complained that we were inserting one item for each
 alternative url in A1, and i agree with that bug report.


 >
 >  >  I am sure that we won't end up with 20 similar models/views if we
 >  >  approach this problem properly btw:)
 >
 >  No, but still having ( basically ) the same code twice already gives
 >  me a bad taste in my mouth.
 >
 >  I am really not trying to be inflammatory here, but I do feel very
 >  strongly about this, and I do feel that we are creating an A and a B
 >  team of content instead of being able to integrate whatever we come up
 >  with seamlessly, which really for me is the main feature of A2. This
 >  is besides the fact that it will make the service framework mostly
 >  useless for many types of content as you are basically going to have
 >  to write everything from scratch anyway.
 >
 >  - Nikolaj
 >
 I hope I was able to explain my technical/architectural objections. I
 am not trying to make the service framework useless. it works well for
 quite a few services, but I also think that it does not work for
 others, and would need some additional work for them. I took note that
 you think i am protective of my "grand scheme", and I'll consider it.

 -Max