[Octave-bug-tracker] [bug #53835] GUI Settings: Variables used by handle_settings (e.g., m_custom_style, et al.) are not initialized

[Dan Sebald]

URL:
  <http://savannah.gnu.org/bugs/?53835>

                 Summary: GUI Settings: Variables used by handle_settings
(e.g., m_custom_style, et al.) are not initialized
                 Project: GNU Octave
            Submitted by: sebald
            Submitted on: Fri 04 May 2018 04:07:49 PM UTC
                Category: GUI
                Severity: 3 - Normal
                Priority: 5 - Normal
              Item Group: Missed Error or Warning
                  Status: None
             Assigned to: None
         Originator Name: 
        Originator Email: 
             Open/Closed: Open
         Discussion Lock: Any
                 Release: dev
        Operating System: Any

    _______________________________________________________

Details:

As part of fix for Bug #53276 I originally made a change that basically
reversed the order in which the GUI layout and GUI settings are done at
startup.  That is, I made the change:


@@ -1203,34 +1188,35 @@ namespace octave
   }
 
   void main_window::read_settings (void)
   {
     QSettings *settings = resource_manager::get_settings ();
 
     if (! settings)
       {
         qDebug ("Error: QSettings pointer from resource manager is NULL.");
         return;
       }
 
+    emit settings_changed (settings);
+
     set_window_layout (settings);
 
     // restore the list of the last directories
     QStringList curr_dirs
       = settings->value ("MainWindow/current_directory_list").toStringList
();
     for (int i=0; i < curr_dirs.size (); i++)
       {
         m_current_directory_combo_box->addItem (curr_dirs.at (i));
       }
-    emit settings_changed (settings);
   }


The reason I moved the emitted settings_changed() sooner is that it looked to
me that restoring the layout was emitting settings_changed() which in turn
called slots:

 
  void
  octave_dock_widget::handle_active_dock_changed (octave_dock_widget *w_old,
                                                  octave_dock_widget *w_new)
  {
    if (m_custom_style && this == w_old)
      {
        set_style (false);
        update ();
      }

    if (m_custom_style && this == w_new)
      {
        set_style (true);
        update ();
      }
  }


and m_custom_style and all the similar variables within set_style() had not
been initialized yet.  To see that this is the case, here's a dump of what's
happening, in which ALL-CAPS is what's happening inside
main_window::focus_changed() and lower case is what's happening inside
octave_dock_widget::handle_active_dock_changed():


