[2645] in BarnOwl Developers
Re: RFC: popless-search
daemon@ATHENA.MIT.EDU (David A Benjamin)
Sat Sep 18 20:37:40 2010
Date: Sat, 18 Sep 2010 20:37:36 -0400 (EDT)
From: David A Benjamin <davidben@MIT.EDU>
To: Nelson Elhage <nelhage@mit.edu>
cc: barnowl-dev@mit.edu
In-Reply-To: <alpine.DEB.2.00.1009181538150.9932@superconducktor>
On Sat, 18 Sep 2010, David A Benjamin wrote:
>> 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.
Thinking about this some more, I wonder if this makes much sense in the
first place. Why shouldn't people be allowed to call, say, help and then
popless:quit in their startup file? Or maybe just help if you want to
display that on startup for whatever reason.
Contexts are certainly useful for things like "what editwin are you
talking about" and "is there an editwin active at all", but I don't think
there's much point in restricting what happens before/after startup. It
doesn't look like there's anything that hasn't already been initialized at
that point, or am I missing something?
I'm thinking we want to just punt all the context-filtering related
commits and push popless-search without them. I've reflected that in my
popless-search branch and put the context stuff on top in the branch
context-work.
David
>> 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
>>>
>>
>
