bugGNU Octave - Bugs: bug #58144, "Cdata" icon for...

 
 

bug #58144: "Cdata" icon for UIcontrol is incorrectly sized

Submitter:  Yonggon Lee <yonggonlee>
Submitted:  Wed 08 Apr 2020 10:35:58 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Inaccurate Result
Status:  Fixed Assigned to:  None
Originator Name:  Yonggon Lee Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 22 Apr 2020 12:08:50 AM UTC, comment #16: 

I like the appearance better with 24x24 scaling so I made that change here https://hg.savannah.gnu.org/hgweb/octave/rev/9a40d2b40db8.

I think this bug has been resolved at this point so I'm marking as Fixed and closing report.

Rik <rik5>
Group administrator
Tue 21 Apr 2020 07:18:31 PM UTC, comment #15: 

I don't know if there are other drawbacks but, with 16x16 icons, clipping at 24 instead of 32 make things better looking.
In the attached image, first row is Octave 5.2, then the other rows are clipping at 32, clipping at 24 and Matlab. Left is a default figure and right is a tailored figure with 16x16 icons.


Guillaume <gyom>
Mon 20 Apr 2020 05:07:17 PM UTC, comment #14: 

I don't know which way is better, but changing clipping to 24x24 from 32x32 is technically quite easy.  I've attached a patch which does that.


diff -r 49384057fb03 libgui/graphics/ToolBarButton.cc
--- a/libgui/graphics/ToolBarButton.cc  Mon Apr 20 08:28:22 2020 -0700
+++ b/libgui/graphics/ToolBarButton.cc  Mon Apr 20 10:03:28 2020 -0700
@@ -49,7 +49,7 @@ namespace QtHandles
     action->setVisible (tp.is_visible ());

     // Get the icon data from cdata or as a named icon