M_ACTIVE_DOCK:0  DOCK:0xfaa5a0
m_custom_style:1  w_old:0  w_new:0xfaa5a0  this:0xd8bb10
m_custom_style:10  w_old:0  w_new:0xfaa5a0  this:0xf702b0
m_custom_style:41  w_old:0  w_new:0xfaa5a0  this:0xfaa5a0
m_custom_style:0  w_old:0  w_new:0xfaa5a0  this:0x10ffd90
m_custom_style:115  w_old:0  w_new:0xfaa5a0  this:0x1445850
m_custom_style:107  w_old:0  w_new:0xfaa5a0  this:0x141e220
m_custom_style:47  w_old:0  w_new:0xfaa5a0  this:0x14a38b0
M_ACTIVE_DOCK:0xfaa5a0  DOCK:0xd8bb10
m_custom_style:1  w_old:0xfaa5a0  w_new:0xd8bb10  this:0xd8bb10
m_custom_style:10  w_old:0xfaa5a0  w_new:0xd8bb10  this:0xf702b0
m_custom_style:41  w_old:0xfaa5a0  w_new:0xd8bb10  this:0xfaa5a0
m_custom_style:0  w_old:0xfaa5a0  w_new:0xd8bb10  this:0x10ffd90
m_custom_style:115  w_old:0xfaa5a0  w_new:0xd8bb10  this:0x1445850
m_custom_style:107  w_old:0xfaa5a0  w_new:0xd8bb10  this:0x141e220
m_custom_style:47  w_old:0xfaa5a0  w_new:0xd8bb10  this:0x14a38b0
M_ACTIVE_DOCK:0xd8bb10  DOCK:0xf702b0
m_custom_style:1  w_old:0xd8bb10  w_new:0xf702b0  this:0xd8bb10
m_custom_style:10  w_old:0xd8bb10  w_new:0xf702b0  this:0xf702b0
m_custom_style:41  w_old:0xd8bb10  w_new:0xf702b0  this:0xfaa5a0
m_custom_style:0  w_old:0xd8bb10  w_new:0xf702b0  this:0x10ffd90
m_custom_style:115  w_old:0xd8bb10  w_new:0xf702b0  this:0x1445850
m_custom_style:107  w_old:0xd8bb10  w_new:0xf702b0  this:0x141e220
m_custom_style:47  w_old:0xd8bb10  w_new:0xf702b0  this:0x14a38b0
M_ACTIVE_DOCK:0xf702b0  DOCK:0x10ffd90
m_custom_style:1  w_old:0xf702b0  w_new:0x10ffd90  this:0xd8bb10
m_custom_style:10  w_old:0xf702b0  w_new:0x10ffd90  this:0xf702b0
m_custom_style:41  w_old:0xf702b0  w_new:0x10ffd90  this:0xfaa5a0
m_custom_style:0  w_old:0xf702b0  w_new:0x10ffd90  this:0x10ffd90
m_custom_style:115  w_old:0xf702b0  w_new:0x10ffd90  this:0x1445850
m_custom_style:107  w_old:0xf702b0  w_new:0x10ffd90  this:0x141e220
m_custom_style:47  w_old:0xf702b0  w_new:0x10ffd90  this:0x14a38b0
M_ACTIVE_DOCK:0x10ffd90  DOCK:0x1445850
m_custom_style:1  w_old:0x10ffd90  w_new:0x1445850  this:0xd8bb10
m_custom_style:10  w_old:0x10ffd90  w_new:0x1445850  this:0xf702b0
m_custom_style:41  w_old:0x10ffd90  w_new:0x1445850  this:0xfaa5a0
m_custom_style:0  w_old:0x10ffd90  w_new:0x1445850  this:0x10ffd90
m_custom_style:115  w_old:0x10ffd90  w_new:0x1445850  this:0x1445850
m_custom_style:107  w_old:0x10ffd90  w_new:0x1445850  this:0x141e220
m_custom_style:47  w_old:0x10ffd90  w_new:0x1445850  this:0x14a38b0
M_ACTIVE_DOCK:0x1445850  DOCK:0x14a38b0
m_custom_style:1  w_old:0x1445850  w_new:0x14a38b0  this:0xd8bb10
m_custom_style:10  w_old:0x1445850  w_new:0x14a38b0  this:0xf702b0
m_custom_style:41  w_old:0x1445850  w_new:0x14a38b0  this:0xfaa5a0
m_custom_style:0  w_old:0x1445850  w_new:0x14a38b0  this:0x10ffd90
m_custom_style:115  w_old:0x1445850  w_new:0x14a38b0  this:0x1445850
m_custom_style:107  w_old:0x1445850  w_new:0x14a38b0  this:0x141e220
m_custom_style:47  w_old:0x1445850  w_new:0x14a38b0  this:0x14a38b0
M_ACTIVE_DOCK:0x14a38b0  DOCK:0x141e220
m_custom_style:1  w_old:0x14a38b0  w_new:0x141e220  this:0xd8bb10
m_custom_style:10  w_old:0x14a38b0  w_new:0x141e220  this:0xf702b0
m_custom_style:41  w_old:0x14a38b0  w_new:0x141e220  this:0xfaa5a0
m_custom_style:0  w_old:0x14a38b0  w_new:0x141e220  this:0x10ffd90
m_custom_style:115  w_old:0x14a38b0  w_new:0x141e220  this:0x1445850
m_custom_style:107  w_old:0x14a38b0  w_new:0x141e220  this:0x141e220
m_custom_style:47  w_old:0x14a38b0  w_new:0x141e220  this:0x14a38b0
m_custom_style:1  w_old:0  w_new:0x141e220  this:0xd8bb10
m_custom_style:1  w_old:0  w_new:0x141e220  this:0xf702b0
m_custom_style:1  w_old:0  w_new:0x141e220  this:0xfaa5a0
m_custom_style:1  w_old:0  w_new:0x141e220  this:0x10ffd90
m_custom_style:1  w_old:0  w_new:0x141e220  this:0x1445850
m_custom_style:1  w_old:0  w_new:0x141e220  this:0x141e220
m_custom_style:1  w_old:0  w_new:0x141e220  this:0x14a38b0
M_ACTIVE_DOCK:0x141e220  DOCK:0xd8bb10
m_custom_style:1  w_old:0x141e220  w_new:0xd8bb10  this:0xd8bb10
m_custom_style:1  w_old:0x141e220  w_new:0xd8bb10  this:0xf702b0
m_custom_style:1  w_old:0x141e220  w_new:0xd8bb10  this:0xfaa5a0
m_custom_style:1  w_old:0x141e220  w_new:0xd8bb10  this:0x10ffd90
m_custom_style:1  w_old:0x141e220  w_new:0xd8bb10  this:0x1445850
m_custom_style:1  w_old:0x141e220  w_new:0xd8bb10  this:0x141e220
m_custom_style:1  w_old:0x141e220  w_new:0xd8bb10  this:0x14a38b0
>> M_ACTIVE_DOCK:0xd8bb10  DOCK:0xfaa5a0
m_custom_style:1  w_old:0xd8bb10  w_new:0xfaa5a0  this:0xd8bb10
m_custom_style:1  w_old:0xd8bb10  w_new:0xfaa5a0  this:0xf702b0
m_custom_style:1  w_old:0xd8bb10  w_new:0xfaa5a0  this:0xfaa5a0
m_custom_style:1  w_old:0xd8bb10  w_new:0xfaa5a0  this:0x10ffd90
m_custom_style:1  w_old:0xd8bb10  w_new:0xfaa5a0  this:0x1445850
m_custom_style:1  w_old:0xd8bb10  w_new:0xfaa5a0  this:0x141e220
m_custom_style:1  w_old:0xd8bb10  w_new:0xfaa5a0  this:0x14a38b0
>> M_ACTIVE_DOCK:0xfaa5a0  DOCK:0xd8bb10
m_custom_style:1  w_old:0xfaa5a0  w_new:0xd8bb10  this:0xd8bb10
m_custom_style:1  w_old:0xfaa5a0  w_new:0xd8bb10  this:0xf702b0
m_custom_style:1  w_old:0xfaa5a0  w_new:0xd8bb10  this:0xfaa5a0
m_custom_style:1  w_old:0xfaa5a0  w_new:0xd8bb10  this:0x10ffd90
m_custom_style:1  w_old:0xfaa5a0  w_new:0xd8bb10  this:0x1445850
m_custom_style:1  w_old:0xfaa5a0  w_new:0xd8bb10  this:0x141e220
m_custom_style:1  w_old:0xfaa5a0  w_new:0xd8bb10  this:0x14a38b0


