[2655] in BarnOwl Developers
Re: RFC: popless-search
daemon@ATHENA.MIT.EDU (Nelson Elhage)
Sun Sep 19 19:04:10 2010
From: Nelson Elhage <nelhage@MIT.EDU>
To: David A Benjamin <davidben@mit.edu>
Cc: barnowl-dev@mit.edu
In-Reply-To: <alpine.DEB.2.00.1009182022030.9932@superconducktor>
Date: Sun, 19 Sep 2010 19:04:05 -0400
I think that's probably reasonable. I suspect we'll want to have some
context restriction in the future, but it may want to be combined with
some notion of calling a command interactively vs. programmatically.
- Nelson
On Sat, 18 Sep 2010 20:37:36 -0400 (EDT), David A Benjamin <davidben@MIT.EDU> wrote:
> 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
> >>>
> >>
> >
>
