video room plugin: introduced string_ids_user

[IgorKhomenko]

Issue reference #3364 (comment)

[januscla]

Thanks for your contribution, @IgorKhomenko! Please make sure you sign our CLA, as it's a required step before we can merge this.

[lminiero]

I see many problems in this patch:

1. there's no place where you initialize string_ids_user from the configuration file, which is what we do for string_ids; this means people need to set the static value to TRUE and recompile if they want to use this, which makes little sense;
2. this is not backwards compatible, since if people set string_ids to TRUE, the user counterpart defaults to FALSE meaning user IDs will not be strings, breaking people's expectations (and possibly applications);
3. it makes little sense to only offer this flexibility for user IDs; as someone else mentioned in the issue, someone may want to do it the other way around (strings for rooms, numbers for users).

As such, this patch needs a lot of fixing IMHO before we can review/test it. While it's ok to use the dedicated booleans for the checks, these should all be configurable via the configuration file, and when the new properties are not provided, the old string_ids should act as a fallback.

[IgorKhomenko]

@lminiero

1 - this is addressed and pushed
2 - what you say make total sense. Probably we can check if string_ids_user is intentionally presented in config (either set to true or false) and if not - then use string_ids as a fallback.
3 - did not get the point. With this patch there is now a way to separately configure it for rooms and users, e.g. it can be any combination, e.g. true/true or true/false or false/true or false/false

[lminiero]

did not get the point. With this patch there is now a way to separately configure it for rooms and users

I don't see this possibility: string_ids is for both, not only for rooms, which is why I stressed the backwards compatibility part. You can't change the meaning of a property to have it only impact part of what it impacted before, it would break existing applications. There would need to be a new property called, e.g., string_ids_room specifically for rooms, like you added one specifically for users, and both should default to whatever the value of string_ids is set to, unless individually changed to something else via config.

[lminiero]

@IgorKhomenko did you have a chance to revisit the PR with the changes I suggested?

[lminiero]

Hi @IgorKhomenko, did you close the PR to create a different one, or are you not interested in adding the feature anymore? I was waiting for feedback but didn't get any. Thanks!