renaming some classes and using the octave namespace in the GUI

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

renaming some classes and using the octave namespace in the GUI

John W. Eaton
Administrator
We have a variety of class names in the GUI that are inconsistent and
that I think are a bit worse than they could be.  For example, we have
variable_editor (OK) but terminal_dock_widget (not so OK).  I propose
the following changes to better reflect the purpose of each object and
to drop the suffix that indicates the type of Qt object that we are
inheriting from:

   doc_browser, not documentation_dock_widget
   file_browser, not files_dock_widget
   history_browser, not history_dock_widget
   command_terminal, not terminal_dock_widget
   workspace_browser, not workspace_view

I'm not sure about command_terminal.  Is there something better?

We are also inconsistent in the names passed to QObject::setObjectName.
For exmaple, we have things like HistoryDockWidget, Shortcut_Manager,
variable_editor, OctaveTerminal (and TerminalDockWidget!).  What should
these be?  Does changing the name affect anything else?

I also propose to move all the GUI classes inside the octave namespace.

Any objections?

jwe



-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

Daniel Sebald
On 02/08/2018 10:56 AM, John W. Eaton wrote:

> We have a variety of class names in the GUI that are inconsistent and
> that I think are a bit worse than they could be.  For example, we have
> variable_editor (OK) but terminal_dock_widget (not so OK).  I propose
> the following changes to better reflect the purpose of each object and
> to drop the suffix that indicates the type of Qt object that we are
> inheriting from:
>
>    doc_browser, not documentation_dock_widget
>    file_browser, not files_dock_widget
>    history_browser, not history_dock_widget
>    command_terminal, not terminal_dock_widget
>    workspace_browser, not workspace_view
>
> I'm not sure about command_terminal.  Is there something better?
>
> We are also inconsistent in the names passed to QObject::setObjectName.
> For exmaple, we have things like HistoryDockWidget, Shortcut_Manager,
> variable_editor, OctaveTerminal (and TerminalDockWidget!).  What should
> these be?  Does changing the name affect anything else?

No, not if object name is not seen.  The "Title" is typically what is
seen.  In a changeset I'm creating, the object name is important because
for the variable editor the object name matches exactly the Octave space
variable name, so that when one tries to open the variable in the editor
more than once it checks all the object names in the space to see it
isn't already open.

That doesn't mean consistency isn't good, because often one does a
QObject->findChild(Object *, QString name), and consistency makes memory
recall easier.


> I also propose to move all the GUI classes inside the octave namespace.
>
> Any objections?

Could we wait a week on this?  I'm right in the process of wrapping up
some changes to the variable editor that turn it into a QMainWindow type
of interface (as opposed to QTabWidget) and such a sweeping change would
make a mess for patches.  Also, I'm doing some restructuring to make
things more in line with signals/slots and that might influence naming
convention in the space.

I'm not sure about the "browser" name change.  I prefer the names match
what they are derived from.  This is sort of how Qt is set up, e.g.,
here's one hierarchy of inheritance:

QAbstractButton
QPushButton
QCommandLinkButton

The reason this is a good thing to do is because the OOP philosophy is
to build on previous classes (reuse), just adding a few new features at
the next level.  The name similarity aids in remembering what something
is derived from and the member functions it would have inherited.

Dan


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

John W. Eaton
Administrator
On 02/08/2018 12:43 PM, Daniel J Sebald wrote:

> In a changeset I'm creating, the object name is important because
> for the variable editor the object name matches exactly the Octave space
> variable name, so that when one tries to open the variable in the editor
> more than once it checks all the object names in the space to see it
> isn't already open.

You lost me there.  Exactly how are Qt Object names relevant for opening
a variable in the variable editor?

jwe


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

John W. Eaton
Administrator
In reply to this post by Daniel Sebald
On 02/08/2018 12:43 PM, Daniel J Sebald wrote:

> I'm not sure about the "browser" name change.  I prefer the names match
> what they are derived from.  This is sort of how Qt is set up, e.g.,
> here's one hierarchy of inheritance:
>
> QAbstractButton
> QPushButton
> QCommandLinkButton