-    QImage img = Utils::makeImageFromCData (tp.get_cdata (), 32, 32);
+    QImage img = Utils::makeImageFromCData (tp.get_cdata (), 24, 24);

     if (img.width () == 0)
       {
@@ -104,7 +104,7 @@ namespace QtHandles
       case T::properties::ID_CDATA:
         {
           // Get the icon data from cdata or as a named icon
-          QImage img = Utils::makeImageFromCData (tp.get_cdata (), 32, 32);
+          QImage img = Utils::makeImageFromCData (tp.get_cdata (), 24, 24);

           if (img.width () == 0)
             {


The visual difference is shown in the attached file Octave_24x24_clipping.png.



(file #48886,

Rik <rik5>
Group administrator
Sun 19 Apr 2020 01:11:34 PM UTC, comment #13: 

Thank you. This is perhaps a situation where Octave and Matlab can behave slightly differently but my concern is that if one uses 16x16 icons, they appear quite small because of the extra margins. See attachment with Matlab on top and Octave at the bottom.


Guillaume <gyom>
Sun 19 Apr 2020 12:29:11 AM UTC, comment #12: 

The icon size can stay the same, but I do think it is more reasonable--when there is a size mismatch between CDATA and the icon--to take the data from the center of the CDATA image.

I implemented that change here: https://hg.savannah.gnu.org/hgweb/octave/rev/f3f29e1801fc.

The result is shown in the file uitoggle_clipping_2.png.

Already marked as Ready for Test.


Rik <rik5>
Group administrator
Sat 18 Apr 2020 09:14:40 PM UTC, comment #11: 

In fact I like Octave's behavior for uitoolbar buttons much better than Matlab's. What are you trying to do, avoid the margins?

Matlab specifically recommends using 16-by-16 images (the second in your example), to keep margins safe. This is probably because otherwise this ugly uncontrolled clipping behavior happens.

In Octave you can feed cdata up to 32-by-32 and then the image is gracefully clipped. So my recommendation would thus be to just document this, and eventually limit the possible size of cdata for those objects.

Pantxo Diribarne <pantxo>
Group Member
Sat 18 Apr 2020 08:37:35 PM UTC, comment #10: 

@Guillaume: I can confirm what you are seeing.  I uploaded a modified version of your script called tst_uitoggle.m which shows that the clipping is occurring in the upper left corner.


f = figure ("toolbar", "none");
t = uitoolbar (f);

%% Try 16 different sizes
for i=8:8:128

  %% Create gridded image with black squares separated by white lines
  img = zeros (i, i, 3, "uint8");
  img (1:8:end, :, :) = 255;
  img (:, 1:8:end, :) = 255;

  %% Color each 8x8 square on diagonal differently
  for j = 1:(i/8)
    idx = (8*(j-1)) + (1:8);
    img (idx, idx, :) = 256 - 256/(i/8) * (j-1);
  endfor

  b = uitoggletool (t, "cdata", img);
end


Image of the result is attached as file uitoggle_clipping.png.


(file #48862,

Rik <rik5>
Group administrator
Fri 17 Apr 2020 03:40:35 PM UTC, comment #9: 

Thanks for the feedback, Rik. I tried a number of things but I don't understand Qt enough. Perhaps it should be opened as a different issue or forgotten if it is indeed a window manager setting. Here is some code to illustrate what I observe:


f = figure ("toolbar", "none");
t = uitoolbar (f);
for i=8:8:128
  img = zeros (i, i, 3);
  img (1:8:end, :, :) = 1;
  img (:, 1:8:end, :) = 1;
  b = uitoggletool (t, "cdata", img);
end


I attach a screenshot of the output with Octave and Matlab. There are bigger margins in Octave resulting in smaller buttons when the cdata image is 16x16, which is the recommended size (and indeed the size of the icons used by Matlab for its interface):
https://www.mathworks.com/help/matlab/ref/uipushtool.html#br48a3r_sep_shared-CData

Octave is cropping at 32 from top left while Matlab seems to be cropping at 24 from the centre. In Matlab, there is no margin between buttons and the cdata can cover the entire button so if a button is indeed 24x24, using 16x16 allows to create a margin within each button instead of between.


Guillaume <gyom>
Thu 16 Apr 2020 10:05:39 PM UTC, comment #8: 

Documentation for QAction class at https://doc.qt.io/qt-5/qaction.html.

It doesn't have a setIconSize member function.

Rik <rik5>
Group administrator
Thu 16 Apr 2020 09:07:21 PM UTC, comment #7: 

That's a different issue.  The function setIconSize is from the class QAbstractButton which both PushButton and ToggleButton derive from.  I guess that QAction doesn't have such a member function.

Do you need to override the icon size here?  In general, the window manager will select a size that is appropriate for the ToolBar and is consistent with the user's own settings (small icons, medium icons, large icons).

Rik <rik5>
Group administrator
Thu 16 Apr 2020 08:21:48 PM UTC, comment #6: 

Rik, I was trying to apply your change to the buttons of a uitoolbar (uipushtool and uitoggletool) and was looking at libgui/graphics/ToolBarButton.cc:


  action->setIcon (QIcon (QPixmap::fromImage (img)));
  action->setIconSize (QSize (32, 32)); // ?????


but it doesn't seem setIconSize() can be used like this here:


../libgui/graphics/ToolBarButton.cc:124:21: error: ‘class QAction’ has no member named ‘setIconSize’; did you mean ‘setIcon’?
             action->setIconSize (QSize (32, 32));


Is this a different issue?

Guillaume <gyom>
Thu 16 Apr 2020 07:54:21 PM UTC, comment #5: 

Actually, this was easier to fix than I thought.  I checked in a change on the development branch here (https://hg.savannah.gnu.org/hgweb/octave/rev/a56ee7986ea4).

In order to see the fix you will need to be able to download the source code from Octave Mercurial repository and then compile.  Or wait until the 7.1 release of Octave.

This might get backported to the 6.1 release, but that is not yet certain.

Rik <rik5>
Group administrator
Thu 16 Apr 2020 06:05:13 PM UTC, comment #4: 

The issue is in libgui/graphics/PushButtonControl.cc at line 61.  The function is


PushButtonControl::PushButtonControl (octave::base_qobject& oct_qobj,
                                      octave::interpreter& interp,
                                      const graphics_object& go,
                                      QPushButton *btn)
  : ButtonControl (oct_qobj, interp, go, btn)
{
  uicontrol::properties& up = properties<uicontrol> ();

  btn->setAutoFillBackground (true);
  octave_value cdat = up.get_cdata ();
  QImage img = Utils::makeImageFromCData (cdat,
                                          cdat.rows (), cdat.columns ());
  btn->setIcon (QIcon (QPixmap::fromImage (img)));
}


I modified the code to print out the width and height of the QImage which is correct.  I then checked the size of the QPixmap, and it too is correct.  The issues seems to be with QIcon.  The window manager uses the style that you have selected for buttons, and applies it to the QIcon when the setIcon() function is called.  For normal systems, this means the size is something small like 16x16 or 22x22 pixels.

I'm not a Qt programmer, but the resolution to the problem will be found by changing something related to the QIcon class in Qt.

Rik <rik5>
Group administrator
Tue 14 Apr 2020 08:16:19 PM UTC, comment #3: 

Confirmed.

I wrote a smaller test script tst_58144.m which is attached.  The code for that is


load startbutton.mat

figure()

h = uicontrol ('Style','pushbutton', ...
               'Position', [60 12 60 75], ...
               'CData', startbutton);

%% icon should be 60x75 since that is the size of
%% startbutton (60x75x3 uint8 image).
%% Instead, it is 12x14 pixels (on my machine)


I also extracted the startbutton image from your file and placed it in its own file "startbutton.mat" to make things simpler.


(file #48838, file #48839)

Rik <rik5>
Group administrator
Tue 14 Apr 2020 07:34:34 PM UTC, comment #2: 

I just uploaded two files. Put them in a same directory and run GUItest.m in Matlab or Octave. You will see the difference.

Thank you very much for your help!

comment #1:

> Could you upload the image file and any code you used to initialize the variable "startbutton"?
>
> It is very helpful to be able to reproduce the error in order to fix it.

Yonggon Lee <yonggonlee>
Thu 09 Apr 2020 03:19:29 PM UTC, comment #1: 

Could you upload the image file and any code you used to initialize the variable "startbutton"?

It is very helpful to be able to reproduce the error in order to fix it.

Rik <rik5>
Group administrator
Wed 08 Apr 2020 10:35:58 PM UTC, original submission:  

I am trying to put an image on the UIcontrol pushbutton.

uicontrol('Style','pushbutton','Position',[10 10 75 60],...
      'CData',startbutton)

The startbutton has been defined properly with the size same as the defined pushbutton.

Instead of covering the entire pushbutton, the image is shown in the center of the pushbutton in very small size...

The attached picture shows MATLAB case on the left, Octave case on the right.

Yonggon Lee <yonggonlee>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #48897:  icons32vs24.png added by gyom (22KiB - image/png)
file #48886:  ToolBarButton.24x24.patch added by rik5 (870B - text/x-patch)
file #48887:  Octave_24x24_clipping.png added by rik5 (17KiB - image/png)
file #48869:  icons.png added by gyom (3KiB - image/png)
file #48864:  uitoggle_clipping_2.png added by rik5 (9KiB - image/png)
file #48862:  tst_uitoggle.m added by rik5 (457B - text/x-matlab)
file #48863:  uitoggle_clipping.png added by rik5 (6KiB - image/png)
file #48858:  uitoolbar.png added by gyom (8KiB - image/png)
file #48838:  startbutton.mat added by rik5 (4KiB - application/octet-stream)
file #48839:  tst_58144.m added by rik5 (295B - text/x-matlab)
file #48835:  GUItest.m added by yonggonlee (1KiB - text/plain - I uploaded two files. GUItest.m creates very simple GUI with a banner and a button. gosetlogo.mat has images in it.)
file #48836:  gosetlogo.mat added by yonggonlee (36KiB - application/octet-stream - I uploaded two files. GUItest.m creates very simple GUI with a banner and a button. gosetlogo.mat has images in it.)
file #48789:  Capture.JPG added by yonggonlee (12KiB - image/jpeg - CData implementation (Left Matlab, Right Octave))

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by yonggonlee (Submitted the item)
  • -email is unavailable- added by yonggonlee
  •  

    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 group members can vote.

     

    Follow 22 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-04-22 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2020-04-21 gyom Attached File- Added icons32vs24.png, #48897
    2020-04-20 rik5 Attached File- Added ToolBarButton.24x24.patch, #48886
        Attached File- Added Octave_24x24_clipping.png, #48887
    2020-04-19 gyom Attached File- Added icons.png, #48869
    2020-04-19 rik5 Attached File- Added uitoggle_clipping_2.png, #48864
    2020-04-18 rik5 Attached File- Added tst_uitoggle.m, #48862
        Attached File- Added uitoggle_clipping.png, #48863
    2020-04-17 gyom Attached File- Added uitoolbar.png, #48858
    2020-04-16 rik5 StatusConfirmed Ready For Test
        Release5.2.0 dev
    2020-04-14 rik5 StatusNeed Info Confirmed
        SummaryCdata for UIcontrol is not properly implemented "Cdata" icon for UIcontrol is incorrectly sized
    2020-04-14 rik5 Attached File- Added startbutton.mat, #48838
        Attached File- Added tst_58144.m, #48839
        Operating SystemMicrosoft Windows Any
    2020-04-14 yonggonlee Attached File- Added GUItest.m, #48835
        Attached File- Added gosetlogo.mat, #48836
    2020-04-09 rik5 StatusNone Need Info
    2020-04-08 yonggonlee Attached File- Added Capture.JPG, #48789
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code