Re: FIXMEs in GUI/IDE code

[Torsten]

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?

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

As mentioned above, I think there are more important problems at the
moment. But it could be a nice feature in the future.

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

The array is not supposed to be configurable but my intention was to
bundle constant definitions used in several files in a central place. In
this case, the array is only used in main-window.cc and the related enum
in settings-dialog.cc. But since the two parameters are definitely
belonging together I would like to move the array declaration back to
ressource-manager.h (without static).

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

> 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?

The relevant code creating the settings pointer is in
ressource-manager.cc:

void
resource_manager::do_set_settings (const QString& file)
{
  delete settings;
  settings = new QSettings (file, QSettings::IniFormat);
}

Whenever resource_manager::get_settings() is requested in other files,
the settings pointer created in do_set_settings() is just returned. In
my opinion we only have to check for a null pointer in this function.

Even if "file" does not exist, a new empty one is created (therefore we
have to make sure that all settings->value() calls are provided with a
default value of the parameter). Whatever might cause Qt to return a
null pointer (maybe a non existing file in a directory with no write
access), we have two possibilities:

1. Print out an error message and stop the program
2. Let Qt generate a settings file at one of the default positions qt
generally uses for setting files depending on the platform


Torsten