bugGNU Octave - Bugs: bug #53468, Octave 4.3.0+ can't load figures...

 
 

bug #53468: Octave 4.3.0+ can't load figures saved with previous versions

Submitter:  Pantxo Diribarne <pantxo>
Submitted:  Sun 25 Mar 2018 09:29:43 PM UTC
   
 
Category:  Plotting Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
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

Tue 12 Feb 2019 09:46:18 PM UTC, comment #21: 

@Mike: Thanks, I pushed the change here:

http://hg.savannah.gnu.org/hgweb/octave/rev/b3b78bf83113

Pantxo Diribarne <pantxo>
Group Member
Tue 12 Feb 2019 01:20:41 AM UTC, comment #20: 

Thanks, that works for me, the tests are properly installed and distributed now.

One more small thing, after running the test, the figures and all graphical elements still exist in the user's environment. Running 'figure' after the test creates 'Figure 4'. Would it be better to write the test as something like


%!test
%! unwind_protect
%!   hf = hgload ("ofig403.ofig");
%! unwind_protect_cleanup
%!   try, close (hf); end_try_catch
%! end_unwind_protect


Mike Miller <mtmiller>
Group Member
Sat 09 Feb 2019 10:27:29 PM UTC, comment #19: 

I did "make install" and then ran "__run_test_suite__" from the installed binary and made sure the hgload related tests were there and passed.

I pushed the patch here:

http://hg.savannah.gnu.org/hgweb/octave/rev/7b9a5ab8350f


Pantxo Diribarne <pantxo>
Group Member
Sat 09 Feb 2019 09:24:32 PM UTC, comment #18: 

Thanks for tackling that, patch looks right to me, but I don't have time to test it today. If the tst and ofig files are installed with "make install", and are included in the tarball with "make check", then it's doing the right thing.

Mike Miller <mtmiller>
Group Member
Sat 09 Feb 2019 08:42:09 PM UTC, comment #17: 

Sorry. Does the attached patch do the right thing?

