patchPSPP - Patches: patch #6210, Make viewport {width, length}...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6210: Make viewport {width, length} under direct ownership of the user interface

Submitter:  John Darrington <jmd>
Submitted:  Wed 19 Sep 2007 02:32:32 AM UTC
 
Category:  None Item Group:  None
Status:  Done Assigned to:  blp
Open/Closed:  Closed

( Jump to the original submission )

Wed 26 Sep 2007 04:27:53 AM UTC, comment #16: 

>So far as I can tell, that just leaves file #14025 to be
>committed. I'll let you do this then I think we can close this
>item.


Done, on both counts.

Ben Pfaff <blp>
Project Administrator
Wed 26 Sep 2007 12:09:05 AM UTC, comment #15: 

OK.  I've committed this.   So far as I can tell, that just leaves file #14025 to be committed.  I'll let you do  this then I think we can close this item.

John Darrington <jmd>
Project Administrator
Tue 25 Sep 2007 02:19:03 PM UTC, comment #14: 

>I've attached a patch which adds the appropriate bits to the GUI.


Looks OK to me.

>There are at least half a dozen reasons why it's not ideal, but in
>the end it all boils down to the fact that using the ascii driver
>for the gui is not the best solution anyway.


Better output needs to be a release goal for 0.6.0+1, I think.

Ben Pfaff <blp>
Project Administrator
Tue 25 Sep 2007 06:27:37 AM UTC, comment #13: 

It looks good.

I've attached a patch which adds the appropriate bits to the GUI.  There are at least half a dozen reasons why it's not ideal, but in the end it all boils down to the fact that using the ascii driver for the gui is not the best solution anyway.

But I think it's good enough for now.

