Don't make DEFAULT_DEVICE a constant

[JonnyJD]

I would consider not to have a DEFAULT_DEVICE constant but rather do an actual function call. Even though the default device is at the moment mostly a constant in libdiscid, it might be something which changes on runtime.

I am not entirely sure how OSX handles the devices, but isn't it the case there that the devices change on runtime when the users inserts / removes a disc? At least https://github.com/metabrainz/libdiscid/blob/master/src/disc_darwin.c#L155 suggest this and Picard has issues with it. Even on other systems that could happen when the user attaches a portable drive, even though dynamic detection of the device is currently not supported by libdiscid on those platforms.

[JonnyJD]

I would rather see that as an implementation problem in libdiscid or even a general problem for Mac OS X.
The default device should not change.

The solution for mac is to use device numbers, rather than raw devices.
DEFAULT_DEVICE = 1 would be the default for Mac, pointing to the first disc drive, which has an actual raw device name only when a disc is inserted.
See: http://tickets.musicbrainz.org/browse/LIB-28

[JonnyJD]

For Windows the default device can technically change, but does so only when explicitely reassigned in the partitioning managment.
I would still call that a constant.

[phw]

Well, my portable CD drive is not a constant, especially as I frequently run out of free USB ports ;) Not even it's drive letter on Windows is constant as it all depends on whether I plug in some other drive before or after connecting the CD drive.

The point is that it is not a libdiscid issue. libdiscid is just fine, it's interface can handle a runtime change in the default device, even though the current libdiscid implementation can't.

I would advise not to cripple the python-discid interface by making the default device a constant. If you use a method to access the default device you keep the interface flexible without loosing anything.

[JonnyJD]

Hm, didn't think about portable CD drives. That is kind of a problem. So for portable drives the situation is similar to Mac OS X, but in contrast to Mac OS X, device numbers are used nowhere in Windows environments. (and on Windows the drive letter doesn't depend on an actual disc beeing in the drive.

Having a static device name always was the intended way for libdiscid (MB_DEFAULT_DEVICE internally on platforms except Mac).
Though that doesn't necessarily mean MB_DEFAULT_DEVICE must be used when read() gets NULL as a device parameter.

I wouldn't call something "default device" if it changes often.
Thats all from the linux perspective though, where we have /dev/cdrom as a default_divice provided by the system already. The underlying device /dev/cdrom links to can be different (mostly /dev/sr0 on current systems).

[JonnyJD]

I understand that you want to read automatically from the first disc (drive) currently detected, but I am not sure if we should make that available as something called "default device".
This is mostly about metabrainz/libdiscid#20.

[JonnyJD]

One possibility to make this available would be to allow "drive numbering" in general (not only on Mac, where it is proposed only). This would enable "1" as a default device, though I would still use "/dev/cdrom" as a default on Linux.

Anyways. I don't like the idea of an ever-changing "default device" (as returned string, the internal device mapping can change as often as it wants to).

[phw]

Yes, indeed, metabrainz/libdiscid#20 is the libdiscid side of this (for Windows).

When you look at discid_get_default_device() from the outside it does not look like it is meant to return a hard coded constant all the time. My assumption as a user of the library would rather be that it returns a meaningful device for my system. That it uses a constant internally for most implementations is IMHO an incomplete implementation and I can't really see the intention that the implementation should work that way.

As it is the default device works on none of my systems. On my Windows laptop it returns "D:" although my CD drive is E:, and on my Linux it returns "/dev/cdrom" even though I only have "/dev/cdrom1" (a symlink to /dev/sr0). To make it worse if I boot Windows on the same laptop the actual drive letter is either E:, F: or none at all depending on whether an external HDD is plugged in and whether the CD drive is plugged in or not. Picard can handle at least the first two cases, but I see no reason why this detection shouldn't get implemented directly in libdiscid.

But no matter how you turn it, discid_get_default_device() doesn't look like it returns a constant, doesn't actually return a constant on OSX and might not return a constant on other platforms in the future. So I wouldn't make it a constant in your implementation. Feel free to ignore my advice, but in general when designing a public API and something is not absolutely without doubt a constant you are better off not making it a constant.