Notice how m_custome_style is some random value for all the
octave_dock_widgets in the first pass.  Then after the "emit settings_changed
(settings)" the m_custom_style has been initialized.  The issue with the
change I made (which ostensibly initializes the variables first) is that not
having the octave_dock_widget::handle_settings() call *after* the layout meant
the File Browser was left highlighted when it shouldn't have been (until
activating and deactivating corrected the matter).  That because at the end of
octave_dock_widget::handle_settings() is the following:


#endif

    notice_settings (settings);  // call individual handler

    set_style (false);
  }


That is, the set_style(false) resets the octave_dock_widget highlight state to
off.  Whether this should be done is one question, but it looks like currently
some other code is doing a re-activate of m_active_dock after a
"Preferences...", so that is covered.

In summary, the issue is mainly that m_custom_style, m_bg_color_active,
m_fg_color_active, m_icon_color_active, m_bg_color, m_fg_color, and
m_icon_color are being used via main_window::read_settings() before they are
initialized to anything.  I suppose the worst consequence of this is that the
octave_dock_widgets are (non-visibly) being assigned random colors that are
then overwritten just prior to making the whole application visible.  In other
words, this doesn't appear to be a segfault type of issue.  One approach would
be to issue


+    emit settings_changed (settings);


both before and after the a layout code (the first initializes the
m_custom_style, etc. and the second clears all the highlights).  However,
probably the more proper thing to do is to initialize the m_custom_style, etc.
in the octave_dock_widget::octave_dock_widget() constructor.




    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?53835>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/