Re: FIXMEs in GUI/IDE code

[Torsten]

On 06.01.2013 19:33, Daniel J Sebald wrote:
> 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

I realized something similar (using a struct) but without storing the
icon's paths in the settings. Thanks for the tips!

Torsten