This maybe makes sense for the fairly generic sort of widgets that Qt
provides.  But what I'm talking about are Octave objects.  The history
browser will always be something that allows users to examine the
command history, but it may not always be derived from the QtDockWidget
object.  Having that in the name doesn't make sense to me.  It's like
Hungarian notation.  It seems like it might be a good idea until you
decide to change the type of a variable.

We could use another name than browser but I don't want to append
DockWidget just because it currently happens to be implemented using a
QtDockWidget object.

jwe


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

John W. Eaton
Administrator
In reply to this post by Daniel Sebald
On 02/08/2018 12:43 PM, Daniel J Sebald wrote:

> Could we wait a week on this?  I'm right in the process of wrapping up
> some changes to the variable editor that turn it into a QMainWindow type
> of interface (as opposed to QTabWidget) and such a sweeping change would
> make a mess for patches.  Also, I'm doing some restructuring to make
> things more in line with signals/slots and that might influence naming
> convention in the space.

Are you working on any files outside of the variable editor?

I realize that any changes like this may cause trouble for patches, but
so can any changes, no matter what the purpose.

jwe


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

Daniel Sebald
In reply to this post by John W. Eaton
On 02/08/2018 11:49 AM, John W. Eaton wrote:

> On 02/08/2018 12:43 PM, Daniel J Sebald wrote:
>
>> In a changeset I'm creating, the object name is important because for
>> the variable editor the object name matches exactly the Octave space
>> variable name, so that when one tries to open the variable in the
>> editor more than once it checks all the object names in the space to
>> see it isn't already open.
>
> You lost me there.  Exactly how are Qt Object names relevant for opening
> a variable in the variable editor?

Below is the code hunk that I changed.  If we want to get away from the
tabbed interface (i.e., only one variable visible at a time), then we
can't use a pointer m_tab_widget because that only works for QTabWidget.
  The variable name was being stored in the tab widget name.  The Qt
signal/slot framework allows being less specific about exactly what the
type of objects are and more simply passing data around however the
objects are arrange.  (That provides flexibility because one isn't tied
into the exact arrangement.)

So, we have to store this variable name somewhere else.  How about the
typically unused object name?  Then we can use the Qt-provided
convenient ways of finding objects.  The findChild() is nice in that it
will keep searching the whole parent/child chain.  In other words, we
don't need to have some pointer and indirection to get the info we want,
just that there is some object out there by name "name".  It could be a
QDockWidget, something we derive from QDockWidget (provided we don't
override the meta-type).  If the overall design is changed, just change
QDockWidget to something else, and then one isn't constrained by the
whole m_tab_widget pointer approach.  The other *really* nice thing
about the findChild template is that it returns a pointer of the proper
class and we can avoid that not-recommended dynamic_cast.

