Opened 8 years ago
Last modified 6 years ago
#3990 new defect
[PATCH] ConfigDB complains about empty strings
Reported by: | elexis | Owned by: | |
---|---|---|---|
Priority: | Should Have | Milestone: | Backlog |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
#3979 added the first two config entries that allow to enter strings.
However the current options code throws an error if the user saves an empty string to be used later:
ERROR: Encountered config setting 'playername.singleplayer' without value while parsing 'config/user.cfg' on line 12
Reproduce: Empty the singleplayername, close the dialog, enter the singleplayermenu. Reported by Imarok in irc on 2016-05-16.
It will replace the emptystring automatically with the OS username. But the message is still shown and misleading.
The message was added in r15980.
Adding a default.cfg
entry doesn't influence the appearance of the message.
If default.cfg
contains a non-empty string, the empty string from user.cfg
won't replace it and the message is still shown.
20:51 <@leper> removing it works, but hides the fact that a higher priority config file cannot delete settings by just setting it to ""
It were consistent if the config system wouldn't care whether the given string was empty and just use that if specified (even when passing a comma-separated list).
Attachments (1)
Change History (6)
by , 8 years ago
Attachment: | t3990_allow_empty_strings_in_config_v1.patch added |
---|
comment:1 by , 8 years ago
Keywords: | review removed |
---|
See comments in irc (http://irclogs.wildfiregames.com/2016-06-14-QuakeNet-%230ad-dev.log)
comment:3 by , 7 years ago
Description: | modified (diff) |
---|
Besides the empty SP/MP playername, the empty buddylist from https://code.wildfiregames.com/D209 also triggers this error.
From 2016-05-16:
20:32 < elexis> it's due to the defaults.cfg not having an entry for that 20:37 < elexis> we could just delete that LOGERROR 20:50 < elexis> it was added in r15980 20:50 < WildfireBot> r15980. Author: leper. Commit message: Use an FSM to parse our config files instead of using CParser. Refs <a class="closed ticket" href="/ticket/2589" title="task: Remove CParser (closed: fixed)">#2589</a>. Print error messages if we encountered an invalid setting. http://trac.wildfiregames.com/changeset/15980 20:51 <@leper> removing it works, but hides the fact that a higher priority config file cannot delete settings by just setting it to "" 20:52 <@leper> which is the reason this message exists 20:54 < elexis> so it would overwrite the config value with emptystring 20:54 < elexis> not sure if we need a warning / error msg for that 21:14 <@leper> elexis: nope, it ignores the setting 21:15 <@leper> elexis: as it would ignore the last line otherwise I think it should stay 21:15 <@leper> and if people edit the config file manually (which they shouldn't have to) then they should fix warnings they cause 21:15 < elexis> ah, so if default.cfg has foo="bar" and user.cfg has foo="" it would yield foo="bar"? hm 21:17 <@leper> very likely, check the code to be sure
From 2016-05-19:
14:31 <@leper> if someone else fixes all the issues that will arise, meh 14:31 <@leper> if not, no 14:31 < elexis> which issues would arise? 14:31 < elexis> if the config file specifies empty string, it should load empty string, no? 14:31 <@leper> what happens if code expects a setting to actually be there? 14:32 <@leper> and being set to something 14:32 < elexis> then that function should be fixed 14:34 <@leper> likely, but then again there are likely countless places that will break in one way or another from someone feeding you bogus config data 14:34 <@leper> (which shouldn't be any more trusted than any other user input, which is why the config parser is conservative in accepting data 14:34 <@leper> ) 14:40 < elexis> the users of config values shouldnt rely on not getting an empty string, I can check many occurances. (there are probably many cases for broken user data which are not checked also) 14:43 <@leper> true, but you have to start somewhere 14:44 < elexis> start: the user cant break anything from the GUI :P 14:46 <@leper> fix: check if a config setting exists before using it? 14:47 <@leper> (or unassign me from config related tickets and do whatever if I don't have to bother about any bugs arising from that)
From 2016-06-15:
17:14 <@leper> #3990 I still don't think that is the way to go, but if someone wants to handle and be responsible for all the bugs that will cause, meh
If a comma separated list is given, the empty strings will be recognized correctly too, even if the list ends with a comma.