bugGNU Octave - Bugs: bug #35329, change of paperorientation does...

 
 

bug #35329: change of paperorientation does not update papersize

Submitter:  Ben Abbott <bpabbott>
Submitted:  Thu 19 Jan 2012 03:02:06 PM UTC
   
 
Category:  Plotting Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  bpabbott
Originator Name:  Ben Abbott 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 24 Jan 2012 08:35:51 PM UTC, comment #11: 

Thanks Rik.

I was wondering about the coding style part.

I have some more changes to graphics.cc. I'll review my coding before pushing.

Ben

Ben Abbott <bpabbott>
Group Member
Tue 24 Jan 2012 05:18:44 PM UTC, comment #10: 

Good patch Ben.  I can see you're going to be a full-fledged C++ programmer soon. 

I made some style changes and checked them in here (http://hg.savannah.gnu.org/hgweb/octave/rev/08779abcb640).  I'll explain what I did so you can see the method in my madness.

I like to carryover the Octave spacing convention for functions and indexing that exists in m-files.  To whit, use a space between the function name and the opening parenthesis, but cuddle the parenthesis when the name is a variable and the operation is indexing.  Here is a good example from earlier in graphics.cc.


static Matrix
default_data (void)
{
  Matrix retval (1, 2);

  retval(0) = 0;
  retval(1) = 1;

  return retval;
}


This is completely optional, but I like using the in-place operators such as *=, +=, etc.  In m-file implementations these are 3X faster.  In C++, gcc is probably smart enough to optimize away the temporary.  However, who wants to take a chance?


  pos(0) = pos(0) * 72;
     =>
  pos(0) *= 72;


The graphics property framework has a front end and a back end.  The front end (set routines) has to deal with caseless strings because people will type anything in any format.  However, the values are always stored in a standardized format and the back end (get routines) always return a lower case version of the property in question.  I changed some of your patch to use std::string where it was possible.  The equality operator is overloaded for strings so this allows the code to be simplified.


  caseless_str porient = get_paperorientation ();
  if (porient.compare ("landscape"))

     =>

  if (get_paperorientation () == "landscape")


There's no way that you would know about it, but the swapping of temporary values happens so frequently that C++ includes a routine for it in the std library.  I rewrote the following.


  double tmp;
  tmp = a;
  a = b;
  b = tmp;

    =>

  std::swap (a, b);


Finally, I added the 'const' keyword to your declarition of papertype conversion values.  The compiler might have figured out that you only assigned to these once.  But, it is better to be explicit and tell the compiler that.  In this case, it can then do all the multiplication and division only once at compile time and the magic values are stored in the code.  Otherwise, there is a chance that these values are computed again every time the function is called.


  const double mm2in = 1.0 / 25.4;
  const double tol = 0.01;


This may look like a lot, but it really isn't.  These are style issues only and the code works without them.

Rik <rik5>
Group administrator
Tue 24 Jan 2012 01:32:40 PM UTC, comment #9: 

I've pushed a changeset.

I'll close this. If something is wrong with the changeset, this can be reopened.

Ben Abbott <bpabbott>
Group Member
Tue 24 Jan 2012 03:02:43 AM UTC, comment #8: 

I've attached a changeset that works for me. If there are no objections, I'll push it to the default branch tomorrow.

(file #24879)

Ben Abbott <bpabbott>
Group Member
Tue 24 Jan 2012 01:46:28 AM UTC, comment #7: 

I think I found the problem. I'll post a changeset once I've done more testing.

Ben Abbott <bpabbott>
Group Member
Tue 24 Jan 2012 01:27:19 AM UTC, comment #6: 

I'm close to a fix, but am unable to figure out how to add an updater for the paperorientation to the sources.

I'm attached a patch (paperorientation.patch) that ends with the error below.


Undefined symbols for architecture x86_64:
  "figure::properties::set_paperorientation(octave_value const&)", referenced from:
      figure::properties::set(caseless_str const&, octave_value const&) in liboctinterp_la-graphics.o
ld: symbol(s) not found for architecture x86_64


I don't have a solid understanding for what needs to be done to add an updated to the figure::properties. So, I just tried to copy what was done for papertype.

(file #24878)

Ben Abbott <bpabbott>
Group Member
Mon 23 Jan 2012 01:02:26 AM UTC, comment #5: 

I revived my old m-file implementation and reviewed the updating portions in graphics.cc.

At the present, the m-file and c++ implementations interfere with each other.

It looks to me as if the proper think to do is to do the fix in graphics.cc

Ben Abbott <bpabbott>
Group Member
Sun 22 Jan 2012 10:36:42 PM UTC, comment #4: 

Opps, I submitted before I was done.

In 2008, I looked at implementing all the listeners as m-files. The problem I encountered was how to go about registering the listeners as objects were created.

In addition, the listeners tend to recursive. For example, changing papertype triggers a change to papersize, changing papersize triggers a change to papertype, & orientation. So it is necessary to suppress recursion while the listener is updating.

Since there is a partial c++ side implementation for the paper properties, it is either necessary to extend the c++ code, or to remove it and rely upon m-files entirely.

In either event, I still have my work from 2008. I'll dig it out and see if I can separate the part for the paper properties.

Ben Abbott <bpabbott>
Group Member
Sun 22 Jan 2012 10:24:47 PM UTC, comment #3: 

I should have been more thorough in my original comment. There is already some listeners in place for the paper properties.


set (gcf, "papertype", "a4")
get (gcf, "papersize")
ans =    8.2677   11.6929


However, the listener doesn't always to the right thing.

The papertype should be automatically detected.


set (gcf, "papersize", [8.5, 11.0])
get (gcf, "papertype")
ans = <custom>


In addition to the papertype, the orientation should also be detected.


set (gcf, 'papertype', 'usletter')
set (gcf, "papersize", [8.5, 11.0])
get (gcf, "papertype")
ans = usletter
set (gcf, "papersize", [11.0, 8.5])
get (gcf, "papertype")
ans = <custom>
get (gcf, "paperorientation")
ans = portrait


Ben Abbott <bpabbott>
Group Member
Sun 22 Jan 2012 08:10:40 PM UTC, comment #2: 

What needs to be done in the C++ code?  Do you want to add a permanent listener on "paperorientation" that updates papersize?

Just brainstorming, is the following approximately what you are after?


function orientation_listen (h, dummy)

  papersize = get (h, "papersize");
  paperorientation = get (h, "paperorientation");
  if ((papersize(1) > papersize(2) && strcmp (paperorientation, "portrait"))
      || (papersize(1) < papersize(2) && strcmp (paperorientation, "landscape")))
    set (h, "papersize", papersize([2, 1]));
    paperposition = get (h, "paperposition");
    set (h, "paperposition", paperposition([2, 1, 4, 3]));
  endif

endfunction

addlistener (gcf, "paperorientation", @orientation_listen);



Rik <rik5>
Group administrator
Thu 19 Jan 2012 03:02:47 PM UTC, comment #1: 

At the moment the print() function assumes landscape means width > length.

The gs_papersize() function included in _print_parse_opts_.m has some information that may be relevant to fixing this, and should be removed after the c++ side is fixed.

Ben Abbott <bpabbott>
Group Member
Thu 19 Jan 2012 03:02:06 PM UTC, original submission:  

The following shows that, unlike matlab, octave does not update papersize from paperorientation:

get(gcf, 'paperorientation')
ans = portrait
get(gcf, 'papersize')
ans =

  8.50   11.00
set(gcf, 'paperorientation', 'landscape')
get(gcf, 'papersize')
ans =

  8.50   11.00

Ben Abbott <bpabbott>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #24879:  changeset.patch added by bpabbott (8KiB - application/octet-stream - 1st attempt at a working changeset)
file #24878:  paperorientation.diff added by bpabbott (6KiB - application/octet-stream - non-working attempt)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by bpabbott (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-01-24 bpabbott StatusPatch Submitted Fixed
        Assigned toNone bpabbott
        Open/ClosedOpen Closed
    2012-01-24 bpabbott Attached File- Added changeset.patch, #24879
        StatusNone Patch Submitted
    2012-01-24 bpabbott Attached File- Added paperorientation.diff, #24878

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code