GNOME Bugzilla – Bug 660236
Paging for long text in folks-inspect
Last modified: 2012-07-23 23:37:53 UTC
If you had automatic paging like git that'd be cool. I think Raul was working on this a while ago.
Created attachment 197612 [details] [review] patch This is WIP and RFC mostly.
Review of attachment 197612 [details] [review]: Looks like a nice approach, though I haven't had a chance to test it yet. A few comments. ::: tools/inspect/command-backends.vala @@ +52,3 @@ public override void run (string? command_string) { + Utils.start_paged_output (); Might make more sense to move the [start|stop]_paged_output() calls to Inspect.Client.run_interactive(). This would also fit in with bug #657063. ::: tools/inspect/utils.vala @@ +53,3 @@ + Utils.stop_paged_output (); + + Utils._pager = FILE.popen ("less", "w"); How well does `less` cope with outputting colour escape sequences? I realise that folks-inspect doesn't use them at the moment, but I've always wanted to add them. Also, how does piping the output through `less` change the behaviour of signals? Not having to press ‘q’ to get back to the folks-inspect command line would be good if possible. Bear in mind bug #660235. @@ +137,3 @@ + } + catch (GLib.ConvertError e1) {} + catch (GLib.IOChannelError e2) {} Error handling needs implementing.
(In reply to comment #2) > How well does `less` cope with outputting colour escape sequences? I realise > that folks-inspect doesn't use them at the moment, but I've always wanted to > add them. Well, the debug command uses them. > > Also, how does piping the output through `less` change the behaviour of > signals? Not having to press ‘q’ to get back to the folks-inspect command line > would be good if possible. Bear in mind bug #660235. > > @@ +137,3 @@ > + } > + catch (GLib.ConvertError e1) {} > + catch (GLib.IOChannelError e2) {} > > Error handling needs implementing. I am deliberatively swallowing the errors, I guess I can make a comment of why we don't care about them (i.e.: less exit when data was still available).
(In reply to comment #2) > + Utils._pager = FILE.popen ("less", "w"); Please respect $PAGER!
Created attachment 198108 [details] [review] patch Updated according to reviews.
Review of attachment 198108 [details] [review]: ::: NEWS @@ +4,3 @@ * Bug 660217 — folks-0.6.3.2 requires tracker-0.12, but configure.ac calls VALA_CHECK_PACKAGES([tracker-sparql-0.12]) +* Bug 660236 — Paging for long text in folks-inspect Double space. ::: tools/inspect/utils.vala @@ +63,3 @@ + Utils._pager = FILE.popen (pager, "w"); + Utils._io_channel = new IOChannel.unix_new (Utils._pager.fileno ()); + Utils._page_output = true; How about not using a pager if PAGER == ""? At the moment, if I run `PAGER= folks-inspect` I can't see any paged output.
Review of attachment 198108 [details] [review]: With this patch (and not with master), folks-inspect leaves the shell in a bad state. Typing doesn't echo anything back and when I run subsequent shell commands, they don't get echoed either (so it's also like "set -v off" gets set). The only way to get my shell back in a decent state is with "reset", so we obviously need to fix this before merging the patch. I guess we're not cleaning up the I/O settings properly. This may be specific to 'less', since I don't hit this problem if I run folks-inspect with PAGER="more" or PAGER="". Though I still hit Philip's problem with PAGER="" printing no output (within folks-inspect). Maybe you just haven't pushed those fixes to your public branch yet. Other than that, some minor typos: + /* If the output is not a TTY (cause its a pipe or a file or a + * toaster) we don't page. */ s/cause its/because it's/ + /* In case the previous clean up wasn't finish */ s/finish/finished/
Created attachment 218447 [details] [review] Redone to use GLib.Process, rebased on master, quit doesn't page. I reworked Raul's branch a bit. rebased it on master, changed it from using FILE.popen to using GLib.Process since FILE objects aren't pclose()'d in vala yet. That didn't fix the shell echo issue, but not paging the quit command seems to here.
Review of attachment 218447 [details] [review]: Just a reminder from IRC: This needs to be tested with: • Pressing Ctrl+D from within folks-inspect. • Killing folks-inspect from a different terminal window. We need to ensure we actually (gracefully) kill the pager process before we quit (main_client.quit()). ::: tools/inspect/inspect.vala @@ +286,3 @@ + int status; + if (command_name != "quit") + Utils.start_paged_output (); It might be neater to add a property to the Folks.Inspect.Command class saying whether the output should be paged. Also, please use braces around single-line ‘if’ blocks. ::: tools/inspect/utils.vala @@ +32,3 @@ + /* To page or not to page? */ + private static bool _page_output = false; + private static IOChannel _io_channel; This needs to be nullable. @@ +53,3 @@ + { + /* If the output is not a TTY (cause its a pipe or a file or a + * toaster) we don't page. */ Travis’ typo fixes from comment #10 don’t seem to have made it into this branch. @@ +68,3 @@ + try + { + GLib.Shell.parse_argv(pager, out args); Missing space before the ‘(’. @@ +72,3 @@ + catch (GLib.ShellError e) + { + debug ("Error parsing pager arguments"); What happens if we reach here? args will be empty, and then we call spawn_async_with_pipes() on it. This should probably also be a warning() rather than a debug(). @@ +87,3 @@ + out Utils._pager_fd, // Std input + null, // Std out + null); // Std error This should really all be indented 4 spaces past the “GLib.” only. @@ +104,3 @@ + catch (GLib.Error e) + { + debug ("Error shutting down io channel %s", e.message); s/io channel /IO channel: / This should probably be a warning() rather than a debug(). @@ +190,3 @@ + } + else + GLib.stdout.printf (str); Missing braces!
Created attachment 219341 [details] [review] Add paging to folks Updated the branch and patch after Philip's review. I'm wondering if we should ignore the GLib.error when Utils._io_channel.shutdown (true); fails, since it fails if we quit the pager with a broken pipe error. Also, killing folks-inspect still doesn't kill the pager process. I'm wondering if we should merge this since it is a huge improvement and file a new bug for the pager not getting killed when we kill the folks-inspect process.
Created attachment 219360 [details] [review] Add paging to folks (updated) https://www.gitorious.org/folks/folks/commits/660236-paging Here’s a heavily modified version which seems to work in all the cases I can throw at it, but still needs some more testing by someone more inventive/mischievous. The main two changes are: • Removed wait() in favour of a GChildWatch. • Changed it to save/restore the user’s terminal attributes on startup and quitting.
Review of attachment 219360 [details] [review]: philip, This seems to work well when I test here. I still get the problem of killing folks-inspect from a different terminal doesn't kill the pager, but that's what I had in the previous patch also. Is that working for you there btw?
Comment on attachment 219360 [details] [review] Add paging to folks (updated) Committed to master. Killing folks from a different terminal doesn’t kill the pager for me, but everything’s cleaned up properly when you do quit the pager yourself, which I think is acceptable. We do send SIGTERM to the pager, but I guess it has its fingers in its ears. commit 19f77e2a6ca09fb74635c5387bd2e9a98aa7bfe3 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Sat Jul 21 01:25:01 2012 +0100 Bug 660236 — Paging for long text in folks-inspect Use $PAGER to allow paging of the output of really long commands in folks-inspect. Based on work by Raúl Gutierrez Segales and Jeremy Whiting. Closes: https://bugzilla.gnome.org/show_bug.cgi?id=660236 NEWS | 1 + tools/inspect/Makefile.am | 1 + tools/inspect/inspect.vala | 320 ++++++++++++++++++++++++++++++++++++-------- tools/inspect/utils.vala | 6 +- 4 files changed, 268 insertions(+), 60 deletions(-)