Saturday, May 27, 2017

Re: Bug/non-determinism in output of maparg() and map commands

2017-05-27 20:29 GMT+03:00 Brett Stahlman <brettstahlman@gmail.com>:
> On Sat, May 27, 2017 at 11:51 AM, Nikolay Aleksandrovich Pavlov
> <zyx.vim@gmail.com> wrote:
>> 2017-05-27 19:32 GMT+03:00 Brett Stahlman <brettstahlman@gmail.com>:
>>> On Sat, May 27, 2017 at 10:39 AM, Nikolay Aleksandrovich Pavlov
>>> <zyx.vim@gmail.com> wrote:
>>>> 2017-05-27 18:02 GMT+03:00 Brett Stahlman <brettstahlman@gmail.com>:
>>>>> On Sat, May 27, 2017 at 8:35 AM, Nikolay Aleksandrovich Pavlov
>>>>> <zyx.vim@gmail.com> wrote:
>>>>>> 2017-05-27 12:45 GMT+03:00 Bram Moolenaar <Bram@moolenaar.net>:
>>>>>>>
>>>>>>> Nikolay Pavlov wrote:
>>>>>>>
>>>>>>>> 2017-05-26 20:43 GMT+03:00 Bram Moolenaar <Bram@moolenaar.net>:
>>>>>>>> >
>>>>>>>> > Brett Stahlman wrote:
>>>>>>>> >
>>>>>>>> >> >> On Tuesday, May 23, 2017 at 8:25:33 AM UTC-5, Brett Stahlman wrote:
>>>>>>>> >> >> > On Tue, May 23, 2017 at 4:35 AM, Bram Moolenaar <Bram@moolenaar.net> wrote:
>>>>>>>> >> >> > >
>>>>>>>> >> >> > > Brett Stahlman wrote:
>>>>>>>> >> >> > >
>>>>>>>> >> >> %--snip--%
>>>>>>>> >> >> > >
>>>>>>>> >> >> > > The best solution is probably to also add the raw rhs, with the terminal
>>>>>>>> >> >> > > codes replaced. This won't work when changing the terminal type, but
>>>>>>>> >> >> > > that is very unlikely to happen.
>>>>>>>> >> >> >
>>>>>>>> >> >> > You mean adding a key such as "raw_rhs" to the dictionary returned by
>>>>>>>> >> >> > maparg()? If so, then yes this would help, but there would still need to
>>>>>>>> >> >> > be a way to determine lhs, which is currently even more ambiguous than
>>>>>>>> >> >> > rhs. While it's true that I probably already have lhs if I'm calling
>>>>>>>> >> >> > maparg(), I need a way to determine which lhs(s) is/are ambiguous with a
>>>>>>>> >> >> > given lhs. Mapcheck() gives me only the rhs of the conflicting map. To
>>>>>>>> >> >> > save and restore, I'd need to know the lhs in canonical form as well.
>>>>>>>> >> >>
>>>>>>>> >> >> Perhaps mapcheck() could take an optional arg requesting something more than a simple boolean return. When called with this extra arg, mapcheck() could return a conflicting/ambiguous lhs (or list thereof) in some canonical format (possibly determined by the value of the extra arg itself). As long as the format returned could be fed to maparg(), it would be possible to find conflicting mappings, remove them temporarily, and subsequently restore them...
>>>>>>>> >> >
>>>>>>>> >> > If you define a mapping you will want to know whether the mapping
>>>>>>>> >> > already exists and needs to be restored. For that you can use maparg(),
>>>>>>>> >> > no need to use mapcheck().
>>>>>>>> >> >
>>>>>>>> >> > Not sure why you would want to remove "conflicting" mappings. Perhaps
>>>>>>>> >> > when you map the ; key, and the user has ;x mapped? Then you would need
>>>>>>>> >> > a list. Adding a maplist() function would be better than adding
>>>>>>>> >> > arguments to mapcheck().
>>>>>>>> >>
>>>>>>>> >> Yes. Very much like that. I'm implementing a sort of transient mode, in
>>>>>>>> >> which I'll "shadow" existing maps with very short (generally single
>>>>>>>> >> character) mappings, which are expected to be ambiguous/conflicting with
>>>>>>>> >> existing maps, and even builtin operators. Of course, when I exit the
>>>>>>>> >> transient mode, I'd need to restore the mappings that were shadowed.
>>>>>>>> >>
>>>>>>>> >> The global and builtin maps are not a problem, since the transient maps use
>>>>>>>> >> <buffer> and <nowait>; however, without parsing the output of one of the :map
>>>>>>>> >> functions, I have no way of knowing which buf-local mappings will be ambiguous
>>>>>>>> >> with the transient maps I'm defining. And parsing the :map output is
>>>>>>>> >> problematic for the reasons already mentioned: e.g., no way to tell the
>>>>>>>> >> difference between function key <F8> and the corresponding 4 characters. I'd
>>>>>>>> >> actually considered taking some sort of iterative approach: e.g., trying all
>>>>>>>> >> possible permutations of lhs as input to maparg() and testing the results, in
>>>>>>>> >> an attempt to deduce the canonical form, but this would be extremely messy,
>>>>>>>> >> and I don't even know whether it would be deterministic... The maplist()
>>>>>>>> >> function you mentioned, if it returned all ambiguous left hand sides in
>>>>>>>> >> canonical form, or even a list of the corresponding maparg()-style
>>>>>>>> >> dictionaries, would be perfect. Of course, there would also need to be a way
>>>>>>>> >> to get the rhs's canonical form: e.g., the extra "raw_rhs" key in the maparg()
>>>>>>>> >> or maplist() dictionary.
>>>>>>>> >
>>>>>>>> > OK, so for this you would use maplist() to get the list of mappings to
>>>>>>>> > disable, use maparg() to get the current mapping, clear the mapping, do
>>>>>>>> > your stuff, then restore the cleared mappings. You then need to make
>>>>>>>> > sure you restore the mappings exactly as they were, even when your
>>>>>>>> > "stuff" fails miserably.
>>>>>>>> >
>>>>>>>> > It's a lot easier if we would have a way to temporarily disable
>>>>>>>> > mappings. It's mostly the same as above, but you won't need to use
>>>>>>>> > maparg() to get the current mapping and the restore operation. Instead
>>>>>>>> > you would disable instead of clear, and later re-enable instead of
>>>>>>>> > restore. Still need to make sure the re-enbling does happen, no change
>>>>>>>> > in that part.
>>>>>>>>
>>>>>>>> Not sure I understood what exactly you suggest to disable/restore. All
>>>>>>>> mappings at once with one command? I would actually disagree here: I
>>>>>>>> need something similar for translit3, but it only remaps
>>>>>>>> single-character mappings, leaving most of other user mappings alone.
>>>>>>>> One mapping at a time? It would be good, but given that request is
>>>>>>>> temporary remapping naming the functionality enable/disable looks
>>>>>>>> strange. And there are still issues with determining {lhs}.
>>>>>>>
>>>>>>> Let's use an example: Suppose a plugin has a special mode for entering
>>>>>>> data (e.g. chemical formulas). It would then map some keys, e.g. "a".
>>>>>>> If the user already has a mapping for "a" it needs to be restored when
>>>>>>> leaving the special mode. If the user has mappings starting with "a" we
>>>>>>> would like to disable those, to avoid the timeout waiting for the next
>>>>>>> character.
>>>>>>>
>>>>>>> We do not want to disable mappings that don't interfere, to maximise the
>>>>>>> freedom for the user to use other mappings at the same time.
>>>>>>>
>>>>>>>> One of the logical variants would be `:map <push> {lhs}
>>>>>>>> {new-rhs}`/`:unmap <push> {lhs}`+`:map <pop> {lhs}`, but this is hard
>>>>>>>> to implement and is rather limited, though less limited then
>>>>>>>> enable/disable everything variant.
>>>>>>>
>>>>>>> This quickly gets complicated if we need to take into account all the
>>>>>>> possible modes a mapping can be used in.
>>>>>>>
>>>>>>>> I would instead suggest a function mappings_dump()/mappings_add():
>>>>>>>> first is similar to `nvim[_buf]_get_keymap` and should dump all
>>>>>>>> mappings as a list of maparg()-like dictionaries. Second should define
>>>>>>>> mappings being given a list of them. Of course, this means that
>>>>>>>> dictionaries need to be fixed to allow correctly saving/restoring.
>>>>>>>>
>>>>>>>> The advantages:
>>>>>>>>
>>>>>>>> 1. Easier to implement. Code for creating a maparg() dictionary is
>>>>>>>> already there, iterating over all mappings is not a problem. Results
>>>>>>>> needs to be incompatible with maparg() or use additional keys though:
>>>>>>>> e.g. Neovim altered the contents of `noremap` and `buffer` keys: first
>>>>>>>> is now 0, 1 or 2 (you can't correctly restore a mapping if you can't
>>>>>>>> distinguish `map <script>` and `noremap`) and second is a buffer
>>>>>>>> number or zero.
>>>>>>>> 2. More flexible: you can save and restore everything, push or pop
>>>>>>>> individual mappings, create a temporary mapping which is just like
>>>>>>>> mapping X, but has `<Plug>(Translit3TemporaryMap)` lhs instead (to be
>>>>>>>> returned from `<expr>` mappings in order to select either plugin
>>>>>>>> behaviour or fall back to previously present user mapping instead).
>>>>>>>>
>>>>>>>> I can imagine other usages enable/disable or push/pop could not
>>>>>>>> achieve: generating configuration with mappings like :mkvimrc, but
>>>>>>>> allows doing adjustments (parsing `:mkvimrc` output is not fun,
>>>>>>>> especially if you want to be forward compatible), creating a plugin
>>>>>>>> which analyses how often different mappings are used (need to copy all
>>>>>>>> mappings to temporary then replace existing mappings with plugin
>>>>>>>> ones).
>>>>>>>> 3. This is also forward compatible: just need to state in the
>>>>>>>> documentation that new significant keys may be added in the future to
>>>>>>>> the dictionaries so they should be preserved.
>>>>>>>
>>>>>>> I don't see much use for this. I can't think of a practical example how
>>>>>>> a plugin manipulates mappings it didn't create itself or even knows what
>>>>>>> they are for.
>>>>>>
>>>>>> Still Vim has :mkvimrc which does manipulate (dump) mappings from
>>>>>> third-party plugins. Also I need this functionality for some <expr>
>>>>>> mappings: if some condition is true (e.g. `>` is preceded by `-` (C,
>>>>>> completion) or transliteration mode was enabled, or transliteration
>>>>>> mode is enabled *and* character that does not start a new
>>>>>> transliteration sequence is a continuation of previous one) use plugin
>>>>>> mapping. If it is false, fall back to whatever was there previously,
>>>>>> including falling back to whatever mapping was there previously.
>>>>>>
>>>>>> Also check https://github.com/neovim/neovim/issues/6123, this is the
>>>>>> issue backing Neovim nvim_get_keymap() API function.
>>>>>>
>>>>>>>
>>>>>>> Another complication is that mappings can be added/removed by other
>>>>>>> mappings and by autocommands.
>>>>>>
>>>>>> I do not see how this complication is relevant to the discussion. I.e.
>>>>>> I do not see how this complication should affect usage or
>>>>>> implementation of both proposed changes.
>>>>>>
>>>>>>>
>>>>>>> Disabling and re-enabling mappings is definitely more efficient than
>>>>>>> removing and adding-back mappings.
>>>>>>
>>>>>> And it is also definitely both harder to implement and less flexible.
>>>>>
>>>>> Harder to implement, perhaps, but not necessarily less
>>>>> flexible. Though the discussion thus far has centered mostly
>>>>> on enable/disable functionality, there's nothing about the map
>>>>> handle interface that limits it to this. It could support
>>>>> query and execute functions, for instance. For cases in which
>>>>> you wish to keep the original behavior, but need to "wrap" it
>>>>> somehow, you could use the map handle to attach prolog/epilog
>>>>> callback functions to a map. Presumably, such callback
>>>>> functions (which could be either lambdas or funcrefs) would
>>>>> accept an argument that allowed them to obtain information
>>>>> about the original map, possibly even its exact lhs and rhs.
>>>>> The prolog callback would be even more useful if Vim provided
>>>>> a way (e.g., nonzero return) for it to abort the original map.
>>>>
>>>> Enable, disable, query, execute plus two callbacks. *Four* functions
>>>> and two callbacks in place of just two simple functions, mostly using
>>>> the functionality that is already there. Five if you remember about
>>>> :mkvimrc and that somebody may want to replace that on top of new API:
>>>> query will need a mirror function for creating a mapping then.
>>>>
>>>> This is going to introduce a big amount of bugs just to add the
>>>> flexibility which is naturally available through a much simpler
>>>> approach. Emulating everything you mention on top of current VimL
>>>> state plus mappings_dump()/mappings_load() / (mappings_clear()*) is
>>>> not going to make plugins considerably slower (as long as you can
>>>> operate on lists and use `map()`/`filter()`/etc: main VimL
>>>> optimization principle is "the less Ex commands the faster the code")
>>>> and I do not see any other benefits, except for "with some handles
>>>> implementation it may be slightly easier to pinpoint third-party
>>>> plugins' bugs".
>>>>
>>>> * Found an issue in my proposal: `:execute 'unmap'` would not be easy
>>>> or efficient to use, so additionally need either `mappings_clear({list
>>>> to clear})` or make `mappings_load()` unmap mappings when rhs key is
>>>> missing.
>>>>
>>>>>
>>>>> Hmm... This may be overkill, but it might even be possible to
>>>>> support the idea of a "virtual map handle": i.e., a handle not
>>>>> to a specific map, but to a *set* of maps matching certain
>>>>> criteria: e.g., <buffer>, <expr>, maps matching a mode mask,
>>>>> maps starting with specific char(s), etc... Once such a
>>>>> virtual handle had been obtained, a single call would suffice
>>>>> to enable/disable, or even attach callbacks to all maps in the
>>>>> set. Of course, some operations (e.g., execute) would be
>>>>> permitted only on non-virtual (single map) handles.
>>>>
>>>> And this is just mappings_dump() + filter() with my approach without
>>>> any need to invent a new DSL to describe criterias (or not invent DSL,
>>>> but use VimL expressions which would be just as efficient as
>>>> filter()). If I got it right then plus some way to attach callbacks to
>>>> "new mapping defined" event to keep "callback attached" state.
>>>
>>> So if I understand your approach correctly, what I've been calling "handles"
>>> would be simply maparg-style dicts, and it would be up to the user to filter
>>> on the dict members using map() and lambdas/funcrefs. With the user
>>> responsible for all filtering, the API functions would mostly just accept
>>> lists of these handle dicts. I believe this approach would be sufficient,
>>> though for reasons of both efficiency and convenience, it would be nice if
>>> functions that return lists of mappings allowed the caller to limit the range
>>> of mappings returned somehow. It could be optional args, some sort of filter
>>> criteria dict, or even an an optional predicate lambda/funcref. The
>>> lambda/funcref predicate would permit complete flexibility (provided the
>>> handle dict contained keys for all relevant attributes), though it would
>>> probably be less efficient, since Vim would be forced to apply it to every
>>> single mapping in the system, while it could most likely optimize the other
>>> approaches.
>>
>> Implementing lambda/funcref is not needed because it is the same as
>> using filter(). Could only save a few listitem_alloc() calls which is
>> nothing. Should actually be *less* efficient because `filter()` will
>> use expressions and both lambdas and funcrefs use Ex commands (lambdas
>> are really normal functions with special names and the only line:
>> `return {expr}`).
>
> The efficiency issue I alluded to has nothing to do with
> command vs function. If I specify criteria in a dict of some
> sort, Vim will most likely be able to limit its search to
> certain subsets of the internal map data structures: e.g., if
> I specify <buffer> maps only, it can probably avoid searching
> global maps altogether. With an arbitrary predicate (either
> filter string or lambda), in the absence of insanely complex
> and error-prone optimizations, Vim would have no way of
> limiting the search in this way, but would need to apply the
> test to each and every map in the system.

