patchGNU Octave - Patches: patch #9993, Property inspector GUI

 
 

patch #9993: Property inspector GUI

Submitter:  Remi Thebault <rtbo>
Submitted:  Tue 10 Nov 2020 11:11:54 PM UTC
   
 
Category:  Core : new feature Priority:  5 - Normal
Status:  In Progress Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 10 Dec 2020 09:44:59 PM UTC, comment #24: 

In its current state, the patch can hang Octave or fail on errors about non existing objects, e.g. when running "demo text" and pressing Return repeatedly to run the next demo fast. This looks like threading issues that must be fixed prior to integrating the patch, and I don't currently have the time either to dig seriously and validate the current code. Maybe someone else will jump in and give his opinion.

>> May be it will be easier when the graphics toolkit code get correctly splitted from the GUI code (they are still tightly coupled by things like base_qobject and similar...)


AFAICS, jwe is currently restructuring the code base so that the GUI can be started from a cli session in a tty:

https://octave.discourse.group/t/new-command-window-widget/501

So I think we indeed better wait a bit.

Pantxo Diribarne <pantxo>
Group Member
Sun 06 Dec 2020 12:04:40 PM UTC, comment #23: 

I attached a new "all-in-one" patch with the addition of a "Description" column in the tree that maps the object to a descriptive property (when I could find one available).
I also added some reactivity in regards of the property changes.
I made a few tests by setting some props from the terminal, it reacts correctly in the PI tree and table.

My life being more busy now than a few weeks ago, I'll have difficulty to go further in a short period of time, so I would recommend to merge this before it goes out of sync with the main branch and eventually stales.

The 'inspect' command could be easy to implement from there by someone familiar with the QtHandle code. That could be me in a future patch.
May be it will be easier when the graphics toolkit code get correctly splitted from the GUI code (they are still tightly coupled by things like base_qobject and similar...)

