bugGNU Octave - Bugs: bug #55795, Default Qt figure toolbar should...

 
 

bug #55795: Default Qt figure toolbar should use an uitoolbar

Submitted by:  Pantxo Diribarne <pantxo>
Submitted on:  Thu 28 Feb 2019 08:06:24 AM UTC  
 
Category:  Plotting with OpenGL Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Wed 13 Mar 2019 11:53:53 PM UTC, comment #22:

Using the profiler, I find that 31 calls to figure take 2.193 seconds, and that _add_default_menu_ is taking 1.942 seconds or 88.5% of the time. Here are the details

plot: 31 calls, 3.397 total, 0.013 self
newplot: 31 calls, 3.095 total, 0.016 self
figure: 31 calls, 2.193 total, 0.014 self
__add_default_menu__: 31 calls, 1.942 total, 0.067 self
1) uimenu: 713 calls, 1.428 total, 0.055 self
2) uitoggletool: 155 calls, 0.211 total, 0.012 self
3) uipushtool: 93 calls, 0.162 total, 0.008 self
4) addlistener: 124 calls, 0.026 total, 0.026 self
5) uitoolbar: 31 calls, 0.020 total, 0.002 self
6) set: 155 calls, 0.009 total, 0.009 self
7) __add_default_menu__>toggle_visibility_cb: 31 calls, 0.007 total, 0.003 self
8) __add_default_menu__>init_mouse_tools: 31 calls, 0.006 total, 0.002 self

Looking at uimenu, since it is the largest contributor to the total, I find

uimenu: 713 calls, 1.428 total, 0.055 self
1) __go_uimenu__: 713 calls, 1.214 total, 1.214 self
2) __uiobject_split_args__: 713 calls, 0.157 total, 0.108 self
3) binary >: 713 calls, 0.001 total, 0.001 self
4) nargout: 713 calls, 0.001 total, 0.001 self

Seems like a lot of the time is taken in the C++ code. Still, since this is not a user-function, we could have _add_default_menu_ call _go_uimenu_ directly and avoid the overhead of function calling in the interpreter as well as the input validation. That looks like it would save (.157 + .055) / 1.428 or 14.8%.

Rik <rik5>
Project Administrator
Wed 13 Mar 2019 11:33:11 PM UTC, comment #21:

On a performance note, I did

tic; plot (1:10); toc

v5.1.0 : 109 ms
v6.0.0 : 225 ms

Basically, a doubling. Is there a way we could improve this?

Maybe _add_default_menu_.m could make the menubar and toolbar invisible, then construct the objects, and then call the toolbar visibility function at the end?

I did that and it seemed to help quite a bit

v6.0.0 : 104 ms

