bugGNU Octave - Bugs: bug #57211, __appdata__ property should be...

 
 

bug #57211: __appdata__ property should be hidden

Submitter:  Rik <rik5>
Submitted:  Tue 12 Nov 2019 12:48:51 AM UTC
   
 
Category:  Plotting Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  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 13 Nov 2019 04:47:17 PM UTC, comment #14: 

I pushed cset version 2 here (https://hg.savannah.gnu.org/hgweb/octave/rev/e18237114cf8).

I updated the commit message.  In particular, the default of the new _appdata_ property is not an empty structure, but rather an empty matrix.  This is in keeping with the rest of the graphics properties, and it probably has performance benefits because an octave_map is a pretty heavyweight object.  However, it did necessitate changing the code in getappdata.m so that it returns an empty struct if the graphics object has no appdata applied.


--- a/scripts/gui/getappdata.m  Wed Nov 13 09:13:41 2019 -0500
+++ b/scripts/gui/getappdata.m  Wed Nov 13 08:24:12 2019 -0800
@@ -62,11 +62,11 @@ function value = getappdata (h, name)
     if (numel (h) != 1)
       error ("getappdata: Only one handle H may be used when fetching appdata");
     endif
-    try
-      value = get (h, "__appdata__");
-    catch
+
+    value = get (h, "__appdata__");
+    if (isempty (value))
       value = struct ();
-    end_try_catch
+    endif
   endif


Marking as fixed and closing report.

Rik <rik5>
Group administrator
Wed 13 Nov 2019 02:54:31 PM UTC, comment #13: 

@Pantxo: Your patch (file #47858) looks good, applies and builds without problems.

Now I can load the appdata from TestGUI.fig (file #47839) in bug #57212, which can be closed as well, when your patch applied to the Octave repo.


>> h = hgload ("TestGUI.fig")
h = -35.973
>> getappdata (h)
ans =

  scalar structure containing the fields:

    GUIDEOptions =

      scalar structure containing the fields:

        active_h = [](0x0)
        taginfo =

          scalar structure containing the fields:

            figure: 1x1 scalar
            text: 1x1 scalar

        override = 0
        release = 13
        resize = none
        accessibility = callback
        mfile = 1
        callbacks = 1
        singleton = 1
        syscolorfig = 1
        blocking = 0
        lastSavedFile = /home/IT/jzimmer/Documents/MATLAB/GUI/gui/TestGUI.m

    lastValidTag = figure1


Same appdata output comes with


getappdata (h)
get (h, "applicationdata")
get (h, "__appdata__")


but in get(h) the output of the respective fields in omitted / hidden.

Kai Torben Ohlhus <siko1056>
Group Member
Wed 13 Nov 2019 01:02:40 PM UTC, comment #12: 

@Markus: You are right, this is probably the safest approach. I attached an updated patch that both adds "__appdat__" as a default property for all graphics objects and makes "applicationdata" and alias for it.

(file #47858)

Pantxo Diribarne <pantxo>
Group Member
Wed 13 Nov 2019 09:01:38 AM UTC, comment #11: 

Maybe we could keep "__appdata__" as an alias to the new "applicationdata" similarly to what we did for "vertexnormalsmode" and "normalmode" in this change:
https://hg.savannah.gnu.org/hgweb/octave/rev/a0ebc922fd52

But maybe keep the old alias in (all?) future versions...

Markus Mützel <mmuetzel>
Group administrator
Wed 13 Nov 2019 08:52:46 AM UTC, comment #10: 

Forget the objection of comment #8.  Your patch is fine.  Oversaw the further indexing with ".(name)" in the end, which might fail.

Kai Torben Ohlhus <siko1056>
Group Member
Wed 13 Nov 2019 08:49:58 AM UTC, comment #9: 

Correction comment #8: ... as with your patch now all graphic handles have this property ...

Kai Torben Ohlhus <siko1056>
Group Member
Wed 13 Nov 2019 08:48:07 AM UTC, comment #8: 

Wow, that update came fast.

Was it possible to remove all try-catches in getappdata?


diff -r fad2b816df2d -r 854033674ab7 scripts/gui/getappdata.m
--- a/scripts/gui/getappdata.m        Tue Nov 12 22:37:19 2019 +0100
+++ b/scripts/gui/getappdata.m        Wed Nov 13 09:25:39 2019 +0100
@@ -50,7 +50,7 @@
     value = cell (numel (h), 1);
     for i = 1:numel (h)
-      try
-        value{i} = (get (h(i), "__appdata__")).(name);
-      end_try_catch
+      value{i} = (get (h(i), "applicationdata")).(name);
     endfor

@@ -62,11 +62,7 @@
     if (numel (h) != 1)
       error ("getappdata: Only one handle H may be used when fetching appdata");
     endif
-    try
-      value = get (h, "__appdata__");
-    catch
-      value = struct ();
-    end_try_catch
+    value = get (h, "applicationdata");
   endif

 endfunction


as with your patch not all graphic handles have this property and all "h" are checked to be such:


if (! all (ishghandle (h(:))))


Just had a look that Rik introduced that try-catch semantic and this indeed increased the performance, even though it looks like a reasonable use-case of that anti-pattern to me ;-)
https://hg.savannah.gnu.org/hgweb/octave/rev/a94a1d742938

Regarding comment #6 and comment #7.  Then it looks like it should better not be renamed...

Kai Torben Ohlhus <siko1056>
Group Member
Wed 13 Nov 2019 08:44:12 AM UTC, comment #7: 

@Markus: No, indeed. The _appdata_ fields in graphics objects stored with previous versions of Octave will be treated as custom property. So it will be available in the restored object but won't be accessible using *appdata functions.

Pantxo Diribarne <pantxo>
Group Member
Wed 13 Nov 2019 08:39:49 AM UTC, comment #6: 

Pantxo: What about data a user stored using "setappdata"? Will it be possible to load older .fig files and still access that data using "getappdata"?

Markus Mützel <mmuetzel>
Group administrator
Wed 13 Nov 2019 08:28:52 AM UTC, comment #5: 

I attached an updated cset that makes that also renames the property.

Pantxo Diribarne <pantxo>
Group Member
Wed 13 Nov 2019 08:18:26 AM UTC, comment #4: 

You have for sure deeper insight into the current and recent use of the "__appdata__" property.  No further objections from my side.

Kai Torben Ohlhus <siko1056>
Group Member
Wed 13 Nov 2019 08:11:16 AM UTC, comment #3: 

@Kai: The _appdata_ is meant to store data relevant for high level objects such as legend, colorbar, subplots ... which we were never able to handle with struct2hdl.m. So we basically saved it in .ofig files but never used it (or even explicitly removed it) while restoring figures in struct2hdl.m. So I think it is safe to rename the property.

Pantxo Diribarne <pantxo>
Group Member
Wed 13 Nov 2019 02:24:47 AM UTC, comment #2: 


> But should we also rename the property to "applicationdata" and eventually help resolving bugs like bug #57212?


This would definitely resolve bug #57212.  But do we break compatibility with figures saved with older versions of Octave?

Kai Torben Ohlhus <siko1056>
Group Member
Tue 12 Nov 2019 06:32:36 PM UTC, comment #1: 

From the following test, it looks like Matlab store the appdata in a hidden property "applicationdata":


hax = subplot (2,1,1);
plot (1:10)
isequal (getappdata (hax), get (hax, "applicationdata"))


I attached a patch that makes "__appdata__" a hidden property of any graphics object. But should we also rename the property to "applicationdata" and eventually help resolving bugs like bug #57212?

(file #47850)

Pantxo Diribarne <pantxo>
Group Member
Tue 12 Nov 2019 12:48:51 AM UTC, original submission:  

The functions getappdata, setappdata, isappdata, rmappdata rely on a graphical property _appdata_ which is added to a graphics object using addproperty().

The issue is that this data store is internal and not meant to be displayed.  Executing "get (h)" on a graphics object with a significant amount of appdata is very confusing because there can be pages of display for this data store.

Ideally the property would not be displayed by get() which is Matlab-compatible behavior.

In C++ when properties are created they can have an attribute 'h' which stands for "hidden" and determines if the property should be displayed.

Either the "__appdata__" property needs to be added to all graphics objects in graphics.in.h with the hidden attribute, or the addproperty() function needs to be extended so that attributes can be added when the property is created.

Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #47858:  bug57211_4.patch added by pantxo (4KiB - text/x-patch)
file #47856:  bug57211_2.patch added by pantxo (5KiB - text/x-patch)
file #47850:  bug57211.patch added by pantxo (1KiB - text/x-patch)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by pantxo (Updated the item)
  • -email is unavailable- added by rik5 (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 group members can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-11-13 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2019-11-13 siko1056 StatusPatch Submitted Patch Reviewed
    2019-11-13 pantxo Attached File- Added bug57211_4.patch, #47858
    2019-11-13 pantxo Attached File- Added bug57211_2.patch, #47856
    2019-11-13 siko1056 Dependencies- bugs #57212 is dependent
    2019-11-12 pantxo Attached File- Added bug57211.patch, #47850
        StatusConfirmed Patch Submitted

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code