(file #50409)

Remi Thebault <rtbo>
Sun 06 Dec 2020 10:13:45 AM UTC, comment #22: 


> * the inspector should be able to live without the GUI running, at least I'd be very happy if that happened :-). Since your inspector window inherits octave_dock_widget, that makes me suspect that the inspector won't run without the main window running. Am I wrong?


With an intermediate container, that should not be the main problem to solve. The main problem (for me at least), is that I have no clue how works the code in the QtHandles namespace.

> * we already have a mechanism to notify the graphics toolkit when graphics object properties change and at first sight you seem to have re-engineered another independent one. Maybe your approach is better (and we know the current mechanism is sometimes fragile), and it could be merged, but it would be better to not have multiple functions for the same task.


I agree with this. I didn't intend my code to work without the GUI, and it lives outside of the graphics toolkit.
I currently don't have a grasp on the QtHandles namespace code and how it connects to the interpreter.

BTW this makes me think that I currently don't listen to properties change (only to object creation/deletion). I will add this.

Remi Thebault <rtbo>
Thu 03 Dec 2020 09:58:10 PM UTC, comment #21: 

The patch got lost after a preview.

(file #50394)

Pantxo Diribarne <pantxo>
Group Member
Thu 03 Dec 2020 09:57:33 PM UTC, comment #20: 

I attached a new patch with binary data included.

What I meant is two-fold:

  • the inspector should be able to live without the GUI running, at least I'd be very happy if that happened :-). Since your inspector window inherits octave_dock_widget, that makes me suspect that the inspector won't run without the main window running. Am I wrong?
  • we already have a mechanism to notify the graphics toolkit when graphics object properties change and at first sight you seem to have re-engineered another independent one. Maybe your approach is better (and we know the current mechanism is sometimes fragile), and it could be merged, but it would be better to not have multiple functions for the same task.
Pantxo Diribarne <pantxo>
Group Member
Sun 29 Nov 2020 06:36:34 PM UTC, comment #19: 

Maybe it entails more than just moving code around?

I suppose what Pantxo means is a pane attached to a graphics window rather than the GUI, more or less like Matlab does (see attached pic).
BTW in Matlab, after docking the graphics windows the property inspector pane stays undocked.


Philip Nienhuis <philipnienhuis>
Group Member
Sun 29 Nov 2020 03:22:42 PM UTC, comment #18: 


> I merged all the patches into a single one for convenience (attached).

Thank you. There is a PNG icon somewhere in the diff (look for hsplitter-handle).
Unless I am missing something, the mercurial diff format do not include the binary content.
My patch-5 included it with the git diff format.

diff --git a/libgui/src/icons/hsplitter-handle.png b/libgui/src/icons/hsplitter-handle.png
new file mode 100644
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..3247c8d0a32ec0d2218c47458f2dab026310e61f
GIT binary patch
literal 147
zc%17D@N?(olHy`uVBq!ia0vp^tUzqe!3HFc+--0MQk(@Ik;M!Qe1}1p@p%4<6riAF
ziEBhjaDG}zd16s2LwR|*US?i)adKios$PCk`s{Z$Qb0uto-U3d7QM+C85uv$A2@KJ
p!B1FBBGhtKQOMI3US6S03^%JI+IDe-uK}uM@O1TaS?83{1OV(+EHnTB




> Could the inspector be untied from the main GUI?

If this means moving the code to libgui/graphics folder, yes that is easy.
I guess the "inspect" function would be trivial to implement from there.

Remi Thebault <rtbo>
Wed 25 Nov 2020 09:43:18 PM UTC, comment #17: 

With the patch.

(file #50344)

Pantxo Diribarne <pantxo>
Group Member
Wed 25 Nov 2020 09:42:45 PM UTC, comment #16: 

This indeed looks like a very useful piece of work. I merged all the patches into a single one for convenience (attached).

I didn't look at the implementation yet but I have a first remark: I am a terminal user and I'd definitely vote for having access to the inspector without the need of the GUI, trough the currently missing "inspect" function [1]:

https://www.mathworks.com/help/matlab/ref/propertyinspector.html;jsessionid=158c0e3dc5e515b88f0d6ecc55f8

Could the inspector be untied from the main GUI?

Maybe it could be integrated in the Qt graphics toolkit, where all the logic of graphics property updates is already in place?  
If so take a look at the discussion here:

https://octave.1599824.n4.nabble.com/Removing-init-qt-dependency-on-libgui-td4698367.html

Pantxo Diribarne <pantxo>
Group Member
Tue 24 Nov 2020 08:09:11 AM UTC, comment #15: 

I'm not sure what you mean with 'Handle input' ?

Some mapping in the list to the actual entity in the plot would be a big help for many users (incl. me).

As to highlighting properties/entities in the figure - the other way round, clicking on something in the figure and opening the P.I. with focus on the right spot would be at least as useful.

My point about complexity is that graphics in Octave is usually quite slow [*]. This property inspector gives a hint as to why: the insanely big number of properties and children for even a simple XY plot :-)

[*] W.r.t. graphics: about the only thing I know of where Octave is faster than Matlab is ginput() - can take ages on Matlab  8-:

BTW, cc'ing Pantxo, the Octave graphics expert around here, I expect him to like this patch and he may have valuable suggestions.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 23 Nov 2020 12:14:31 PM UTC, comment #14: 

Yes, patch 5 is on top of 4.
I did a hg pull between 4 and 5. Although I was careful not to merge what I pulled into my changes, it may have an impact on the patch application. In addition, patch 5 is made in the git format because there's a binary file (splitter handle image).
May be you need to apply with a git option? I'm not familiar with Mercurial.

I don't see the handle image on your screenshot, may be it is from patch-4 level?

Regarding your last remark, I had the same impression of complexity. I was thinking to add in the tree a column with text that identifies the object. For this I need to map each type of object with a property that is relevant for the user. (e.g. the figure title, the axes title, the menu text...)
For some objects there won't be anything to display, but it may help to navigate in this tree.

A step further is to have the graphics objects selected/highlighted in the figure itself. This goes way beyond the property inspector. There is a lot to do on this side! (selecting, moving, editing objects in the figure)

Remi Thebault <rtbo>
Sun 22 Nov 2020 09:26:27 PM UTC, comment #13: 

Just tried 'on top of'.
I got a few failed hunks in patch 4 & 5, probably by csets by Torsten for the property inspector.
Not too difficult to fix manually except for settings-dialog.ui, always a tricky thing I - really don't know what to do there so I just guessed.
It may be better to wrap up all present + future patches into one, relative to the very latest dev tip.

Anyway on Linux it works fine; building for window now but I suppose that'll go & run well.
An undocked P.I. pane survives a restart now and there's a nice prefs tab. I agree fonts maybe a bit too much there.
The shortcuts don't work but they don't work for any pane here (or I don't understand how they're supposed to work).

Right, next job for me is to find my way trough all the properties. In the attached example pic I see three nested 'uimenu' series for a simple 'sin (1:02:9)' plot and two 'line' children.
It all looks a bit excessively complicated to me, I have to find where the properties are located that matter to me.



Philip Nienhuis <philipnienhuis>
Group Member
Sun 22 Nov 2020 07:40:54 PM UTC, comment #12: 

Thanks Remy, you're quick!

patch #5 on top of patch #4 or instead?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 22 Nov 2020 04:18:11 PM UTC, comment #11: 

Thanks for your detailed review.

Attached patch-5 which should address all your remarks.

The settings includes row height and alternating colors.
I'm a bit reluctant to add font like the variable editor did.
IMO this should be more global settings and not per widget.

I also save in the settings the columns sizes and splitter position (but this doesn't show in the settings dialog).

I also added an icon to make the splitter handle visible.
This makes more obvious to the user that the panes can be resized.

(file #50314)

Remi Thebault <rtbo>
Sun 22 Nov 2020 12:19:56 AM UTC, comment #10: 

FTR, this afternoon I made a Windows build incl. patch #3; works nice.
I'll try patch #4 later this weekend.

Some notes:

  • I undocked the P.I. and closed the pane. After restarting Octave and setting visibility of the P.I. to "on" in the main menu it doesn't show up at all. "Reset Default Window Layout" in the main menu gets it back. So restoring an undocked P.I. across Otave sessions needs a bit of attention.
  • There are no shortcut for the P.I. pane (I'd suppose it could be Ctrl-7 & Ctrl-Shift-7).
  • There's no tab in the Preferences pane for the property inspector.
  • Maybe row height can be made a little lower? (that's what I was searching in the Preferences pane).


Philip Nienhuis <philipnienhuis>
Group Member
Sat 21 Nov 2020 09:41:40 PM UTC, comment #9: 

Adding patch-4 that adds to the properties table a column with hyperlinks to the internet documentation.

Remi Thebault <rtbo>
Fri 20 Nov 2020 08:51:56 PM UTC, comment #8: 


> In the mean time buiding a Windows binary failed.

This seems to be me introducing a HANDLE enum symbol, which is already defined as an opaque pointer on Windows.
I think it is the same for BOOL and CALLBACK.

The 3rd patch should fix this. (I can't build on Windows myself to check)

Remi Thebault <rtbo>
Fri 20 Nov 2020 08:48:10 PM UTC, comment #7: 
Remi Thebault <rtbo>
Wed 18 Nov 2020 11:12:24 PM UTC, comment #6: 

Works nicely in Linux (building for Windows now).
I never realized that a simple plot of sin(x) has so many uimenus.

After playing around with it I think this is a very useful tool. I'm often struggling to find out what property belongs to which handle/child and then how to adapt it. This tool presents a nice overview. Thanks for creating this.

In the mean time buiding a Windows binary failed, partial log attached. As it compiles fine on Linux I suppose this is an mxe-octave issue.


(file #50301)

Philip Nienhuis <philipnienhuis>
Group Member
Wed 18 Nov 2020 09:40:30 PM UTC, comment #5: 

Now that's a simple patch :-)
I'll try asap.

Thanks for explaining the get_properties thing, good. I probably got triggered by the long *.ui listing and the svg colors.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 17 Nov 2020 08:54:46 PM UTC, comment #4: 

Attached patch-2 which should work at least up to Qt-5.12 (I didn't check older).
5.12 is the last LTS version (before 5.15 released this year).

The second patch applies over the first one. I'm very new to Octave development so I'm not sure of how you review the code, but it seems easier that way. I can collapse everything together at your convenience.

> Looking at the patch, have you hard-coded all properties for all objects?

There is no hard-coding. My code uses

graphics_object::get_properties ().all_property_names ()

in the table model constructor to get the properties to display.
Then each property is displayed/edited according its type.

Similarly, the tree uses the parent and children properties and it doesn't need to know anything about the graphics objects.

So this is very generic already.

Remi Thebault <rtbo>
Sun 15 Nov 2020 08:44:48 PM UTC, comment #3: 

Qt is at 15.5 in mxe-octave so it should build there for Windows installers.
However, to get there I'd first have to build an Octave dist archive in Linux (Mageia 7) where Qt is still at 5.12.
So there's a bit of a deadlock ... :-) and I don't know how to fool mxe-octave to accept a patched dist archive.

Looking at the patch, have you hard-coded all properties for all objects?
I wonder (just dreaming) if there can be an algorithm that explores the source code, or maybe later on the symbol table or so, for objects and their properties and then displays them with general/generic display tools. That way future expansion to e.g., classdef objects could come in sight.
Sooner or later Octave will have to move from handle-based to object based graphics, such a development cold be very useful then.

For now I think your patch could be useful enough. I'm very curious ...

Philip Nienhuis <philipnienhuis>
Group Member
Sun 15 Nov 2020 03:23:38 PM UTC, comment #2: 

Thanks for the patch, but same issue for me as in comment #1. shrunkBy was introduced in Qt 5.14. and may used platforms to build oct() ave have older Qt versions installed. Could you please replace this code line by a workaround that works with older Qt 5 versions?

Torsten Lilge <ttl>
Group Member
Sun 15 Nov 2020 02:08:09 PM UTC, comment #1: 

I tried your patch with dev octave. It applied cleanly, then I reran configure, but when building I got:

:
  CXX      libinterp/dldfcn/__init_fltk___la-__init_fltk__.lo
../dev_p/libgui/src/property-inspector-table.cc: In member function ‘void octave::color_editor_widget::set_rgb(const QColor&, octave::color_editor_widget::rgb_source)’:
../dev_p/libgui/src/property-inspector-table.cc:1373:42: error: ‘class QSize’ has no member named ‘shrunkBy’
     const auto sz = ui->rgbIcon->size ().shrunkBy (QMargins (4, 4, 4, 4));
                                          ^~~~~~~~


Philip Nienhuis <philipnienhuis>
Group Member
Tue 10 Nov 2020 11:11:54 PM UTC, original submission:  

Initial implementation of Property Inspector


HIGHLIGHTS

  * a widget with two main views
    - a tree to display and interact with graphics object tree
    - a table, displaying the properties of the object selected in the
      tree

  * the tree receive notifications of graphics object creation/deletion
    to update itself

  * the table provides display and edition of most types of properties


SHORTCOMINGS

  * Not all property types have yet proper editors/painting

  * Updating properties does not always reflect in the figures.
    - updating line color for example not always trigger redraw.
    - when redraw happens, all previous property editions are visible

  * No link to help is provided. 2 possible strategies:
    - compute URL to online help from handle type and property name
    - embed the help in the executable. I have no clue how to do this.


DETAILED CHANGELOG

  * liboctave/util/oct-mutex.h
    - adding unique_lock template class that encapsulate a critical resource
      providing lock/unlock methods. unique_lock is moveable but non-copiable
      and also acts like a smart pointer.
      - not a strictly needed addition, but is used to provide safely the
        gh_manager instance to the GUI

  * libinterp/corefcn/graphics.in.h
    - adding some introspection of graphics properties
      - prop_type() method (returning new property_type enum)
      - as_base_property() method to retrieve a pointer that can be casted
      - as_[type]_property() that dynamic_cast the base property to derived
        type (not used - compiled out at the moment)

  * libinterp/corefcn/event-manager.h
    - adding lifetime events of graphics objects in the interpreter events
      interface. Needed to track creation and removal of graphics objects in
      the tree.

  * libinterp/corefcn/graphics.cc
   - fire the graphics objects lifetime events

  * libui/src/qt-interpreter-events.h
  * libui/src/qt-interpreter-events.cc
    - adding signals for the new graphics objects

  * libgui/src/interpreter-qobject.h
  * libgui/src/interpreter-qobject.cc
    - provide access to the gh_manager through a unique_lock
    - the GUI cannot use the gh_manager without locking it
    - Not very useful today because the gh_manager is not consistently locked
      in the interpreter thread (e.g. when setting/reading properties).
      This could lead to race conditions (although I didn't observe any).
    - May be a better strategy is to have fine grain (per property) locking.

  * libgui/src/property-inspector.h
  * libgui/src/property-inspector.cc
    - implementation of the property inspector
      - simple widget that instantiate the more complex views and models and
        make them talk together

  * libgui/src/property-inspector-tree.h
  * libgui/src/property-inspector-tree.cc
    - implementation of the graphics handle tree model
    - quite straightforward Qt tree model code

  * libgui/src/property-inspector-table.h
  * libgui/src/property-inspector-table.cc
    - implementation of the graphics properties table model and item delegate.
    - to keep code flexible, I tried to take the best decisions based on the
      octave_value of the property rather than based on the derived property
      type, but this is not always possible. (hence the properties
      introspection)
    - there (at the moment) two specific widgets used as cell editors:
      - matrix_widget
      - color_editor_widget (this one use a *.ui designer file)

  * libgui/src/property-inspector-color-editor.ui
    - Qt Designer file for the color_editor_widget

  * libgui/src/main-window.h
  * libgui/src/main-window.cc
  * libgui/src/gui-preferences-sc.h
    - instantiate the property inspector and setup shortcuts

  * libgui/src/module.mk
    - adding new property inspector files

Remi Thebault <rtbo>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #50409:  patch9993_3.patch added by rtbo (205KiB - text/x-patch)
file #50394:  patch9993_2.patch added by pantxo (200KiB - text/x-patch)
file #50344:  patch9993.patch added by pantxo (201KiB - text/x-patch)
file #50314:  property-inspector-5.patch added by rtbo (89KiB - text/x-patch)
file #50312:  property-inspector-4.patch added by rtbo (24KiB - text/x-patch)
file #50311:  property-inspector-3.patch added by rtbo (10KiB - text/x-patch)
file #50290:  property-inspector-2.patch added by rtbo (775B - text/x-patch)
file #50239:  property-inspector-1.patch added by rtbo (120KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by rtbo (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 20 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-12-06 rtbo Attached File- Added patch9993_3.patch, #50409
    2020-12-03 pantxo Attached File- Added patch9993_2.patch, #50394
    2020-11-29 philipnienhuis Attached File- Added ML_PropertyEditor.PNG, #50363
    2020-11-25 philipnienhuis Carbon-Copy- Added philipnienhuis
    2020-11-25 pantxo Attached File- Added patch9993.patch, #50344
    2020-11-25 pantxo CategoryNone Core : new feature
    2020-11-24 philipnienhuis StatusNeed Info In Progress
    2020-11-24 philipnienhuis Carbon-Copy- Added pantxo
    2020-11-22 philipnienhuis Attached File- Added PropertyInspector_for_sin(1:0.2:9).png, #50315
    2020-11-22 rtbo Attached File- Added property-inspector-5.patch, #50314
    2020-11-21 rtbo Attached File- Added property-inspector-4.patch, #50312
    2020-11-20 rtbo Attached File- Added property-inspector-3.patch, #50311
    2020-11-18 philipnienhuis Attached File- Added default-octave_part.txt, #50301
    2020-11-17 rtbo Attached File- Added property-inspector-2.patch, #50290
    2020-11-15 philipnienhuis StatusNone Need Info
    2020-11-10 rtbo Attached File- Added property-inspector-1-radio-edition.png, #50241
        Attached File- Added property-inspector-1-array-edition.png, #50242
        Attached File- Added property-inspector-1-color-edition.png, #50243
    2020-11-10 rtbo Attached File- Added property-inspector-1-general-view.png, #50240
    2020-11-10 rtbo Attached File- Added property-inspector-1.patch, #50239

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code