Code review comment for lp:~marcustomlinson/unity-scopes-shell/lp-1552082

Thanks! The changes look good, but we need to consider overall impact and edge cases. I don't mean to make your life hard, but this worries me slightly:

37 + catch(const unity::FileException&)
38 + {
39 + // File was not found, so we create an empty one.
40 + auto f = fopen(filePath, "w");
41 + fclose(f);
42 +
43 + m_settings.reset(new unity::util::IniParser(filePath));
44 + }

Before, QSettings would never throw obviously. With the proposed changes to IniParser, an exception from the ctor means it failed on open *or* on obtaining the read lock. Now, when the latter happens, we will try to overwrite the file which may or may not succeed (and potentially overwrite user changes doing so) but we will also try to create IniParser instance, which may throw again and this is not handled (makes shell crash).

I think we should try hard not to loose user changes, failure in obtaining read lock may be a transient issue. This really is a problem, last time I touched settings here and in unity-scopes-api was related to fixing a tricky issue where scope would occasionally go blank when changing the settings (which triggered scope refresh), just because of occasional exception on file access. On a second thought, I think the existing code may be guily of never looking at QSettings::status (but QSettings seems to be doing a good job).