Preferences settings not always reported correctly to code

[Jon A. Cruz]

When working on other code, it was noticed that the icon code was never seeing "dumpDefault" as set to true. When the same places of code were switched to use the "overlaySvg" preference name, no change was observed.

http://wiki.inkscape.org/wiki/index.php/Icons#Debug_Output

That is, the place in the code was always getting "false" as the result, no matter what was set in the preferences, and that those worked later in the same run within other areas of code.

It is critical that all code get proper values reported, no matter when that code is called. This can easily cascade to trigger problems across the entire codebase.

[Jon A. Cruz (jon-joncruz)]

This class is a singleton, but the actual preferences are not loaded in the constructor nor the singleton accessor method. It appears that the safest route is to make the load() call private and move its invocation to the accessor method.

[Krzysztof Kosinski (tweenk)]

I disagree. Preferences should be usable without load(), because otherwise it is impossible (or at least much harder) to unit test them. Additionally, they need to be loaded after we know whether we're in GUI mode or console mode, to emit the correct error messages (on console or as GTK dialog boxes).

If the preferences are needed earlier, move the call to Inkscape::Preferences::load() earlier.

[Jon A. Cruz (jon-joncruz)]

I'm sorry, but you're missing a critical point.

The key is that the preferences *must* be usable. Always. Period.

That means they can't just change in the middle, otherwise code will be broken.

Also for testing, there are many many many ways that will make them testable without having to defer loading. I've worked with TDD for years, so if you need help on testability just ask.

Also, GUI mode vs. non-GUI mode should not be a factor for preferences. Preferences are pure abstract data items and should be loaded the same no matter how the program is loaded. In fact, there are not only two ways the program can be loaded, so trying to detect can quickly grow to a much larger problem than it has to be.

Again, by design a singleton *must* be valid no mater when it is accessed. That's a critical part of the singleton pattern. If not, all sorts of problems arise.

[Krzysztof Kosinski (tweenk)]

> The key is that the preferences *must* be usable. Always. Period.
I disagree again. If you want a setting that is usable at any time during the program's lifetime, use an environment variable. The preferences are intended to store user settings and not to store flags for debugging, because that is not really an user setting.

> Also, GUI mode vs. non-GUI mode should not be a factor for preferences.
> Preferences are pure abstract data items and should be loaded the same
> no matter how the program is loaded.
We need to handle the case where the file is corrupted or unwritable. The user must be notified of this. If we do not distinguish between GUI and non-GUI then either a) the user won't be notified and will wonder what the hell is going on or b) CLI will display dialog boxes and fail when there's no display. Therefore loading of prefs must happen after GTK init and option parsing. It's much easier to provide an explicit loading method than to debug crashes resulting from the preferences being accessed too early somewhere in the app.

A potential solution is to throw an exception if get() is called before setting the interface mode and loading the preferences, and add a new static function get_empty() that would not throw it even if the prefs are not loaded to allow unit testing without loading anything.

[Jon A. Cruz (jon-joncruz)]

You are wrong about preferences. Environment variables are used for something completely different and a quick scan of man pages will show that. Aside from debugging flags (which are user settings), many other preferences are used including flags to enable or disable features.

A quick illustration
* program launch begins
* method A is called, sees a preference is clear and does not add data structures for feature XXX
* launch continues
* method B is called, sees that same preference is set, and tries to use data structures for feature XXX
* program corrupts memory and/or crashes (note that corruption may occur without causing an immediate crash)

Again, user notification is a separate issues. And it is easy to deal with for Inkscape. Your statement "Therefore loading of prefs must happen after..." is incorrect.

*If* preferences are not present or not loaded, the behavior is to fall back to defaults. So the program will at least drop into a known state. Yes, the user must be notified. The very simple way to do this is set an internal boolean flag to indicate problems loading, and add a method to query this.

Then at the point in program initialization where GTK has been intialized and flags have been processed and now does the loading can instead be changed to check for failed loading. If the preference singleton reports that it was unable to load from file and is operating in default mode, the user can be notified.

That solution makes the preference use comply to the standard "singleton" pattern, allows for proper user notification of failure, and is 100% bullet-proof against inconsistent returned results.

[Jon A. Cruz (jon-joncruz)]

A nicer refinement is to have an enum for load status instead of a simple boolean. Values can include "good", "missing", and "corrupted". Among other things in the GUI case this will allow the program , int the case of corrupted settings, to ask the user whether or not to overwrite the corrupted settings with defaults. The state of preference storing can then have a corresponding state and enum for "store" and "in memory only".

[Jon A. Cruz (jon-joncruz)]

Aha!!
I figured out one of the other things complicating a fix.

The preferences class is breaking modularity/encapsulation by use of GUI classes. The end of the include list gives a clue:

#include <vector>
#include <glibmm/fileutils.h>
#include <glibmm/i18n.h>
#include <glib.h>
#include <glib/gstdio.h>
#include <gtkmm/messagedialog.h> <---- this GUI include should not be here

The gtk message dialog should not be built in to the preferences class. Instead some external code that implements the dialogs should be employed. This also frees the preference class to be fed either a stdout "warner" or a gtk dialog "warner" as needed. Sometimes this is known as "dependency injection". The preference class could have a "setErrorHandler()" method that takes an interface/abstract base class, or it could have "addErrorHandler()" and "removeErrorHandler()" calls that add and remove listeners.

Again, please drop into our chat room if any of this has been unclear. It is *much* easier to discuss such architecture in realtime.

[Jon A. Cruz (jon-joncruz)]

Fixed as of revision 21281

[Krzysztof Kosinski (tweenk)]

I know this pattern and the implementation seems OK but you did not address a very important thing. We cannot know which handler to use until the interface mode is known from option parsing. Because of this, get() should not automatically call load(). Otherwise we might end up not displaying the warnings about unwritable prefs at all (because they are only logged to an invisible console). Please double-check whether correct messages are displayed when the preferences.xml file is not writable. Until you can confirm that the correct behavior is preserved I will reopen this.

[Krzysztof Kosinski (tweenk)]

Also please test whether this interferes with make check.

[Jon A. Cruz (jon-joncruz)]

Krzysztof, PLEASE ACTUALLY READ AND TEST CODE BEFORE JUMPING TO CONCLUSIONS!!!

I DID ADDRESS THAT "CRITICAL" POINT IN THE CODE, AND COMMENTED ON IT!!!

IT WAS TESTED AGAINST UNWRITABLE FILES!!!

IT DOES HANDLE WARNINGS OF PROBLEMS THAT OCCUR BEFORE THE COMMAND LINE IS HANDLED!!!

DO NOT LIE ABOUT OTHER PEOPLE'S CODE