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

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> 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).

Alright, I've removed file locking from unity-api and implemented here to retain behavioural compatibility with QSettings.

« Back to merge proposal