Re: FIXMEs in GUI/IDE code

[Daniel J Sebald]

On 01/06/2013 06:10 AM, Torsten wrote:
> On 06.01.2013 00:42, Daniel J Sebald wrote:
> > 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>".
>
> In my opinion we should not offer the possibility of different styles at
> the moment. There are other open problems concerning the usability of
> the gui which are more important than a configurable look and feel.
> Concerning the FIXME, I just can not answer the question in the comment.
> Jacob?

Agreed.  I say simply remove the lines of code, i.e.,

 // FIXME -- what was the intended purpose of this unused variable?
 // QStyle *editor_style = QApplication::style ();

and

#include <QStyle>

in file-editor.cc and main-window.cc.  Some of us know there is a style 
now, and it's easy enough to add back in a line or two of code.


> > 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.
>
> You are right, I should definitely add a sanity check (in case the
> settings file is corrupted). As long as we are not using an existing
> settings parameter for a new purpose there should be no version clash
> concerning the settings.
>
> > 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.
>
> Do you mean the settings file by "resources file"? I would not store the
> whole information (including the internal path of the icons and their
> name prefix) in the settings file. If there are any changes concerning
> the path or the names of the icon files all settings files would contain
> invalid values.

Yes, resources file.  Let me just illustrate the different ways these 
work with this example, but this by no means is unique.

By just storing a number, say from one version to the next the following:

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 +    };

is modified to:

6.10 +  static const char *WIDGET_ICON_SET_PREFIX[] =
6.11 +    {
6.12 +      ":/actions/icons/logo.png",
            ":/actions/icons/jpeg_logo_",
6.13 +      ":/actions/icons/graphic_logo_",
6.14 +      ":/actions/icons/letter_logo_"
6.15 +    };

then if the user has in his or her settings the number "2", when doing a 
version update what used to be letter_logo is now graphic_logo even 
though the "2" is within bounds.  You may feel that wiping all the 
settings clean takes care of that, but I suggest trying to retain as 
much of the settings as possible.

A similar scenario would be to change

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_"
   };

to

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

If what is stored in the resources file is ":GRAPHIC 
LOGO%/actions/icons/graphic_logo_", the algorithm could be

INDEX=-1
FOR I=0:END_OF_LIST
  IF STRING_COMPARE(RESOURCE_ICON_STRING,WIDGET_ICON_SET_PREFIX[I])
    INDEX=I
  ENDIF
ENDFOR
IF INDEX<0
  INDEX=0;
  POP UP MESSAGE "Conflict with previous resource option, choosing 
default."
ENDIF

Something like that.

Dan