(file #14026)

John Darrington <jmd>
Project Administrator
Tue 25 Sep 2007 04:43:51 AM UTC, comment #12: 

>I'm happy for you to commit all these patches.


I committed all but the one that installs the poll loop.  Just before writing the ChangeLog for that one, it occurred to me that there's an equivalent solution that doesn't need signals or polling or any of that: just re-check the size of the terminal after accepting the first line of a command.  Much cleaner and simpler.  I'm attaching the revised version of that patch.  The files that have changed from the previous version are read-line.c and terminal.c.

What do you think?

Ben Pfaff <blp>
Project Administrator
Tue 25 Sep 2007 04:01:22 AM UTC, comment #11: 

OK.  Thanks for the explanation.

I'm happy for you to commit all these patches.  The GUI side of things needs to be looked at closer, which I'll try to do later.

John Darrington <jmd>
Project Administrator
Tue 25 Sep 2007 03:50:42 AM UTC, comment #10: 

>I can't find any documentation which says that poll will return
>when the program receives SIGWINCH.


It's a special case of the general principle that a system call interrupted by a signal that has a user-specified handler will return with an EINTR error.  (Actually, signals can be restartable, which is why I used sigaction without specifying SA_RESTART.)

>I thought you needed ppoll.


ppoll solves a related but separate issue: the problem of atomically unblocking a signal and waiting for its arrival.  ppoll can be approximated with a call to sigprocmask followed by a call to poll, but there's a window between the system calls in which the signal can be delivered, in which case the poll will block without the caller having noticed that the signal set an indicator variable (e.g. window_size_changed).

ppoll is Linux-only, though.  Its cousin pselect also works, but  although pselect is in SUSv3 its support is not universal.  The traditional fallback is to use a pipe: the signal handler writes a byte to the pipe, the poll or select call polls the pipe for readability.  The fallback also avoids the race.

But I didn't think that it was worth worrying about for this case.  I have no reason to believe that it is common for a user to resize his terminal and hit Enter so quickly as to hit the race window ;-)

Ben Pfaff <blp>
Project Administrator
Tue 25 Sep 2007 01:10:45 AM UTC, comment #9: 

Everything looks fine to me, except that I'm confused by the code:

/* Wait for something to happen: either the user hits a key
         or changes the size of the window. */
      pfd.fd = STDIN_FILENO;
      pfd.events = POLLIN;
      retval = poll (&pfd, 1, -1);
      if (retval > 0 && pfd.revents & POLLIN)
        {

I can't find any documentation which says that poll will return when the program receives SIGWINCH.  For that, I thought you needed ppoll.   However, it seems to work, so perhaps I'm missing something.

John Darrington <jmd>
Project Administrator
Mon 24 Sep 2007 01:45:38 AM UTC, comment #8: 

These two are fine:

 viewport-fallback-obviousness.patch
 viewport-dont-call-it-a-script.patch

you can check these in if you're happy to do so.

I'd like to look at the others in more detail, and think about it for a while.  Unfortunately, this morning I must attend to my daytime work ...

John Darrington <jmd>
Project Administrator
Mon 24 Sep 2007 12:32:32 AM UTC, comment #7: 

OK, here's a series of patches that should apply on top of yours.  In order:

viewport-fallback-obviousness.patch
viewport-dont-call-it-a-script.patch
viewport-drop-long-view.patch
viewport-happy-signal-handler.patch

In testing viewport-drop-long-view.patch, I found out that in fact force_longview() was called when --testing-mode was used (don't know why my grep missed that earlier).  But I think that the heuristic I used there makes sense: only a tty has a width, so only wrap messages to the width when going to a tty.  What do you think?

I didn't look at the GUI half of this yet.

I'm not feeling particularly "with it" today, so there are probably a bunch of loose ends here, although I did test it.

(file #14016, file #14017, file #14018, file #14019)

Ben Pfaff <blp>
Project Administrator
Wed 19 Sep 2007 04:56:19 AM UTC, comment #6: 

>Two comments: ...


Agreed on both counts.

Ben Pfaff <blp>
Project Administrator
Wed 19 Sep 2007 04:52:37 AM UTC, comment #5: 

Ben wrote:

> I was planning to implement it in the ascii driver by allowing ascii
> driver options "length" and "width" to take the keyword "terminal" as
> value, instead of an (possibly macro-expanded) integer. If "terminal"
> is used, then that dimension adapts itself to the current size of the
> terminal.


That sounds plausible.  Two comments:

1. The gui needs to make use of this functionality too, so perhaps "terminal" is not the best word.  Maybe "auto" would be better?

2. Presumbably  SET {LENGTH,WIDTH} commands should be ignored by drivers with "auto" set for that value.

John Darrington <jmd>
Project Administrator
Wed 19 Sep 2007 04:22:31 AM UTC, comment #4: 

John wrote:

> ...in that case set_fallback_viewport does nothing. But perhaps it
> should be refactored to make the code more intuitive.


I see.  Yes, it could be a little more obvious.

> > I'd be happy to code one for you.
> Please do so.


OK, I will try to get around to it soon.

>Ben wrote:
> > I assume that only the ascii driver cares about terminal size?


John replied:

> Now that you mention it, it needs a bit of thought. The manual for
> 'that other software' says "These two subcommands apply only to text
> output". So it seems that it shouldn't apply to postscript or html
> drivers. In the case of an interactive PSPP session, there are two
> drivers active --- one going to the terminal, the other to pspp.list
> Common sense says that resizing the terminal should only affect the
> dimensions of the terminal output. So maybe we need to think about
> this a bit more.


I was planning to implement it in the ascii driver by allowing ascii
driver options "length" and "width" to take the keyword "terminal" as
value, instead of an (possibly macro-expanded) integer.  If "terminal"
is used, then that dimension adapts itself to the current size of the
terminal.  This would gracefully handle the case you point out above.

Ben Pfaff <blp>
Project Administrator
Wed 19 Sep 2007 03:54:15 AM UTC, comment #3: 

> I think that the new version of get_termcap_viewport calls
> set_fallback_viewport unconditionally, not just when tgetnum
> fails to return good values.


Yes that's true, but in that case set_fallback_viewport does nothing.  But perhaps it should be refactored to make the code more intuitive.

> I do not think it is a good idea to do so much work from a signal handler.


You're right.

>  I'd be happy to code one for you.


Please do so.

> The long_view variable appears to be effectively dead:
> I don't recall what it was originally for.


I don't recall either.  Let's remove it.

> As long as popup_message is changing anyhow, I think that "Syntax Error" is a
> better name than "Script Error".  I am trying to move away from the term
> "script", at least in user documentation and user-visible messages, because it
> is not a term used by that other software.


OK.

> I assume that only the ascii driver cares about terminal size?


Now that you mention it, it needs a bit of thought. The manual for 'that other software' says "These two subcommands apply only to text output".  So it seems that it shouldn't apply to postscript or html drivers.   In the case of an interactive PSPP session, there are two drivers active --- one going to the terminal, the other to pspp.list Common sense says that resizing the terminal should only affect the dimensions of the terminal output.  So maybe we need to think about this a bit more.

John Darrington <jmd>
Project Administrator
Wed 19 Sep 2007 03:33:55 AM UTC, comment #2: 

>What's not been done, is to get the output engine to respect these
>values. It only looks at them at startup. I don't know how much effort
>would be involved to allow it to dynamically reconfigure itself...


I can prepare a patch, if you like.  I assume that only the ascii
driver cares about terminal size?

Ben Pfaff <blp>
Project Administrator
Wed 19 Sep 2007 03:31:17 AM UTC, comment #1: 

I think that the new version of get_termcap_viewport calls set_fallback_viewport unconditionally, not just when tgetnum fails to return good values.

I do not think it is a good idea to do so much work from a signal handler.  It would be better if the signal handler could just set a flag that higher-level code could notice and act upon.  I think that this would require using a select(2) loop and rl_callback_read_char in readln_read in place of the current simple call to readline or ds_read_line.  I don't know whether you are familiar with the care and feeding of such loops: if not, I'd be happy to code one for you.

The long_view variable appears to be effectively dead: although force_long_view can set it to true, there is no call to force_long_view anywhere in the source tree.  I don't recall what it was originally for.

As long as popup_message is changing anyhow, I think that "Syntax Error" is a better name than "Script Error".  I am trying to move away from the term "script", at least in user documentation and user-visible messages, because it is not a term used by that other software.

Ben Pfaff <blp>
Project Administrator
Wed 19 Sep 2007 02:32:32 AM UTC, original submission:  

This patch solves half the problem.  I've moved get_termcap_viewport to src/ui/terminal/main.c, made it a handler for SIGWINCH, and done the equivalent work in the GUI.
So now SHOW LENGTH and SHOW WIDTH will tell you how big your window is.

What's not been done, is to get the output engine to respect these values. It only looks at them at startup.  I don't know how much effort would be involved to allow it to dynamically reconfigure itself...

John Darrington <jmd>
Project Administrator

 

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

Attached Files
file #14026:  viewport-gui.patch added by jmd (4KiB - text/x-diff)
file #13986:  viewport.patch added by jmd (15KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by blp (Posted a comment)
  • -email is unavailable- added by jmd (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.

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-09-26 blp StatusReady For Test/Review Done
        Open/ClosedOpen Closed
    2007-09-25 jmd Attached File- Added viewport-gui.patch, #14026
    2007-09-25 blp Attached File- Added viewport-happy-signal-handler-2.patch, #14025
    2007-09-24 blp Attached File- Added viewport-fallback-obviousness.patch, #14016
        Attached File- Added viewport-dont-call-it-a-script.patch, #14017
        Attached File- Added viewport-drop-long-view.patch, #14018
        Attached File- Added viewport-happy-signal-handler.patch, #14019
    2007-09-19 jmd Attached File- Added viewport.patch, #13986

    Back to the top


    Powered by Savane 3.9