Fix edge case of playerName with uppercase letters

[ashthespy]

Fixes the renaming of device back to default (for the edge case that I had of a playerName in camelCase ).

It would seem that my issues were nothing related to mdns and node v8 ;-)

[xipmix]

I'm confused about how this change helps. Can you state what your problem was before and how the change fixes it?
I'm guessing it's because mdns doesn't like camelCase and forcing lowercase avoids the issue, but I could be wrong.

[ashthespy]

The ControllerSystem.prototype.saveGeneralSettings lets you set an arbitrary playerName, which in my instance was in camelCase.

However in volumiodiscovery the if ((theName != name) && (!forceRename)) test will fail for this case, resetting the playerName

[xipmix]

That helps a lot, thank you.

[messismore]

I'm guessing it's because mdns doesn't like camelCase and forcing lowercase avoids the issue, but I could be wrong.

FWIW, "MacBook-Pro", or "iPhone" doesn't seem to be a issue in my network…

It might be a pet peeve, but having Volumio players announce themselves with all lowercase names for Airplay and Spotify Connect irks me more than it should. So inelegant… 🙈

[ashthespy]

I don't know the history and but given that there is .toLowerCase().replace(/ /g,'-'); applied to playerName to convert it to a valid hostname, can't we just keep playerName to what the user sets?

I'm guessing it's because mdns doesn't like camelCase and forcing lowercase avoids the issue, but I could be wrong.

FWIW, "MacBook-Pro", or "iPhone" doesn't seem to be a issue in my network…

It might be a pet peeve, but having Volumio players announce themselves with all lowercase names for >Airplay and Spotify Connect irks me more than it should. So inelegant… 🙈

I could not agree more - I have hard coded the device names in my Spotfy connect installation for exactly this reason!

Also, according to Bonjour's specs you can have quite creative ServiceNames!

[messismore]

…can't we just keep playerName to what the user sets?

I don't know what else playerName is used for, but I'll try to do some digging as soon as I find time for it next week…

See #253

[volumio]

playerName is the actual name of the Volumio player

[ashthespy]

@volumio Since we derive the hostname anyway from the playerName, can't we allow some flexibility (utf-8 and caps) in it?
A quick look shows that playerName is utilised mainly by upnp , airplay_emulation, networkfs and volumiodiscovery and other plugins (volspotconnect).

[volumio]

@ashthespy yes we can, however the hostname should be all lowercase and with - instead of spaces. What's your idea?

[ashthespy]

@volumio How about a validation check in ControllerSystem.prototype.saveGeneralSettings with a warning notification if the user tries setting a playerName that doesn't play(hehe) well with Volumio?
Additionally, we can also push a notification if the parsed hostname is different from the playerName.

Something along the lines of:

hostenameRegex = /^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])+/g;
var validHostname = player_name.match(hostenameRegex)[0] || 'volumio';
if	(validHostname === player_name) {
	self.commandRouter.pushToastMessage('success', self.commandRouter.getI18nString('SYSTEM.SYSTEM_CONFIGURATION_UPDATE'), self.commandRouter.getI18nString('SYSTEM.SYSTEM_CONFIGURATION_UPDATE_SUCCESS'));
} else {
	self.commandRouter.pushToastMessage('warn', self.commandRouter.getI18nString('SYSTEM.SYSTEM_CONFIGURATION_UPDATE'), 'SYSTEM.SYSTEM_CONFIGURATION_HOSTNAME_WARNING');
}
self.setHostname(validHostname);

[xipmix]

+1 for this. Many months ago I tried writing a function to do this and some associated tests but got bogged down with all the name conventions that are allowed. I'll see if I can dig it out and attach here.

[ashthespy]

I just quickly looked up the underlying daemons (upmpdcli , shairport-sync,mdsn) for their respective 'name' parameters, and it looks like there isn't explicit constraints!
Need to check utf8 support though.

@xipmix Do you mean for conventions for hostname? The regex snippet matches the RFC 1123, but only for the first part of a FQDN.

[ashthespy]

@volumio a little OT, but is there a specific reason behind displaying device names in uppercase in volumiodiscovery?

[volumio]

@ashthespy mainly for aesthetics

[xipmix]

The 'conventions' for hostname I was referring to were for mdns, netbios, upnp, etc. See #974, #1361. But one step at at time, let's sort out DNS (and mDNS) first.