Just for the record: I think using device numbering is a bad solution on any platform where it is not common. But that's only indirectly related to this ticket so let's discuss that somewhere else :) (EDIT: I've commented on http://tickets.musicbrainz.org/browse/LIB-28)

[JonnyJD]

discid_get_default_drive():

Return the name of the default disc drive for this operating system.

Returns
a string containing an operating system dependent device identifier

To me that sounds much like there is exactly one outcome on a specific operating system that should not be dependent on the actual system.
The reason why this isn't just a C constant: A "C constant" is normally a define (or uses one), which would have to be in the header file, but that would mean it is either the same for all provided platforms or we would need an include for every platform (or would change the include file with the build scripts).

That OS X is messed up in that context is a problem, but that is only an OS X problem and only until we implement device numbers on OS X. There are no actual names for the device, only for a disc loaded in a device (and with that name you can access the device again, but only then).

[phw]

The reason why this isn't just a C constant: A "C constant" is normally a define (or uses one), which would have to be in the header file, but that would mean it is either the same for all provided platforms or we would need an include for every platform (or would change the include file with the build scripts).

That's an explanation why it is not technically a constant. But it does not explain why it should be a constant. I at least hope that discid_get_default_device() exists not only as a workaround for the fact that a define would not work but also because it is just sane API design. You can't assume a hard coded constant will work on every system and indeed the current implementation makes discid_get_default_device() on some systems rather useless for everything but a libdiscid usage example.

I think before insisting on discid_get_default_device() being a constant we should be clear about why it should be a constant (sorry, that's absolutely not clear to me) and what the intended use case for discid_get_default_device() is.

[JonnyJD]

What you cited is not the explanation, yes. But what I quoted is the definition of the value that makes it constant per operating system.

This doesn't mean we can't change it, but this change might be unexpected.
Though I am not sure other people take description of the function that serious.

I also want to note, that discid_read() does mention the default device should be exactly what is used by read (when NULL is given):

This function reads the disc in the drive specified by the given device identifier. If the device is NULL, the default drive, as returned by discid_get_default_device() is used.

So implementing something different for the use in read() would be even worse than making default_device depend on the current state of the current machine.

I'll think about it.

[phw]

As you said in http://tickets.musicbrainz.org/browse/PICARD-503 this discussion is blocking you from releasing a stable API. So maybe I ask it the other way this time: Were do you see the downside of not having DEFAULT_DEVICE a constant?

Not having it a constant would mean the python-discid API is decoupled from our general "how constant should the default device be" discussion. This is a pretty nice example on how constants can lead to a less stable and future proof API ;)

[JonnyJD]

So maybe I ask it the other way this time: Were do you see the downside of not having DEFAULT_DEVICE a constant?

Pure API design reasons.

Using a function, rather than a constant gives more possibilities. That is correct.
However, that also opens new questions on how this function can and should be used.
A constant would be valid to save in a configuration file, the result of a function possibly wouldn't. So you have to display the device name somewhere else than in the config. Otherwise the configuration might work once, but not the next time.
With a constant you are certain that it will stay the same for the whole time the code is running. You don't need to make sure you request it at one time and save it for later use (making sure the device name is consistent for one invocation).
While you can do all of that also with a function (just create an own constant and assign get_default_device()),
it complicates things for the user of the API.
API design is not only about making things possible. It is also about making things simple and/or easy to use.

I am not at all opposed to having a default device that is of actual use to the user.
I do have concerns though that having some dynamic default string makes it difficult to save and display the default for later use (like in a configuration option of a GUI).

For Mac OS X I proposed a solution (having numbers, possibly adding the (current) device name as a run-time information). For Linux/BSD/Solaris a default is already anticipated (though /dev/cdrom on linux is by convention, not sure if there is an actual standard involved, probably not; on the BSDs this works fine out of the box).
For Windows I have no definate answer. I would use numbers like on Mac, but actual Windows users are probably opposed to that solution. And I heard there are a lot of Windows users.

So I really would like to hear some feedback/opinion.
If I do implement an API I am not happy with, I would at least like to know this was due to popular demand.
I actually hoped I would have a bit more feedback from Windows and Mac users by now.

[zas]

I would prefer a function, returning an ordered list of detected devices, with the first one = default.

This way, if libdiscid is able to provide the list of devices (ie. on win D:,E: or on linux /dev/cdrom, /dev/dvrom, /dev/sr3 ), we'll still have one call.
I would do something discid.get_devices([features]), returning a list of devices supporting specified features if any.

And implement it as discid.get_devices() == [disc.DEFAULT_DEVICE], for now.
We have rough python code to detect devices for win and linux in Picard ( https://github.com/musicbrainz/picard/blob/master/picard/util/cdrom.py ), i think it would be great to move this out, to python-discid, and perhaps to libdiscid.

Of course, i would keep constant for a while for compatibility, deprecated.

[JonnyJD]

Returning lists is something that would work fine for python-discid, but not so well in C on the actual libdiscid. Returning string lists isn't something that can be done nicely in a C API (due to explicit memory management).
At least something that would need an actual API change in libdiscid.
Though having something like that would be interesting, that would really be independent of DEFAULT_DEVICE then (separate feature and function)

Moving the get_devices() function from picard to python-discid is an option, although that also would be different from DEFAULT_DEVICE. DEFAULT_DEVICE or get_default_device() should be part of the list returned by get_devices().

`features` are currently not device dependent, but platform dependent. Technically it would be possible that some devices support these features and others don't. However in practice this doesn't seem to occur anymore. The reason some platforms don't support all features is that every platform needs a different implementation for these as this is kernel/driver dependent.

This is basically about (newly created) #33, but doesn't directly answer the question whether the (single) default value should be DEFAULT_DEVICE or get_default_device().
Or I missed your point.

[zas]

Well, then the question is simply : is "default device" something that change or no ?
And i would say No, so a constant is fine for me.

[JonnyJD]

Well, not trying to argue for both sides, but to get the facts right:

The current libdiscid code gets the first available disc drive on Mac OS X and returns it as default. So when you remove the disc from the drive, mount a .dmg file and then put again a disc in the drive the device name changes (like from /dev/rdisk2 to /dev/rdisk3).
So using only a constant implies that we don't do that and return "1" in both cases, accept "1" as a device and then just use the first available device.

Similar for Windows and USB disc drives, but not as bad. However, on Windows people actually understand drive letters as device names and the solution just returning "1" might not be what people want.
FYI: on Windows 9x libdiscid did return "cdaudio" as a default, which was essentially that. This doesn't work on NT-based Windows though.

So a decision for having "DEFAULT_DEVICE" as a constant is probably a decision for using "1" for OSs that don't have stable device names in libdiscid (and adding the feature to return a list of currently available device names later on)

[zas]

@JonnyJD : Thanks for this explanition.
As far i understand, we have a "default device" that can change during runtime, depending on OS.
I had a look at libdiscid code, and it is clear that for some systems (ie. darwin) we have a path that is generated using functions, and two different calls may result in 2 different names.
Then i would use get_default_device() instead DEFAULT_DEVICE as it is more flexible, and reflect the dynamic generation better. Not that it makes much difference in python, but for me, constants always smell like C constants...

[phw]

@zas: You should probably read the related ticket at http://tickets.musicbrainz.org/browse/LIB-28 . This python-discid ticket here depends mostly on the outcome of the discussion there (and JonnyJD and myself don't come to a conclusion there, so feedback is appreciated).

[JonnyJD]

This is not decided, but I added a commit to show what possible code change we are talking about.

[zas]

It looks better in my eyes as a function.

[JonnyJD]

me in LIB-28:

We should probably change to get_default_device() in python-discid (as phw said from the beginning).
The reason being USB disc drives. (Changing drive letter on Windows and from /dev/cdrom to /dev/sr* on Linux).
I don't like having unstabilities, but we don't lose much with that. And we gain the possibility to attach USB drives without restarting the application.

I will probably make a decision on Monday.