(file #46222)

Pantxo Diribarne <pantxo>
Group Member
Fri 08 Feb 2019 11:31:31 PM UTC, comment #16: 

I just noticed the auxiliary files in test/figure_files that go with this test also need to be included in the build system. I think they can also safely be included in the same TEST_FILES variable, but then it's easy to lose track of which .tst file they go with.

It might be more maintainable and easier to understand if these files were all grouped together in a subdirectory, similar to other subdirectories that have associated function files, or the C source files in test/mex.

Mike Miller <mtmiller>
Group Member
Fri 08 Feb 2019 11:13:56 PM UTC, comment #15: 

The file hgload.tst is not properly added to the build system to be distributed with the release and to be installed with the rest of the test scripts. Can you please add it to test/module.mk? See for example https://hg.savannah.gnu.org/hgweb/octave/rev/7094f73d4ece, and please make sure the file is listed in sorted order.

Mike Miller <mtmiller>
Group Member
Sun 03 Feb 2019 11:14:40 PM UTC, comment #14: 

I pushed the patch here:

http://hg.savannah.gnu.org/hgweb/octave/rev/c3c657ba1e97

Marking this bug "ready for test"

Pantxo Diribarne <pantxo>
Group Member
Sun 27 Jan 2019 04:07:55 PM UTC, comment #13: 

I crossed this bug on the release list since it is no more a regression. Nevertheless, it would be good to add tests for regressions. I attached an updated patch that I'll push to stable if it is OK.

(file #46107)

Pantxo Diribarne <pantxo>
Group Member
Wed 11 Apr 2018 08:34:05 AM UTC, comment #12: 

I agree that the test cases from the patch should be added to stable.

My comment was just that the patch also makes a small change to struct2hdl - I am actually happy with it and think it is a good strategy to create the figure invisible first, populate its content and make it visible at the end if required (it might actually be faster and will prevent a figure to appear temporarily if it was meant to be invisible at the end of the day anyway). As long as the input structure to struct2hdl always defines the "Visible" property then things will behave in the same way. It would affect the following code though:


s_oct40 = [];
s_oct40.handle = 1;
s_oct40.type = "figure";
s_oct40.properties.tag = "";
s_oct40.children = [];
s_oct40.special = [];

get (struct2hdl (s_oct40), "Visible")


But it is not an issue if the input to struct2hdl is always coming from hdl2struct/hgload where all of the properties are always present.

Guillaume <gyom>
Wed 11 Apr 2018 12:29:11 AM UTC, comment #11: 

I thought this bug was already fixed, the only thing remaining was to add a test case. Is that not right? If it doesn't change behavior I don't see a problem with adding more tests on stable to prevent future regressions.

Mike Miller <mtmiller>
Group Member
Tue 10 Apr 2018 09:06:56 AM UTC, comment #10: 

@Mike: I think any change that may affect those functions should go to default now (we don't want to change the way we save/load figures in the middle of a major version). So this patch should go to default I think.

@gyom (response to your comment in bug #44670): yes, currently we save all the properties, regardless of their value being the default. So if the figure used to be visible, its "visible" property will be restored afterwards. This is not efficient, but ensures a figure is open in later versions with the same properties  values as before. See the example of the colormap which changed between 4.0.x and 4.2.x

Pantxo Diribarne <pantxo>
Group Member
Tue 10 Apr 2018 08:44:24 AM UTC, comment #9: 

I wrote a comment about the patch in the wrong bug report, see https://savannah.gnu.org/bugs/?44670#comment14

Guillaume <gyom>
Tue 10 Apr 2018 02:41:56 AM UTC, comment #8: 

I haven't had a chance to take a look at this patch or test it yet, do you think this should be on stable for 4.4?

Mike Miller <mtmiller>
Group Member
Wed 28 Mar 2018 01:47:58 PM UTC, comment #7: 

I attached a patch with two binary files from 4.0.3 and 4.2.1 respectively. The figures (2 subplots with demos "text 2" and "hold 6") contain no ui* objects and no light since struct2hdl doesn't handle those objects.

(file #43704)

Pantxo Diribarne <pantxo>
Group Member
Tue 27 Mar 2018 05:58:55 PM UTC, comment #6: 

I think it's appropriate to include data files for tests, preferably in a subdirectory of the test directory along with the associated test commands.

I don't think it's appropriate to have a visible figure appear during the test suite. That would be useful for a demo.

Mike Miller <mtmiller>
Group Member
Tue 27 Mar 2018 04:49:53 PM UTC, comment #5: 

If we can use data files for tests then yes, why not.

Is it also ok if a figure is opened by a test (with "Visible" set to "on")?

Guillaume <gyom>
Tue 27 Mar 2018 04:24:41 PM UTC, comment #4: 

@gyom: yes indeed it would be very useful, but having a test_save_load.ofig binary file would even let us store larger and more complicated figures.

@Mike: Are there already such test-only data files in Octave sources?

Pantxo Diribarne <pantxo>
Group Member
Tue 27 Mar 2018 10:15:59 AM UTC, comment #3: 

Pantxo, do you think it would be useful to add tests with figures saved from previous versions of Octave? See, eg, attachment for a figure saved with 4.2.1.

(file #43696)

Guillaume <gyom>
Mon 26 Mar 2018 09:10:41 PM UTC, comment #2: 
Pantxo Diribarne <pantxo>
Group Member
Mon 26 Mar 2018 06:46:39 PM UTC, comment #1: 

Confirm and agree this should be fixed for 4.4.

Mike Miller <mtmiller>
Group Member
Sun 25 Mar 2018 09:29:43 PM UTC, original submission:  

If I try to open a figure generated with octave 4.2 with the current stable I see the following error:


hgload /tmp/toto.ofig
warning: 'drawmode' is deprecated and will be removed from a future version of Octave
error: addproperty: a 'interpreter' property already exists in the graphics object
error: called from
    struct2hdl>addmissingprops at line 657 column 7
    struct2hdl>createline at line 294 column 3
    struct2hdl at line 144 column 7
    struct2hdl at line 167 column 13
    hgload at line 58 column 5


This is in my opinion a very nasty bug that must be fixed before Octave 4.4.


Pantxo Diribarne <pantxo>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #46222:  test_hgload_2.patch added by pantxo (397KiB - text/x-patch)
file #46107:  test_hgload.patch added by pantxo (395KiB - text/x-patch)
file #43704:  hgload_tests.pacth added by pantxo (308KiB - application/octet-stream)
file #43696:  testfig.m added by gyom (28KiB - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by gyom (Updated the item)
  • -email is unavailable- added by pantxo (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 17 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-02 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2019-02-09 pantxo Attached File- Added test_hgload_2.patch, #46222
    2019-02-08 mtmiller Carbon-CopyRemoved 80942 -
    2019-02-03 pantxo StatusPatch Submitted Ready For Test
    2019-01-27 pantxo Attached File- Added test_hgload.patch, #46107
        Severity4 - Important 3 - Normal
        Item GroupRegression None
    2018-04-10 mtmiller StatusIn Progress Patch Submitted
    2018-03-28 pantxo Attached File- Added hgload_tests.pacth, #43704
    2018-03-27 mtmiller StatusFixed In Progress
        Open/ClosedClosed Open
    2018-03-27 gyom Attached File- Added testfig.m, #43696
    2018-03-26 pantxo StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2018-03-26 mtmiller Severity3 - Normal 4 - Important
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.14-6db8.
    Corresponding source code