[ashthespy]

A small summary of my findings so far. I found playerName, system.name, system_name and hostname which each seem to be used in different places.

system.name used in:

app/plugins/music_service/airplay_emulation/index.js:  var name = this.commandRouter.sharedVars.get('system.name');
app/plugins/system_controller/networkfs/index.js:      var systemShare = self.commandRouter.sharedVars.get('system.name').toUpperCase();
app/plugins/system_controller/system/index.js:         this.commandRouter.sharedVars.addConfigValue('system.name', 'string', self.config.get('playerName'));
app/plugins/system_controller/system/index.js:         self.commandRouter.sharedVars.set('system.name', player_name);
app/plugins/user_interface/websocket/index.js:         var name=self.commandRouter.sharedVars.get('system.name');app/plugins/user_interface/websocket/index.js:                          var name = self.commandRouter.sharedVars.get('system.name');

playerName Used in:

app/index.js:   var name = file.playerName.value;
app/plugins/audio_interface/upnp/index.js:               var nameraw = systemController.getConf('playerName');
app/plugins/system_controller/networkfs/index.js:        var nameraw = systemController.getConf('playerName');
app/plugins/system_controller/volumiodiscovery/index.js: var name = systemController.getConf('playerName').toLowerCase();
app/plugins/system_controller/volumiodiscovery/index.js: systemController.setConf('playerName', theName);

system_name used locally in wizard:

app/plugins/miscellanea/wizard/config.json:  "system_name":{"value":"Volumio","type":"string"}
app/plugins/miscellanea/wizard/index.js:     var systemName = self.config.get('system_name', 'Volumio');

I don't fully understand the difference b/w playerName and the system.name but can we use the playerName for things that are more flexible (mDNS, display, etc) and use system.name as the sanitised name that is used for netbios, upnp, hostname, etc
I started writing some regex for this, but don't know who all will play nice with utf8.
Would https://github.com/bestiejs/punycode.js be a more straight forward option for the sanitising then?

[xipmix]

I dug out the stuff I wrote a while ago. It's incomplete and probably the wrong direction but maybe the tests I wrote could be reworked into something useful. See attached. Licence: public domain.
hostnamecheck.txt
hostnamecheck-tests.txt

I don't know what to advise about depending on punycode. I would suggest getting something that works with the ascii subset, with the intention of adding utf8 support in another iteration.

[ashthespy]

Sorry, github didn't send me any emails for some reason.

@xipmix That is quite extensive and complete, I was thinking of a more simple Ok/NotOk kind of test for the hostname and defaulting to volumio, but your tests make it easier for sure!

I made some preliminary tests with punycode for utf-8 hostnames, and it's quite network depended I feel. My browser could find volümiö.local (by automatically converting it to xn--volmi-nua3b.local but ping and unpnp had some issues.

Given the broad user base of Volumio, I would again suggest we use the PlayerName externally as the 'pretty name' and use the system.name internally (mDNS, upnp, hostname)

If everyone is along the same lines, I shall make a small POC the following days :-)

[volumio]

@ashthespy Hi Ash, sorry for this super late reply...
Let me explain a bit what happens in this part of code.
This is to avoid to have 2 devices on the same network which share the same name, and the purpose of this function is to rename a player if a device with the same name is found.
We take the service name as a reference for the comparison, under the assumption that the service === volumio. But we could well use a mechanism where, if a variable under system called firstName is not set, we set it with the current name. Then we add - + an ordinal number after it, to differentiate the devices names.

However, this is now broken, and I admit that system.name does not help much: basically this is the same as system_name but it's a shared variable used to avoid to call the system plugin to retrieve the configuration.

We had lot of issues with this mdns thing, since when you start airplay, for instance, it finds a duplicate neame, or when you restart the networking stack.
IMHO the regression happened here: f6727fa#diff-dd2c024b6424de9eb10317fc5942d581

Probably the hostname command, (which before was not successful with names with more than a word) triggers an edge case we did not consider, probably resulting in avahi restarting too many times and making mdns go nuts...

Hope this helps you understand and looking forward for your POC!

[messismore]

Has this been fixed?

[barereef]

@volumio This is still an open issue in 2.917. Upon reset the device name changes to dash-case.

[messismore]

Fixed in Volumio 3 🥳