Dan

  void
  variable_editor::edit_variable (const QString& name, const
octave_value& val)
  {
    if (m_stylesheet.isEmpty ())
      {
        QSettings *settings = resource_manager::get_settings ();
        notice_settings (settings);
      }

-  const int tab_count = m_tab_widget->count ();
-  for (int i = 0; i < tab_count; ++i)
+  QDockWidget *existing_qdw = m_main->findChild<QDockWidget *> (name);
+  if (existing_qdw != NULL)
      {
-      if (real_var_name (i) == name)
-        {
-          // Already open.
+      // Already open.

-          m_tab_widget->setCurrentIndex (i);
-          return;
-        }
+//          m_tab_widget->setCurrentIndex (i);
+      return;
      }

    // Do not set parent.


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

Daniel Sebald
In reply to this post by John W. Eaton
On 02/08/2018 12:00 PM, John W. Eaton wrote:

> On 02/08/2018 12:43 PM, Daniel J Sebald wrote:
>
>> I'm not sure about the "browser" name change.  I prefer the names
>> match what they are derived from.  This is sort of how Qt is set up,
>> e.g., here's one hierarchy of inheritance:
>>
>> QAbstractButton
>> QPushButton
>> QCommandLinkButton
>
> This maybe makes sense for the fairly generic sort of widgets that Qt
> provides.  But what I'm talking about are Octave objects.  The history
> browser will always be something that allows users to examine the
> command history, but it may not always be derived from the QtDockWidget
> object.  Having that in the name doesn't make sense to me.  It's like
> Hungarian notation.  It seems like it might be a good idea until you
> decide to change the type of a variable.
>
> We could use another name than browser but I don't want to append
> DockWidget just because it currently happens to be implemented using a
> QtDockWidget object.

OK, it sounds like we are referring to two different things.  My mind is
on the class code at the moment.

Dan


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

Daniel Sebald
In reply to this post by John W. Eaton
On 02/08/2018 12:05 PM, John W. Eaton wrote:

> On 02/08/2018 12:43 PM, Daniel J Sebald wrote:
>
>> Could we wait a week on this?  I'm right in the process of wrapping up
>> some changes to the variable editor that turn it into a QMainWindow
>> type of interface (as opposed to QTabWidget) and such a sweeping
>> change would make a mess for patches.  Also, I'm doing some
>> restructuring to make things more in line with signals/slots and that
>> might influence naming convention in the space.
>
> Are you working on any files outside of the variable editor?
>
> I realize that any changes like this may cause trouble for patches, but
> so can any changes, no matter what the purpose.
>
> jwe
>

linux@ ~/octave/octave/octave $ hg status
M libgui/src/main-window.h
M libgui/src/octave-dock-widget.cc
M libgui/src/octave-dock-widget.h
M libgui/src/variable-editor-model.cc
M libgui/src/variable-editor-model.h
M libgui/src/variable-editor.cc
M libgui/src/variable-editor.h

The main-window.h modification is a trivial change removing some header
files no longer required.

The change to octave-dock-widget.cc is for the purpose of reusing that
thin header portion of its window, i.e., that tiny

WindowTitle ........ FloatButton CloseButton

one sees in the main Octave GUI window.  In the variable editor I'm
making WindowTitle be the variable name.  With the added control, I can
then change the background color of the variable name to indicate which
sub-window is in focus.  So that's new

label_dock_widget

which in turn is inherited by

octave_dock_widget
variable_dock_widget

the latter also being new.  The end result is a really similar
look-and-feel for the main window and variable editor.  I could have
just had variable_dock_widget inherit octave_dock_widget, but
octave_dock_widget has all that code for saving settings and so on,
stuff not really needed by the variable editor (plus, it crashed when I
tried that, so...).

Dan


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

John W. Eaton
Administrator
In reply to this post by Daniel Sebald
On 02/08/2018 01:30 PM, Daniel J Sebald wrote:

> If the overall design is changed, just change
> QDockWidget to something else, and then one isn't constrained by the
> whole m_tab_widget pointer approach.

If you are concerned about that, then don't start by using QDockWidget.
Use a class name that you choose, derived from some Qt object.  Then
this code doesn't have to change at all even if you change the actual
implementation of the widget that contains the variable display.  For
what follows, I'll call this class "variable_editor_widget".

What is the type of m_main?  Is it a QMainWindow?

If instead, we also define that as our own class derived from
QMainWindow (or whatever, let's call it variable_editor_main_window)
then you could add a method to it like this:

   variable_editor_widget *
   variable_editor_main::find_variable (const QString& name)
   {
      return findChild<variable_editor_widget *> (name);
   }

I don't know.  Is that clearer?  I would rather see

   variable_editor_widget *vew = m_main->find_variable (name);

instead of

   QDockWidget *existing_qdw = m_main->findChild<QDockWidget *> (name);

With the former, it seems clear that the code is looking for a variable.
  All I can tell from the latter is that it is looking for a dockwidget
object and I don't know why.

OTOH, if this is only used in one place (inside variable_editor), then
maybe what I'm proposing above is adding an extra layer that is not
really needed.

Anyway, I agree that findChild is better than iterating over a list of
tabs.  I would just like to be sure that overloading the object name for
the purpose of finding variables won't cause confusion later.

jwe


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

Torsten-3
In reply to this post by John W. Eaton
On 08.02.2018 17:56, John W. Eaton wrote:
> We are also inconsistent in the names passed to QObject::setObjectName.
> For exmaple, we have things like HistoryDockWidget, Shortcut_Manager,
> variable_editor, OctaveTerminal (and TerminalDockWidget!).  What should
> these be?  Does changing the name affect anything else?

The object names are directly used in octave_dock_widget for saving the
widgets geometries. If we change names, special care has to be taken in
order to guarantee compatibility to existing setting files.

Torsten



-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

John W. Eaton
Administrator
On 02/08/2018 02:24 PM, Torsten wrote:

> The object names are directly used in octave_dock_widget for saving the
> widgets geometries. If we change names, special care has to be taken in
> order to guarantee compatibility to existing setting files.

OK, I see.  So maybe I'll leave those as is.  But I would like to
understand exactly how this works so I could know what needs to be done
to change one of these names.  For example, I made the attached change
and it wasn't enough to preserve the defaults.  See attached before and
after screenshots.  In both cases, I removed my
~/.config/octave/qt-settings file before starting Octave.

jwe




-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------

diffs.txt (2K) Download Attachment
before.png (171K) Download Attachment
after.png (195K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

Torsten-3
On 08.02.2018 21:15, John W. Eaton wrote:

> On 02/08/2018 02:24 PM, Torsten wrote:
>
>> The object names are directly used in octave_dock_widget for saving the
>> widgets geometries. If we change names, special care has to be taken in
>> order to guarantee compatibility to existing setting files.
>
> OK, I see.  So maybe I'll leave those as is.  But I would like to
> understand exactly how this works so I could know what needs to be done
> to change one of these names.  For example, I made the attached change
> and it wasn't enough to preserve the defaults.  See attached before and
> after screenshots.  In both cases, I removed my
> ~/.config/octave/qt-settings file before starting Octave.
>
> jwe
>

There is also the "windowState" in the [MainWindow], which stores the
dock positions of the widgets. The byte array also contains the names of
the widgets. After changing the names this window state must also be
updated. You should be able to copy it from your local settings file
after exiting octave.

Torsten




-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

Daniel Sebald
In reply to this post by John W. Eaton
On 02/08/2018 01:13 PM, John W. Eaton wrote:

> On 02/08/2018 01:30 PM, Daniel J Sebald wrote:
>
>> If the overall design is changed, just change QDockWidget to something
>> else, and then one isn't constrained by the whole m_tab_widget pointer
>> approach.
>
> If you are concerned about that, then don't start by using QDockWidget.
> Use a class name that you choose, derived from some Qt object.  Then
> this code doesn't have to change at all even if you change the actual
> implementation of the widget that contains the variable display.  For
> what follows, I'll call this class "variable_editor_widget".
>
> What is the type of m_main?  Is it a QMainWindow?

Yes, it's what was originally chosen.  I don't want to start from fresh,
i.e., a QObject, because I'm not (and I don't think anyone is) willing
to code the whole behavior of a window.  But QMainWindow has some nice
features.  Recall, it has a central widget and around the outside of the
central widget is dockable widgets that snap into place and line
everything up.  That's a nice behavior, and additionally it has the
ability to stack windows in a tabified format--so it is a combination of
tabs and multiple views.

By shrinking the central widget to nothing, all one sees are the docked
tables for various variables.  Works well enough.  [As a next possible
feature, I've made the central widget an QMdiArea.  MDI means the
widgets it contains can be arranged in any overlapping fashion, but then
one can tell the MDI to tile the windows.  Maybe some people would like
that behavior instead, so there could possibly be a Preference setting
for whether one wants the central-widget behavior or the MDI behavior.
That's getting too far ahead though without some smaller steps first.]


> If instead, we also define that as our own class derived from
> QMainWindow (or whatever, let's call it variable_editor_main_window)
> then you could add a method to it like this:
>
>    variable_editor_widget *
>    variable_editor_main::find_variable (const QString& name)
>    {
>       return findChild<variable_editor_widget *> (name);
>    }
>
> I don't know.  Is that clearer?  I would rather see

There's variation in these sorts of things.  Is it clearer?  Don't know,
it's more a case of getting in tune with the design.  What the above
does do is centralize the type of child, i.e., there wouldn't then be
variable_editor_widget in a template anywhere but here.  But if you
think of it, one would be coding

variable_editor_widget *mywidgetptr = mymainptr->find_variable(name);

so right there we are already using "variable_editor_widget" as the
return type so using

variable_editor_widget *mywidgetptr =
     mymainptr->findChild<variable_editor_widget *> (name);

isn't restricting things much more.  Could involve more assembly code,
perhaps.


>    variable_editor_widget *vew = m_main->find_variable (name);
>
> instead of
>
>    QDockWidget *existing_qdw = m_main->findChild<QDockWidget *> (name);
>
> With the former, it seems clear that the code is looking for a variable.
>   All I can tell from the latter is that it is looking for a dockwidget
> object and I don't know why.

I could change that to search for (more generally) QWidget or QObject or
(less generally) variable_dock_widget, which I think I do in a couple
other spots.  I started out more generally because I saw that--depending
on the contents of the variable--there could be a sub-editor opened and
I didn't know what type of object that would be.


> OTOH, if this is only used in one place (inside variable_editor), then
> maybe what I'm proposing above is adding an extra layer that is not
> really needed.
>
> Anyway, I agree that findChild is better than iterating over a list of
> tabs.  I would just like to be sure that overloading the object name for
> the purpose of finding variables won't cause confusion later.

I don't think it should be a problem.  The use of the object names is
resident to the m_main... yes, m_main is the Variable Editor main
window.  So, as you say, it is only looking at names for objects
contained within that window.  There could be objects somewhere else in
the GUI with possible variable names (e.g., in the qt_creator are
probably names like "label1", "label2", etc.) but so long as they aren't
contained within the Variable Editor, that's fine.  But the Variable
Editor is dynamically built and the routine is the one providing the
names.  There's even further control of telling findChildren() to only
look one level deep and not continue further, i.e., don't look for the
names of QLabel, QViewTable, QModel, etc.

Dan


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

Daniel Sebald
In reply to this post by Torsten-3
On 02/08/2018 01:24 PM, Torsten wrote:
> On 08.02.2018 17:56, John W. Eaton wrote:
>> We are also inconsistent in the names passed to QObject::setObjectName.
>> For exmaple, we have things like HistoryDockWidget, Shortcut_Manager,
>> variable_editor, OctaveTerminal (and TerminalDockWidget!).  What should
>> these be?  Does changing the name affect anything else?
>
> The object names are directly used in octave_dock_widget for saving the
> widgets geometries. If we change names, special care has to be taken in
> order to guarantee compatibility to existing setting files.

Yes, that's true as well.  The save/recall of settings code is very
generic and uses the names of the various dock widget windows (that's
the stuff I didn't want within the Variable Editor).  I think this is
the source of crashing I see when doing development on these windows at
launch time, but it corrects itself with a relaunch...not sure.

Dan


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: renaming some classes and using the octave namespace in the GUI

Daniel Sebald
In reply to this post by John W. Eaton
On 02/08/2018 02:15 PM, John W. Eaton wrote:

> On 02/08/2018 02:24 PM, Torsten wrote:
>
>> The object names are directly used in octave_dock_widget for saving the
>> widgets geometries. If we change names, special care has to be taken in
>> order to guarantee compatibility to existing setting files.
>
> OK, I see.  So maybe I'll leave those as is.  But I would like to
> understand exactly how this works so I could know what needs to be done
> to change one of these names.  For example, I made the attached change
> and it wasn't enough to preserve the defaults.  See attached before and
> after screenshots.  In both cases, I removed my
> ~/.config/octave/qt-settings file before starting Octave.

Here's an example of how settings are used:

// make the widget floating
void
octave_dock_widget::make_window (void)
{
   // the widget has to be reparented (parent = 0)

   QSettings *settings = resource_manager::get_settings ();

   // save the docking area and geometry for later redocking
   // FIXME: dockWidgetArea always returns 2
   settings->setValue ("DockWidgets/" + objectName () + "_dock_area",
                       m_parent->dockWidgetArea (this));
   settings->setValue ("DockWidgets/" + objectName (), saveGeometry ());
   settings->sync ();

Perhaps there are multiple things that need to be change in the settings
file regarding geometry, etc.

Looking at your screenshots, it reminds me about the titles of the
windows.  I sort of feel that "Window" of "Command Window" is extraneous
and takes up space where "Command" might be sufficient.  I can see why
someone wrote "Command Window" though, because we always say "command
line" or "command window" when speaking.  Also, there is "Editor" and
"Variable Editor"; perhaps now the former should be "File Editor".

Dan


-----------------------------------------
Join us March 12-15 at CERN near Geneva
Switzerland for OctConf 2018.  More info:
https://wiki.octave.org/OctConf_2018
-----------------------------------------