[2644] in BarnOwl Developers
Re: RFC: popless-search
daemon@ATHENA.MIT.EDU (David A Benjamin)
Sat Sep 18 16:51:34 2010
Date: Sat, 18 Sep 2010 16:51:31 -0400 (EDT)
From: David A Benjamin <davidben@MIT.EDU>
To: Nelson Elhage <nelhage@mit.edu>
cc: barnowl-dev@mit.edu
In-Reply-To: <87eicq9798.fsf@mit.edu>
On Sat, 18 Sep 2010, Nelson Elhage wrote:
> 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.
Eh, it was rather on the large side. Next time I'll try to break these
things up into manageable chunks. This patchset could certainly have been
done in several rounds.
> 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.
Yeah. Actually, I bet many of commands marked interactive that setup
initial state in the recwin (such as view) would be problematic in
peoples' startup files. We probably want to go through and audit all the
context flags more thoroughly than I apparently did.
> 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?
Well, it sets a delete_cb to unref the editwin. The deactivate_cb just
detaches the editwin in the manner appropriate for this instance. The
distinction was so that contexts can stick around a little after they've
been deactivated, although it looks like I ended up refcounting the
editwin instead for the callback and not completely finishing this part: I
think it's a little poor that commands which take a context can have it
disappear under their feet when they pop it. I'd rather the contexts also
be refcounted (or something) so that they're guaranteed to stay alive for
the duration of the command. If nothing else, this should ease future perl
bindings.
As far as the API of editcontext itself, yeah I really don't like it. Not
really sure what it should look like, although I did kind of want to avoid
a mess of parameters. I suspect something smelling like subtyping is
wanted here. Eh, it looks like every call to owl_editcontext_new does set
the deactivate_cb afterwards, so I'll make it an argument.
> 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.
Sure. Although we can't search multi-line right now can we? I was under
the impression the UI doesn't even let you pass a regex.
For comparison, I believe less maintains an array of lines and regex
either matches a line or doesn't. We probably want to do that at some
point.
> 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.
I like consider_current. :-)
> 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.
Mmm. Yeah, good point. I thought about perl code, but I clearly didn't
catch getstyle, etc. I'll drop this patch until we figure out later what
we actually want. I primarily wanted that done for popless:start-command's
editwin, but since that keybinding's disabled by default, I'm okay with it
acting really weird.
> 9b31c05 Add BarnOwl::Internal::_valid_in_parent_context hack
>
> Why "parent" context? Why not "current" context or something?
The hack is intended for tab-completion. Again, less interesting now that
popless:start-command is disabled, although it'd be nice to not offer
popless:* and edit:* in the main commandline's completion. Anyway, the
reason is that the editwin callback runs in the parent context. If it were
the current context, then we'd complete edit:*. (Really we probably want
contexts or something like that accessible via perl, but that's later.)
On the other hand, it makes tab-completion for bindkey less nice, which I
noticed recently. I'm happy to drop this as well, since it's a nasty hack.
We could also not have the filtering in bindkey, but... that's more work.
:-)
David
> - 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
>>
>
