FIXMEs in GUI/IDE code

[Daniel J Sebald]

One of the discussions at OctConf12 was the amount of FIXMEs in the 
Octave code.  Let's try to not be introducing more, especially in the 
case of the GUI which is relatively free so far.  Another reason this is 
good practice especially for the GUI is that paradigms are very 
important for widget communication and by avoiding FIXMEs we can avoid 
bad paradigms from taking root.  Instead, bring questionable issues to 
the discussion list so that Jacob, Torsten and others can give input.

Here are a couple just introduced with changeset 
http://hg.savannah.gnu.org/hgweb/octave/rev/9cd14e53e906

libgui/src/m-editor/file-editor.cc

     5.6    QWidget *editor_widget = new QWidget (this);
     5.7 -  QStyle *editor_style = QApplication::style ();
     5.8 +
     5.9 +  // FIXME -- what was the intended purpose of this unused 
variable?
    5.10 +  // QStyle *editor_style = QApplication::style ();
    5.11

The Qt documentation describes what the QStyle is:

http://doc.qt.digia.com/qt/qstyle.html#details

I don't think we want the editor having its own look-and-feel, so I see 
no reason to have an "editor_style".  In addition to removing that line 
of code, the include statement "#include <QStyle>" can probably be 
removed from file-editor.cc.  "main-window.cc" also has an "#include 
<QStyle>".

However, this raises a question of whether there could be an option 
setting for the QStyle that applies to the whole Octave GUI/IDE.  For 
samples of the styles, see the Qt documentation link above.  I'm 
indifferent to that idea.  Personally I prefer consistent look-and-feel 
for all apps.  I recall ten years ago when Linux was often bundled with 
a dozen look-and-feels to choose from at login.  I'm guessing Qt chooses 
it's default QStyle based upon what OS it is running under.


/libgui/src/main-window.cc

     6.5  main_window::notice_settings ()
     6.6  {
     6.7 +  // FIXME -- was this supposed to be configurable in some 
way?  If so,
     6.8 +  // maybe it should be moved back to 
resource-manager.{h,cc}, but not
     6.9 +  // as a static variable.
    6.10 +  static const char *WIDGET_ICON_SET_PREFIX[] =
    6.11 +    {
    6.12 +      ":/actions/icons/logo.png",
    6.13 +      ":/actions/icons/graphic_logo_",
    6.14 +      ":/actions/icons/letter_logo_"
    6.15 +    };

I'm guessing you wanted to move the declaration for the array out of a 
header file so that there aren't multiple inclusions in object code and 
possible conflicts or duplications.  This one is Torsten's prerogative, 
but I will make a comment with a word of warning.

It appears that the code is set up to store the associated parameter as 
a number.  That number is then used to index the array.  The danger is 
that there is currently no sanity check on that number indexing into the 
array.

237 int icon_set = settings->value 
("DockWidgets/widget_icon_set",0).toInt ();
238 QString icon_prefix = QString (WIDGET_ICON_SET_PREFIX[icon_set]);

Therefore, a bogus number could cause problems.  This is especially 
important with changes in versions of the software as new parameters 
come and go and change.

I suggest storing this icon information, and any such options, in a 
fully descriptive way.  For example, these WIDGET_ICON_SET_PREFIX values 
might be something like:

static const char *WIDGET_ICON_SET_PREFIX[] =
  {
    ":NO LOGO%/actions/icons/logo.png",
    ":GRAPHIC LOGO%/actions/icons/graphic_logo_",
    ":LETTER LOGO%/actions/icons/letter_logo_"
  };

and that whole string is stored in the resources file and retrieved upon 
launching and reinstating user settings.  That means then that there is 
a bit of processing to do, i.e., strip the "NO LOGO", "GRAPHIC LOGO", 
"LETTER LOGO" or whatever else might be there and dynamically load the 
option ComboBox with the alternatives.  Similarly, the 
"/actions/icons/logo.png" etc. needs to be pulled out of there for use. 
 Furthermore, it might be good to check the retrieved setting against 
the strings in the valid set, and if no exact match, discard the setting 
as it is then out of date.


Lastly, there is another associated FIXME:

 219 // FIXME -- what should happen if settings is 0?

meaning what should happen if the contents retrieved from the resource 
file is corrupt or not present?  Well, this requires an organized 
approach.  Do we want to retain previous settings as best as possible 
and fill in any new options?  Discard the whole options list if it isn't 
suited to the version number?  Should there be an array of default 
settings?  Or should it be more code intensive where each setting is 
retrieved and sanity checked individually?

Dan