I pushed it here (https://hg.savannah.gnu.org/hgweb/octave/rev/d7856bf83781).

Interestingly, making the uimenu object invisible during the construction of the figure did not help, and actually hurt benchmark performance just a bit.

Rik <rik5>
Project Administrator
Wed 13 Mar 2019 09:58:25 PM UTC, comment #20:

I pushed the patch here https://hg.savannah.gnu.org/hgweb/octave/rev/cb5c1ea2062c.

The only change was adding missing semicolons to the ends of some statements in _add_default_menu_.m. I'm not sure why they were missing,and I'm not sure they were even in this changeset. They may have existed before but its been a long time since anyone looked over the file.

Rik <rik5>
Project Administrator
Wed 13 Mar 2019 05:57:20 PM UTC, comment #19:

The attached patch fixes the issue for me.

(file #46520)

Pantxo Diribarne <pantxo>
Project Member
Tue 12 Mar 2019 06:07:41 PM UTC, comment #18:

Okay, I didn't realize that the status bar was also being made invisible when the toolbar disappeared.

Rik <rik5>
Project Administrator
Tue 12 Mar 2019 06:03:58 PM UTC, comment #17:

@Rik: I am working on a fix, but it's a little more complicated than for menubar, since we have to sync the display of the toolbar and the status bar in C++.

Pantxo Diribarne <pantxo>
Project Member
Tue 12 Mar 2019 04:26:26 PM UTC, comment #16:

There is a similar issue for the "menubar" property. In _add_default_menu_.m this is handled by adding a listener on the "menubar" property which invokes a callback

## Add/Restore figure listeners
toggle_visibility_cb (hf, [], hmenu);
addlistener (hf, "menubar", {@toggle_visibility_cb, hmenu});

where toggle_visibility_cb is

function toggle_visibility_cb (hf, ~, hmenu)
if (strcmp (get (hf, "menubar"), "none"))
set (hmenu, "visible", "off")
else
set (hmenu, "visible", "on")
endif
endfunction

Maybe the simplest thing is to set up a similar callback for the "toolbar" property. The toggle_visibility_cb for menubar would also need to be extended to call the toolbar_visibility_cb, or a second listener could be added to the "menubar" property.

Rik <rik5>
Project Administrator
Mon 11 Mar 2019 06:57:31 PM UTC, comment #15:

Re-opening for the one minor regression.

This code does work

h = figure ();
set (h, "toolbar", "none");

So there is only a problem when the figure property is set during the construction of the figure object. Note that the property is correctly set.

h = figure ("toolbar", "none")
get (h, "toolbar")
ans = none

Rik <rik5>
Project Administrator
Mon 11 Mar 2019 09:59:40 AM UTC, comment #14:

Very happy to see this happening, thanks!

I notice one thing: the figure toolbar is still displayed with the following call:

figure ("ToolBar", "none");

Guillaume <gyom>
Sun 10 Mar 2019 04:55:01 AM UTC, comment #13:

Looks good to me. I checked in the patch here (https://hg.savannah.gnu.org/hgweb/octave/rev/992f55ef87f5).

Marking bug as fixed and closing report.

Rik <rik5>
Project Administrator
Fri 08 Mar 2019 12:33:30 PM UTC, comment #12:

I attached an updated patch that includes both fixes from comment #8 (that I had to redo since your patch did not include binary data (use --git)) and comment #9 (for which I had to update the hot spot).

(file #46465)

Pantxo Diribarne <pantxo>
Project Member
Fri 08 Mar 2019 08:29:28 AM UTC, comment #11:

@Rik:

>> Just for fun, I tried setting the cursors to be 24x24 which matches the toolbar. At least on my system, this works just fine.


Indeed it also works for me. The best way is probably to push it and wait for Windows and Mac builds to see if it also works on those platforms.

>> I don't know if this is intentional, but figure-axes.svg and figure-grid.svg are 32x32, while the other figure-XXX.svg are 48x48.


The former two are my artwork :-) while the others are from the Tango icon set. The size of the page in the original SVG does not matter: you can export a vector drawing to whatever raster size you like. But if you prefer, I can resize them to be 48-by-48 as the others.

I'll finish the patch and push it by the end of the week.

Pantxo Diribarne <pantxo>
Project Member
Fri 08 Mar 2019 01:05:49 AM UTC, comment #10:

I don't know if this is intentional, but figure-axes.svg and figure-grid.svg are 32x32, while the other figure-XXX.svg are 48x48.

Rik <rik5>
Project Administrator
Fri 08 Mar 2019 01:01:00 AM UTC, comment #9:

Just for fun, I tried setting the cursors to be 24x24 which matches the toolbar. At least on my system, this works just fine. Attached diff if you want to try it out.

(file #46459)

Rik <rik5>
Project Administrator
Fri 08 Mar 2019 12:48:41 AM UTC, comment #8:

I made a few small changes. I eliminated some blank lines with trailing spaces, I removed a file from module.mk that actually isn't there, and I removed an extra separator at the start of the toolbar.

diff -r b776f4e2afab libgui/graphics/QtHandlesUtils.cc
--- a/libgui/graphics/QtHandlesUtils.cc Thu Mar 07 22:53:33 2019 +0100
+++ b/libgui/graphics/QtHandlesUtils.cc Thu Mar 07 16:39:08 2019 -0800
@@ -283,7 +283,7 @@ namespace QtHandles
makeImageFromCData (const octave_value& v, int width, int height)
{
dim_vector dv (v.dims ());
-
+
if (dv.ndims () == 3 && dv(2) == 3)
{
int w = qMin (dv(1), static_cast<octave_idx_type> (width));
@@ -306,7 +306,7 @@ namespace QtHandles
int g = d(j, i, 1);
int b = d(j, i, 2);
int a = 255;
-
+
img.setPixel (x_off + i, y_off + j, qRgba (r, g, b, a));
}
}
diff -r b776f4e2afab libgui/graphics/ToolBar.cc
--- a/libgui/graphics/ToolBar.cc Thu Mar 07 22:53:33 2019 +0100
+++ b/libgui/graphics/ToolBar.cc Thu Mar 07 16:39:08 2019 -0800
@@ -84,7 +84,6 @@ namespace QtHandles
{
uitoolbar::properties& tp = properties<uitoolbar> ();

-
bar->setFloatable (false);
bar->setMovable (false);
bar->setVisible (tp.is_visible ());
diff -r b776f4e2afab libgui/graphics/module.mk
--- a/libgui/graphics/module.mk Thu Mar 07 22:53:33 2019 +0100
+++ b/libgui/graphics/module.mk Thu Mar 07 16:39:08 2019 -0800
@@ -195,7 +195,6 @@ libgui_EXTRA_DIST += \
%reldir%/images/pan.png \
%reldir%/images/rotate.png \
%reldir%/images/select.png \
- %reldir%/images/text.png \
%reldir%/images/zoom-in.png \
%reldir%/images/zoom-out.png \
$(__init_qt___UI)
diff -r b776f4e2afab scripts/plot/util/private/__add_default_menu__.m
--- a/scripts/plot/util/private/__add_default_menu__.m Thu Mar 07 22:53:33 2019 +0100
+++ b/scripts/plot/util/private/__add_default_menu__.m Thu Mar 07 16:39:08 2019 -0800
@@ -99,7 +99,7 @@ function __add_default_menu__ (hf, hmenu

ht(1) = uitoggletool (htb, "tooltipstring", "Pan", ...
"tag", "__default_button_pan__", ...
- "__named_icon__", "figure-pan", "separator", "on");
+ "__named_icon__", "figure-pan");
ht(2) = uitoggletool (htb, "tooltipstring", "Rotate", ...
"tag", "__default_button_rotate__", ...
"__named_icon__", "figure-rotate");

The new patch is uploaded.

My sense is that this is probably good enough to get checked in and then resolve the remaining issues as they are exposed.

It would be good to have a UI expert comment, but I think we might need to add a pointer icon for going back to regular click mode with no tools selected. You can do this by clicking on the tool that is currently selected which toggles it, but I don't think that is how many people think.

Also, it seems from the code that we were not using 32x32 images in the past. It might be recommended by Qt, but I wonder how important it really is. We haven't gotten any complaints previously.

(file #46458)

Rik <rik5>
Project Administrator
Thu 07 Mar 2019 10:22:23 PM UTC, comment #7:

I attached an updated (but still WIP) version of the patch in which I removed the File menu related actions. They indeed take much space.

As for the new icons, I tried to improve their readability. In any event, I wouldn't like to come back to Crystal icons since we use the Tango theme everywhere in the GUI.

Now for the cursors, yes, I also find that they're too big, but they need to be 32 pixel wide (see https://doc.qt.io/qt-5/qcursor.html#QCursor-2). Maybe we could ship a set of down-scaled versions (with a larger margin)?

(file #46457)

Pantxo Diribarne <pantxo>
Project Member
Wed 06 Mar 2019 09:58:50 PM UTC, comment #6:

Rik,
What I meant is that people using Matlab r2018b+ code in Octave may hit code snippets intended to deal with the offending toolbar behavior in Matlab. So Octave (-users) may need to catch that.

Philip Nienhuis <philipnienhuis>
Project Member
Wed 06 Mar 2019 04:51:01 PM UTC, comment #5:

@Philip: I think we're fine to create a UX/UI that is pleasing to us. Octave seeks to be identical to Matlab where that would have an impact on running code. For example, the sin function in Octave assumes that the input is in radians, not degrees, because this is what Matlab also assumes. But the choice of which icons to include in the toolbar is not something that would impact running user code. Hence, if we can offer a better user experience maybe we get a few more converts.

The first step, which is important, is creating the toolbar as a uitoolbar object, rather than being hardcoded in C++. This happens to be Matlab-compatible, and then our choice of what to fill it with will be up to us.

Rik <rik5>
Project Administrator
Wed 06 Mar 2019 12:34:47 PM UTC, comment #4:

@Rik comment #3
The comments in the referenced thread may be fun (indeed), but I'm a bit wary of when Octave will be "forced" to go a similar route, or at least cope with the newly introduced code to restore the old Matlab behavior. After all, Octave wants to be Matlab-compatible and that includes the code snippets in the thread. Or am I wrong there?

I very much second the notion that look and feel of SW is extremely important for user satisfaction and thus acceptance of Octave.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 05 Mar 2019 01:24:48 AM UTC, comment #3:

This is a good idea. Octave has needed to convert the toolbar and menus over to standard uiXXX tools for a while.

Things to do

1) Add commit message to patch.
2) Do we need the File menu activities "New Figure", "Open Figure", "Save", "Save As"? Personally, if I require that sort of thing I go to the File menu. I like the existing more minimal toolbar which only has tools for interacting with the plot.
3) The Zoom In, Zoom Out, and Autoscale icons are nearly impossible for me to read. I attached a screen snapshot which should be viewed at 100% magnification in your image viewer to see what I mean. The '+', '-', and '1' are very difficult to make out.
4) In terms of ordering, I would prefer to have Pan and Rotate at the very left, followed by the Zoom operations.
5) The new Text icon which sort of looks like a matrix with "a,b,c" isn't as immediately obvious to me that it is for inserting text as the old one which had "T+". Can we find an icon like the old one?
6) The new Rotate icon is confusing to me. Is there a different choice we could use.
7) The size of the icons in the toolbar seems about right. When you select a tool and then move the mouse over the Canvas, the cursor changes to the tool shape, but it is bigger than it needs to me. The cursor seems unappealingly large. Maybe this is something I'm supposed to change in my Window Manager settings, I'm not sure.