Yes, some basic filtering is needed. But instead of callback it is
better to rely on filter() for more complex filtering.

>
>>
>>>
>>> As for ambiguous/conflicting maps, not sure whether you were proposing to keep
>>> some sort of maplist() function for that, or whether you would require the
>>> user to inspect the output of mappings_dump() to determine ambiguity/conflict.
>>> I'd definitely favor the maplist() approach, since determining
>>> conflict/ambiguity can be a bit tricky, especially when multi-byte encodings
>>> are involved. But of course, anything is possible with mappings_dump(),
>>> provided it accepts a predicate lambda/funcref, which in turn receives all
>>> relevant map attributes. I guess it really just boils down to a tradeoff
>>> between Vim development time/effort/bugs and plugin developer convenience...
>>
>> I do not see how multi-byte encodings make dealing with conflict
>> harder,
>
> I was alluding to the idiosyncrasies outlined in :help
> map-multibyte.
>
>> but to filter out mappings which start with the same key
>> sequences all you need is a way to transform some sequences of bytes
>> representing `rhs` into canonical form which would be found in the
>> `lhs` key of `mappings_dump()` dictionaries. So filtering those out
>> should be as easy as three lines:
>>
>> let all_mappings = mappings_dump(basic_criteria)
>> " basic_criteria allows filtering out mappings
>> " for specific mode and buffer (or leave only globals).
>>
>> " Though given that main ideas for basic_criteria are
>> " "do not reimplement filter" and "C implementation must
>> " be simple" it may as well contain prefix selection.
>> " I selected only above variants because it is all
>> " Neovim supports.
>>
>> let canonical_lhs_prefix = mappings_canonicalize("\<F1>")
>> let needed_mappings = filter(copy(all_mappings),
>> \'v:val.lhs[:'.len(canonical_lhs_prefix).'-1] is# canonical_lhs_prefix')
>
> While this is certainly possible, it's not as simple and clear
> as...
> let conflicting_maps = maplist('<F1>')
>
> Again, it's a pain/convenience tradeoff that pits the needs of
> Vim developer(s) against those of plugin developers. The fact
> that plugin developer pain and inconvenience tends to be
> magnified across many years and a large number of plugin
> developers argues in favor of a more convenient, less
> error-prone implementation. But as you correctly point out,
> this needs to be balanced against the risk of adding complex
> new mechanisms, especially if the existing mechanisms can be
> enhanced to cover most use cases...

If it is a simple as those three lines (just check for a simple
prefix) it would be fine to add this filter to a list of basic filters
of mappings_dump(). Though I still would want to see
`mappings_canonicalize()` function (somebody needs to design a better
name though) to not request changes in C code for cases when such
basic filtering is not enough.

I would go for dictionary though: `mappings_dump({'buffer':
bufnr('%'), 'mode': 'vn', 'prefix': '<F1>'})`.

>
> Sincerely,
> Brett Stahlman
>>
>> Needs additional function to get canonical form for a random string though.
>>
>>>
>>> Sincerely,
>>> Brett Stahlman
>>>
>>>>
>>>>>
>>>>> Sincerely,
>>>>> Brett Stahlman
>>>>>
>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> BLACK KNIGHT: I'm invincible!
>>>>>>> ARTHUR: You're a looney.
>>>>>>> "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
>>>>>>>
>>>>>>> /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\
>>>>>>> /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
>>>>>>> \\\ an exciting new programming language -- http://www.Zimbu.org ///
>>>>>>> \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

--
--
You received this message from the "vim_use" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php

---
You received this message because you are subscribed to the Google Groups "vim_use" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vim_use+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

No comments:

Post a Comment