[vlc-devel] [PATCH] sd: allow passing longname in separate argument

[Pierre Ynard]

> The playlist code really should not have to pass the long name. It
> shokld be inferred from the instantiated SD.

I agree. I'd like if it could be probed through pf_control after loading
the module, but it seems only the lua SD supports it so that would be a
lot of code to implement for me.

There's still a lot more that can be done to fill the long name instead
of "?": using the SD chain, using the SD stem, using the "sd" parameter
in the config chain in particular for lua scripts. It's all better than
"?" and doesn't matter so much since it's for display.

> Perhaps the SD plahlist node creation could be deferred to the
> first invocation of the item_added callback? I don't really see any
> other way, except for letting interfaces take care (privately and
> independantly) of SD, which would break -S however.

I suppose that would be another path to pass it up from the SD, yes;
although it may never happen if the SD finds nothing. But that makes me
think, why not simply let the SD create its playlist node in Open(), or
set the name of that node, or really just fill the long name in the SD
structure to pass it back? Any issue with that? Is it fine to create the
node, load the SD, then change the name of the existing node?

> This ugly hack is a compromise to avoid exposing the deficiency to the
> playlist code.

I'm not sure what this is trying to preserve here. The playlist API
call has the right prototype, granted, but is still tainted by the
expectation of a config chain. And the counterpart is that it pollutes
the SD probing API and a lot of things. The playlist code is certainly
exposed to that deficiency since it has to parse longname from the
config chain. It even makes sense to me that the playlist API call
playlist_ServicesDiscoveryAdd() would take as an argument the display
name of the playlist item to create.

> I think this is replacing a backward approach with another backward
> approach though. In my opinion, the long name should be passed from
> the instantiated module.

It still seems like a step in the right direction, but I hope I've
offered an even better idea.

> Also the Is*Loaded test is busted and IMHO unfixable. Interfaces need
> to stop using it.

The proposed change would go some way towards fixing it by removing the
worst of the SD name string. I'm also skeptical why a playlist call
would take a string to match and remove an SD from the playlist, rather
than an item ID. And I'm skeptical about the concept of uniqueness for
SD modules, several instances of the same SD could run with different
configs, that's certainly what the lua SD does to a different extent.
Since SD are managed by the playlist, maybe I'd rather they were
accessed through their playlist node.

I'm mildly disturbed by how deleting an SD playlist node doesn't stop
the SD. I wonder if that can cause issues.

By the way, unloading an SD correctly removes its playlist node in 2.2,
but it doesn't in 3.0, so it seems something got broken somewhere.

-- 
Pierre Ynard
"Une âme dans un corps, c'est comme un dessin sur une feuille de papier."