For fun, take a look at this thread about Matlab's 2018b release where they took away the toolbar (https://www.mathworks.com/matlabcentral/answers/419036-what-happened-to-the-figure-toolbar-in-r2018b-why-is-it-an-axes-toolbar-how-can-i-put-the-buttons). Users are really, really unhappy with the change. It pays to get the UX/UI experience right.

Rik <rik5>
Project Administrator
Sat 02 Mar 2019 10:18:42 PM UTC, comment #2:

Another update, that fixes some missing file (text.png) in the previous patch.

(file #46393)

Pantxo Diribarne <pantxo>
Project Member
Thu 28 Feb 2019 08:15:25 AM UTC, comment #1:

Sorry the previous patch does not contain binary png data. I attached an updated one.

(file #46372)

Pantxo Diribarne <pantxo>
Project Member
Thu 28 Feb 2019 08:06:24 AM UTC, original submission:

This has already been discussed in the past and is a known incompatibility between Octave's Qt toolkit and Matlab. The current figure toolbar is implemented in C++ and as such it is not only incompatible with Matlab, but also not handy when it comes to modifying it.

I attached a preliminary cset that switches to uitoolbar/uitoggletool/uipushtool for the implementation of the default toolbar. ATM there are no significant differences with the current toolbar, just a few additions:

  • New button section for New, Open, Save and Save as,
  • Use standard icons from the (linux) desktop when available. For this I added a new "__named_icon__" property to ui(toggle/push)tool, so that we don't have to store cdata resources in each button.

I have added some Tango icons for zoom-in/out/original and designed new ones for pan and rotate (which I am not so proud about, don't hesitate to rework).

Pantxo Diribarne <pantxo>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #46520:  toolbar_none.patch added by pantxo (7KiB - text/x-patch)
file #46465:  qt_use_uitoolbar10.patch added by pantxo (261KiB - text/x-patch)
file #46459:  24px_icons.diff added by rik5 (1KiB - text/x-patch)
file #46458:  qt_use_uitoolbar9.patch added by rik5 (242KiB - text/x-patch)
file #46457:  qt_use_uitoolbar8.patch added by pantxo (258KiB - text/x-patch)
file #46393:  qt_use_uitoolbar6.patch added by pantxo (262KiB - text/x-patch)
file #46372:  qt_use_uitoolbar5.patch added by pantxo (262KiB - text/x-patch)
file #46371:  qt_use_uitoolbar4.patch added by pantxo (247KiB - 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 gyom (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by pantxo (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-13 rik5 StatusPatch Submitted => Fixed
        Open/ClosedOpen => Closed
    2019-03-13 pantxo Attached File- => Added toolbar_none.patch, #46520
        StatusIn Progress => Patch Submitted
    2019-03-11 rik5 StatusFixed => In Progress
        Open/ClosedClosed => Open
    2019-03-10 rik5 StatusPatch Reviewed => Fixed
        Open/ClosedOpen => Closed
    2019-03-08 pantxo Attached File- => Added qt_use_uitoolbar10.patch, #46465
    2019-03-08 rik5 Attached File- => Added 24px_icons.diff, #46459
    2019-03-08 rik5 Attached File- => Added qt_use_uitoolbar9.patch, #46458
    2019-03-07 pantxo Attached File- => Added qt_use_uitoolbar8.patch, #46457
    2019-03-05 rik5 StatusNone => Patch Reviewed
    2019-03-02 pantxo Attached File- => Added qt_use_uitoolbar6.patch, #46393
    2019-02-28 pantxo Attached File- => Added qt_use_uitoolbar5.patch, #46372
    2019-02-28 pantxo Attached File- => Added qt_use_uitoolbar4.patch, #46371

    Back to the top


    Powered by Savane 3.4