[2643] in BarnOwl Developers
Re: RFC: popless-search
daemon@ATHENA.MIT.EDU (Nelson Elhage)
Sat Sep 18 15:33:26 2010
From: Nelson Elhage <nelhage@MIT.EDU>
To: David A Benjamin <davidben@mit.edu>, barnowl-dev@mit.edu
In-Reply-To: <alpine.DEB.1.10.1008160313290.12496@dr-wily.mit.edu>
Date: Sat, 18 Sep 2010 15:33:23 -0400
Whoo. Sorry it took me a month to get to this. Awesome on the whole. I'm
really glad for most of this cleanup, and to finally land this
feature. A few small thoughts from the code. Feel free to ignore any you
think don't make sense.
25a69b5 Check contexts on all commands
This makes me slightly nervous, but hopefully any bugs will come out in
pre-release testing. And it's a change I agree we should make.
934edb5 Configure owl_global_set_typwin_inactive to be called on context deactivate
ctx = owl_editcontext_new(OWL_CTX_EDITMULTI, e, "editmulti");
+ ctx->deactivate_cb = owl_global_deactivate_editcontext;
+ ctx->cbdata = &g;
owl_global_push_context_obj(&g, ctx);
The ~whole point of editcontext_new() is that it sets a deactivate_cb
that unref's the editwin, right? But now we're overriding that
immediately in the caller. Would it make sense to pass in an optional
cleanup cb into owl_editcontext_new, or something?
c6e7583 Add an unspeakably ugly version of reverse search
Hm. This will break on multi-line regexes, right? That's probably fine
for now, but let's keep it in mind if we ever do clean it up. Maybe add
that note to the comment.
07ea6dc Add OWL_SEARCH_CONTINUE and OWL_SEARCH_MATCH_CURRENT
Maybe instead of those magic constants, rename the parameter to 'bool
consider_current' or 'bool continue'? I'm not sure there's a need for
specific flag values for something that's a pretty straightforward
boolean.
b5547d1 Disable a number of commands outsite the recwin
This one is going to need some thought.
- OWLCMD_ARGS("getview", owl_command_getview, OWL_CTX_INTERACTIVE,
+ OWLCMD_ARGS("getview", owl_command_getview, OWL_CTX_INTERACTIVE|OWL_CTX_RECWIN,
"returns the name of the filter for the current view",
"", ""),
- OWLCMD_ARGS("getfilter", owl_command_getfilter, OWL_CTX_INTERACTIVE,
+ OWLCMD_ARGS("getfilter", owl_command_getfilter, OWL_CTX_INTERACTIVE|OWL_CTX_RECWIN,
"returns the definition of a filter",
"getfilter <filtername>", ""),
- OWLCMD_ARGS("getstyle", owl_command_getstyle, OWL_CTX_INTERACTIVE,
+ OWLCMD_ARGS("getstyle", owl_command_getstyle, OWL_CTX_INTERACTIVE|OWL_CTX_RECWIN,
"returns the name of the style for the current view",
"", ""),
Hm. These are all ~often called by perl code, so I worry about making
them uncallable because a popwin is open.
zwrite and zcrypt are probably not a huge issue, since I think code can
workaround using zephyr_zwrite, but they also worry me.
I suspect we want to have a distinction between calling commands from
code and interactively. But until we do, we need to think about code
calling these, as well as users.
9b31c05 Add BarnOwl::Internal::_valid_in_parent_context hack
Why "parent" context? Why not "current" context or something?
- Nelson
On Mon, 16 Aug 2010 03:49:38 -0400 (EDT), David A Benjamin <davidben@MIT.EDU> wrote:
> [WARNING: Long email]
>
> Hi all,
>
> So, I've been recently working on adding support for searching in the
> popless; since we've got owl_window and all, it seems silly not to show it
> off in a release. (So says the guy who wrote it...) Anyway, I'm at the
> point that popless searching works (but needs polish) without exceedingly
> awful hacks, and I'd like input on it.
>
> First, a summary of the interesting changes in the branch:
>
> - The viewwin and popwin are now allocated via new and delete methods. New
> viewwins and popwins are created instead of reusing old ones.
>
> - The editwin is now ref-counted, to help with both owl_global and the
> edit context wanting a stake in its ownership. This will also make it
> easier to export into perl as an object in the future. In particular, we
> no longer need the funny thing where owl_global_set_typwin_active expects
> the caller to free the editwin.
>
> - Routines are added to search in the viewwin akin to less's behavior. The
> implementation for searching in reverse is extremely ugly; I think what we
> want is to change the internal representation to a list of lines. (This is
> what less does internally.)
>
> - Contexts now have a bare minimum of hooks so that they may own their
> objects. It's extremely sketchy for now. I'm not sure how this should
> look, so I just implemented what was needed to do what I wanted and
> nothing more. Specifically, they now have deactivate and delete hooks. The
> former makes the editwin inactive so edit:done needn't know how. The
> latter releases a reference to the editwin.
>
> - The viewwin/popwin now has an embeddable editwin to act as a
> command-line. Commands popwin:start-command and popwin:search are added
> that do what you'd expect.
>
> - To avoid people being able to do strange things when the popwin is up,
> various commands are marked to require OWL_CTX_RECWIN. To that effect, we
> actually check the context mask on non-CTX commands instead of blatantly
> ignoring them. (And to that end, the definition of OWL_CTX_ANY is revised
> to not be a lie.)
>
> - An internal API is added for the completion system (until we properly
> export contexts to perl) so that it may limit command offerings to
> commands valid in the current context.
>
> I'm not totally happy with all of it, but I want others' input at this
> point. In particular, the points I think are most sketchy are:
>
> - Contexts are a mess. While this doesn't make them appreciably more
> annoying than before, it's still not pleasant. Long-term, I want to
> completely revamp this thing (and stuff the cmddict or some equivalent
> into the context), but short-term?
>
> - On a related note, commands are kind of unpleasant right now.
> popless:search just doesn't look very good, and there are several commands
> probably shouldn't be valid in popless' context. Perhaps ideally, search,
> start-command, etc., should be defined as appropriate for the particular
> context. I've OWL_CTX_RECWIN'd the commands that I think plain don't make
> sense outside the recwin, but some others, like :show or :help aren't
> particularly recwin-specific but don't work well while the popless is
> open. They won't crash, but some will, say, give an error that you can't
> open a second popless.
>
> - On yet another related note, commands which output to the msgwin don't
> work well when the popwin is open; you can't see the text. Not sure what
> the nicest way to do that is...
>
> - The code to swap out the viewwin's status bar and command line windows
> is quite ugly. It is nicer than the equivalent code for the editwin
> though. I think there should be some kind of 'owl_window container' which
> may show at most one owl_window at a time. Windows may size them in their
> layout code and allow other windows to plug into them. I'll probably work
> on this next.
>
> So... any thoughts? I think it'd be nice if we could ship with a
> full-fledged command line on the popwin. If that's not easy to polish, a
> possibility would be to punt most of that and not bind : and M-x in the
> popwin. Instead, / and ? could be bound to something like
> popwin:start-search which would only allow for searching. (Hrm, we may
> want to do something about the tab-completion?) We may want to do that
> anyway to be like less and show a '/' or '?' as the locktext.
>
> I made some effort to make the git history readable this time, so
> hopefully review will not be too painful (unlike owl_window :D).
>
> The following changes since commit d40ad8b64f56d54882ecd4e4c254f1a3c9f101d6:
>
> Punt OWL_WEBBROWSER_* and userclue (2010-08-07 18:06:17 -0700)
>
> are available in the git repository at:
> git://github.com/davidben/barnowl.git popless-search
>
> David Benjamin (43):
> Make owl_fmtext_search take and return an offset
> Honor the do_search argument to _owl_fmtext_curs_waddstr
> Update the viewwin when the global search string changes
> Add owl_fmtext_line_number and test.
> Add a test case for owl_fmtext_search
> Don't cause confusion if owl_popwin_up on an active popwin
> If owl_popwin_up fails, abort the command
> Implement owl_fmtext_line_extents and add test case
> Actually set the context interactive in the startup routine
> Redefine OWL_CTX_ANY to be all 0s, not all 1s
> To be thorough, track all signal ids in the popwin
> Check contexts on all commands
> Create a new owl_popwin every time instead of rewiring the old one
> Don't leak an owl_popexec if fork or pipe fail
> Don't export owl_viewwin_set_window.
> Delete the popwin when closing it
> Likewise, don't reuse a global owl_viewwin
> Add owl_viewwin_up and owl_viewwin_down
> Never move the screen up when during an owl_viewwin_down
> Draw ~s in empty viewwin lines, to mimic less
> Add half an implementation of owl_viewwin_search
> Rearrange some viewwin functions
> Split the viewwin's status bar into its own window
> owl_editwin should own a reference to its window
> Make owl_global_get_typwin return the context's typwin
> Rename owl_global_get_typwin to owl_global_current_typwin
> Add deactivate and delete hooks for owl_context
> Add a separate owl_context_new and remove owl_context_init
> Add owl_editwin_get_window
> Ref-count the editwin
> Use ref-counting to make editwin ownership saner
> Configure owl_global_set_typwin_inactive to be called on context deactivate
> Add an unspeakably ugly version of reverse search
> Use bool instead of int in owl_window
> Make the check for _owl_window_realize more readable
> Delay realizing a window until we need it for painting
> Add a command-line to the viewwin
> Add owl_function_set_search
> Just remove owl_function_search_{start,continue} altogether
> Add OWL_SEARCH_CONTINUE and OWL_SEARCH_MATCH_CURRENT
> FINALLY! Add a popless:search command
> Disable a number of commands outsite the recwin
> Add BarnOwl::Internal::_valid_in_parent_context hack
>
> Makefile.am | 2 +-
> cmd.c | 8 +-
> commands.c | 123 ++++++++-----
> context.c | 34 +++-
> editcontext.c | 32 ++++
> editwin.c | 24 +++-
> fmtext.c | 52 +++++-
> functions.c | 74 +++++---
> global.c | 82 ++++++----
> keys.c | 12 +-
> message.c | 2 +-
> owl.c | 2 +-
> owl.h | 36 +++-
> perl/lib/BarnOwl/Complete/Client.pm | 4 +-
> perl/lib/BarnOwl/Completion.pm | 3 +-
> perlglue.xs | 41 ++++--
> popexec.c | 18 ++-
> popwin.c | 53 ++++--
> tester.c | 46 +++++-
> viewwin.c | 325 ++++++++++++++++++++++++++++-------
> window.c | 32 +++-
> window.h | 20 ++-
> 22 files changed, 772 insertions(+), 253 deletions(-)
> create mode 100644 editcontext.c
>
