#8877 closed patch (outdated)
ConfigManager rewrite
| Reported by: | SF/jweinberg | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | --Other-- |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Rewrote configmanager to use ConfigFile as detailed here. http://wiki.scummvm.org/index.php/TODO#Config_Manager
The code is there for objects to register for notifications, but nothing is sending those notifications yet.
Also included some ifdefs for an XCode project that will be forthcoming in the future (I used it while writing this patch, but needs to be cleaned up a lot).
Ticket imported from: #1987055. Ticket imported from: patches/982.
Attachments (2)
Change History (16)
comment:1 by , 16 years ago
comment:2 by , 16 years ago
Submitted a 2nd run through. Removed the XCode ifdefs, fixed formatting issues that i found, and also fixed one build warning that I didn't notice before (non virtual destructor for configfile) File Added: config_changes_take_2.diff
comment:3 by , 16 years ago
I will try to review this tonight (wanted to do it sunday, but as so often my already highly limited time for ScummVM was further reduced by other obligations).
comment:4 by , 16 years ago
| Owner: | set to |
|---|
comment:5 by , 16 years ago
Updated patch for newest revision File Added: config_changes_take_3.diff
comment:6 by , 16 years ago
The patch does not comply to our code formatting conventions (see <http://wiki.scummvm.org/index.php/Code_Formatting_Conventions>). In particular, you write if(foo) not if (foo) but there are other things that are not agreeing with the CFC (mostly spaces missing). This needs to be addressed before the patch is accepted.
The patch introduced a crash in massadd.cpp, which I fixed in the attached revised patch.
Also, there was a bug in config manager, where it used scummvm_stricmp incorrectly (two strings match if this func return 0, just like strcmp, while the code assumed the converse!). For clarity, it is much better to use String::equalsIgnoreCase() anyway (I know, the old code used scummvm_stricmp -- the String method is simply much newer than the ConfigMan/ConfigFile code. Would be nice to update it to the new convention, though). I fixed this issue in the attached revised patch, but would recommend to check whether it occurs in other spots, too.
Regarding the observer class: I am not sure we still need it, so I wonder whether to really add it... but if we add it, there must be a way to unregister an observer again! Else if an Engine registers as an observer, we will get crashes once the engine terminates, as the object will be gone but the config man will still carry a reference to it.
Furthermore, I wonder whether it wouldn't be more elegant and easier to use to employ a signal/slot pattern. This way, you don't need to implement a class in order to be able to listen to config changes. I.e. one could use our existing functor code to achieve this.
Furthemore, an "int event" param seems odd, when you define an enum just atop, which obviously contains the only allowed values for "event". So it'd be both cleaner and easier to understand if that was changed to something like: enum ConfigChangeEvent { ... } ... virtual void configFileChanged(ConfigChangeEvent event, const String §ion, const String &key) = 0;
Also, I just noticed that notifyObservers is never called anywhere -- so the whole observer code is dead. Please either strip it, or fully implement it (but for now I'd recommend stripping it -- let's get the rest of the patch cleared before worrying about adding new features, esp. features we might not need ;).
One major caveat remains: By changing ConfigMan to be completely based on ConfigFile, you have changed its time complexity class. So far, reading config values was a fast operation, as it used hashmap lookups. Now, arrays have to be scanned. As long as the config is small, this should probably not hurt that much. But we should not completely ignore this. The desire to make config manager access fast was the motivating reason to base it on a HashMap in the first place.
All in all, I wonder whether you performed some semi-extensive testing with this patch? Like running through all Launcher dialogs, setting config stuff etc. and testing whether everything still works; launching games; testing the "add" and "edit" game stuff in the launcher, renaming a game target (i.e. a section) etc.. Given that I discovered two crashes within just a few minutes of testing... (not that I'd ever write or even *gasp* commit crashing code *ahem* ;)
File Added: config_changes_take_3-rev.diff
comment:8 by , 16 years ago
I got swamped at work, and also I believe a few things were still up in the air about how you wanted this implemented. If you can give me a better idea of what would be acceptable for implementation I would be happy to continue work on it.
comment:11 by , 15 years ago
I don't think this patch is salvageable. Starting from scratch will probably be easier. In short: * Our code formatting guides must be followed * We don't need observer capabilities anywhere yet; before implementing those, we should first determine whether there are any places were we could make effective use of that. Otherwise, it's just dead code * Changing the internal data storage of the config manager from a hashmap to an array is a risky move; I am doubtful it is a good idea, in fact, and would like to see a rational behind that, and some studies that show that this indeed does not cause severe performance degradation on low-end devices with big config files. * ConfigManager should *not* inherit from ConfigFile, it should instead *use* it. I.e. HAS-A not IS-A should be their relation. All knowledge about the storage format and how to parse it should be isolated in ConfigFile, which simply returns an ordered list of all sections, with their key/value pairs (each strings); also any comments. It preserves the order of these. The ConfigManager should be decoupled from the actual storage; a possible design goal is to make it possible to load from multiple config files (e.g. a system wide one and a user specific one). ConfigManager must provide quick access to the contents of the active settings. However, unlike the current implementations, it does *not* need to keep all sections "loaded" (i.e. no need for a huge hashmap with all sections, and a hashmap for each section). Instead it should be sufficient to keep a single hashmap with the "active" settings, for reading (i.e. the merged contents of the "scummvm" section, the active game section/domain/target, and possibly auxillary ones with keymappings etc.). This is the primary use of the ConfMan: asking for the current value of a setting.
The other major use case are the various options dialogs: These typically want to work on a specific game section/target, and then display those values, letting the users edit it (think of the edit dialog in the launcher dialog). A special use case is the options dialog which is reachable from the global main menu (GMM), which wants to work on the "active" settings. (For that, one has to decide how any settings changes would be written to the config file, if at all).
There are some other minor use cases, like when we add new games via the launcher (then a new section needs to be added, and new values); and in general, deciding how to handle "writes" to the ConfigManager is the most difficult part.
comment:12 by , 13 years ago
Closing this as out of date, given the last comment, since it hasn't seen any change in years.
comment:13 by , 13 years ago
| Resolution: | → outdated |
|---|---|
| Status: | new → closed |
comment:14 by , 6 years ago
| Component: | → --Other-- |
|---|
Seems like formatting is wrong in some places: "const SectionRefList & getGameSections() const { return _gameSections; }" should be "const SectionRefList &getGameSections() const { return _gameSections; }" for example (there are some other occurrences like this with pointers)
That enum in ConfigFile (Event) looks a bit strange too, there should be no reason to use typedef there and also it uses 'e_' as constant/enum prefix while we're using 'k' as in for example: 'kFoo'.
The XCode ifdefs should be removed from the patch, they are completely unrelated and till there's no XCode support there should be no reason to have them in the ScummVM source.
That's all I noticed in a quick run down, I might take a look at it again when I have more time though.