bugGNU Octave - Bugs: bug #41957, print -P fails, and print when no...

 
 

bug #41957: print -P fails, and print when no figure exists crashes

Submitted by:  Michael Godfrey <godfrey>
Submitted on:  Tue 25 Mar 2014 05:19:42 PM UTC  
 
Category: PlottingSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: None
Originator Name: Michael GodfreyOpen/Closed: Closed
Release: devOperating 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 Jump to the original submission)

Sat 29 Mar 2014 12:52:43 AM UTC, comment #13:

Good, not just me...

Michael Godfrey <godfrey>
Project Member
Fri 28 Mar 2014 11:01:03 PM UTC, comment #12:

I missed a parenthesis. I checked in a changeset to fix that (http://hg.savannah.gnu.org/hgweb/octave/rev/077575fe9a7e).

Rik <rik5>
Project Administrator
Fri 28 Mar 2014 10:15:02 PM UTC, comment #11:

We have a build failure due to these changes, parse error near the printer error message in _print_parse_opts_.m:

http://hydra.nixos.org/build/9862648/log/tail-reload

Mike Miller <mtmiller>
Project Administrator
Fri 28 Mar 2014 07:58:47 PM UTC, comment #10:

I had already gone ahead and committed the first part of the patch here (http://hg.savannah.gnu.org/hgweb/octave/rev/29f00c0d0657).

For the second part with Ghostscript devices I made this changeset (http://hg.savannah.gnu.org/hgweb/octave/rev/cdc16fbb513f).

Rik <rik5>
Project Administrator
Fri 28 Mar 2014 06:49:15 PM UTC, comment #9:

I have never seen a ghostscript device list that did not
contain ps2write at least so the additional test adds
little.

Michael Godfrey <godfrey>
Project Member
Fri 28 Mar 2014 03:30:55 PM UTC, comment #8:

I think there is still some confusion on the sense of the test. We are checking explicitly that the the printer device is one of the Ghostscript devices. If we want any other allowable format we need to put a test in the if conditional for it. Before, we had a test for postscript as well since this is a common enough format to send to the printer.

If modern printers can accept anything thrown at them then we should just eliminate this test completely and pass everything through.

Rik <rik5>
Project Administrator
Fri 28 Mar 2014 02:59:15 PM UTC, comment #7:

Yes, I think there was a parens problem, but my latest
version only tests for the device in the gnuplot
list, nothing else. Or, at least that is what I intended,
and what I think is appropriate. Using the pswrite or
ps2write restriction means that many printers cannot be
used.

So, just:
if (! (any (strcmp (arg_st.devopt, gs_device_list)))
error...
endif

should do it.

Michael Godfrey <godfrey>
Project Member
Fri 28 Mar 2014 02:02:50 PM UTC, comment #6:

I don't think so, look at the ordering of parentheses. The original patch you submitted prints an error if devopt is not in gs_device_list or if it is either pswrite or ps2write. The parentheses nesting is different in the test I posted, it errors if devopt is not in either gs_device_list or the list of pswrite and ps2write.

Maybe that's what you meant to change it to, but that's not what your patch did. That's how I read Rik's objection to your change also.

Mike Miller <mtmiller>
Project Administrator
Fri 28 Mar 2014 01:50:28 PM UTC, comment #5:

Mike,
That was what the original patch did, but it does not really
make sense. My new suggested version just checks the gnuplot
known devices list. If that leads to a problem with a particular
printer it should be up to the user to figure out. Otherwise, we
start having to know what formats each printer in the world
will accept.

Michael Godfrey <godfrey>
Project Member
Fri 28 Mar 2014 01:44:27 PM UTC, comment #4:

Perhaps the logic was meant to be

This would be making it a more inclusive check, not more restrictive as it currently is. That seems to be what the comment before the error is implying.

Mike Miller <mtmiller>
Project Administrator
Fri 28 Mar 2014 11:03:48 AM UTC, comment #3:

Rik,

The patch:
|| any (strcmp (arg_st.devopt, {"pswrite", "ps2write"}))
is obviously wrong. But, the original code is also
wrong. It does not allow spooling to a PDF printer or anything
except PS. Many modern printers accept other formats. In addition,
the "printer" may be a "virtual" printer which outputs PDF
rather than actually spooling to any device.

So, the only part of this test that is valid is the line

if (! any (strcmp (arg_st.devopt, gs_device_list))

This will allow formats that the specific printer does not
support but there is no obvious way of avoiding this.

So, the patch should read:
diff -r d0d9f6daa4b6 -r 90c199446cea scripts/plot/util/private/__print_parse_opts__.m
--- a/scripts/plot/util/private/__print_parse_opts__.m Mon Mar 24 21:24:16 2014 -0700
+++ b/scripts/plot/util/private/__print_parse_opts__.m Tue Mar 25 17:00:41 2014 +0000
@@ -335,7 +335,8 @@
error ("print: a file name may not specified when spooling to a printer")
endif
if (! any (strcmp (arg_st.devopt, gs_device_list))
- || ! any (strcmp (arg_st.devopt, {"pswrite", "ps2write"})))
+ )
- ## Only postscript and supported ghostscript devices
- error ("print: invalid format for spooling to a printer")
+ error ("print: invalid format for ghostscript")
endif

OK?

Michael Godfrey <godfrey>
Project Member
Thu 27 Mar 2014 03:21:51 PM UTC, comment #2:

>> ishandle([])


ans =

[]

>> version


ans =

7.9.0.529 (R2009b)

About, the change to
- || ! any (strcmp (arg_st.devopt, {"pswrite", "ps2write"})))

I thought I had a reason for that, but please remove it until
I have a chance to rethink.

Michael Godfrey <godfrey>
Project Member
Thu 27 Mar 2014 03:12:43 PM UTC, comment #1:

Could you run the following test code in Matlab?

Does this return an empty matrix, or 0 because an empty matrix is definitely not a handle?

In part of the patch you have

The intent was to check for formats which couldn't be spooled to the printer. On UNIX systems postscript usually can be sent to the printer which is why there was a check for the format NOT being pswrite. By dropping the '!' we will now error out for what seems a legitimate case.

Rik <rik5>
Project Administrator
Tue 25 Mar 2014 05:19:42 PM UTC, original submission:

This patch corrects 2 problems:
1. print -Pa_printer introduces an extraneous -P so that
the resulting lpr command fails. Removed -P.

2. The test for the presence of a figure at the beginning of
print() fails to detect the lack of a figure. This may
be due to a change in isfigure(). If there is no figure
and the opts.figure value is set to empty, isfigure returns
true. Use of isempty instead of isfigure fixes this.

In addition I edited the print help info. Currently it was
unclear and referred to the ability to generate TeX (as
opposed to LaTeX. Since it unconditionally gereates LaTeX
I removed the TeX references and tried to clarify the operation
of the latex modes.

The print command is quite complex and handles lots of different
cases and both fltk and gnuplot. So, it would be helpful if
these patches could be tested in other environments. So far
they work for me and make generating plots with real TeX in the
text strings much more convenient.

Michael Godfrey <godfrey>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #31036:  print_fixes.diff added by godfrey (6KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by godfrey (Submitted the item)
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 28 Mar 2014 11:01:03 PM UTCrik5StatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Fri 28 Mar 2014 10:15:02 PM UTCmtmillerStatusFixed=>In Progress
      Open/ClosedClosed=>Open
    Fri 28 Mar 2014 07:58:47 PM UTCrik5StatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Tue 25 Mar 2014 05:19:42 PM UTCgodfreyAttached File-=>Added print_fixes.diff, #31036

    Back to the top


    Powered by Savane 3.1-cleanup1