@ZyX-I other than let and extend(), what other ways are available for updating dictionaries?

@tarruda remove() works for Dictionaries too.

@tarruda: Also API: vim_set_var, vim_set_vvar. v: variables are mostly set using set_vim_var_\*, but v: is still a dictionary.

@tarruda Any specific reason to stick with watcheradd and watcherdel function names? I suggest renaming it to observe and unobserve just like the ECMAScript specification.

IMO watch(er)?add and watch(er)?del names are related with debugging. Even Gecko engine warns you about that :P

Just my two cents.

@tarruda Any specific reason to stick with watcheradd and watcherdel function names? I suggest renaming it to observe and unobserve just like the ECMAScript specification.

No reason, but I don't usually spend a lot of time thinking about names. Both observe and watch are fine by me. cc @justinmk for a decision.

@tarruda: Also API: vim_set_var, vim_set_vvar. v: variables are mostly set using set_vim_var_*, but v: is still a dictionary.

Can you think of a situation where it would be useful to watch for v: variables? I'm not sure it is safe because these can be set anywhere, and it is not always safe to execute vimscript

@tarruda remove() works for Dictionaries too.

Got that covered, thanks

Other than the documentation and possibly a change in the watcheradd/watcherdel names, I think I'm done with this. Waiting for @justinmk to comment on the names before writing documentation.

I do not like any function names which do not include dict and key, because these functions

1. Only allow watching one specific key.
2. Only allow watching dictionaries.

. From function named watcheradd I would expect to also watch at least lists. Or also funcrefs. From function named dictwatcheradd I would expect to allow watching operations like “new key added”, without specifying every key which may possibly be added. Thus I would suggest

1. getdictkeywatchers(dict, key) -> [funcref*] | error for invalid types and empty key (following getmatches)
2. getdictwatchers(dict) -> {'keys': {key : [funcref+]}} | error for invalid type (following getmatches, but allowing to also report whole dictionary watchers in the future)
3. dictkeywatcheradd(dict, key, funcref | str) -> 0|-1 | error for invalid types, empty key and unknown function (following matchadd)
4. dictkeywatcherdelete(dict, key, funcref | str) -> 0|-1 | error for invalid types, empty key and missing watcher (following matchdelete)
5. cleardictwatchers(dict) -> 0 | error for invalid type (following clearmatches, should also clear whole dictionary watchers in the future)

To avoid memory leaks, I don't increase refcount on the dict when a watcher is added, so when a dict is garbage collected, all its watchers are silently removed. One way to handle is trigger a special notification(without new or old values) to tell the caller if a dict is garbage collected.

Since I'm not sure how to deal with this for cases where a dict is collected by garbage_collect at the toplevel(not sure if it is safe to call vimscript there), I will leave as it is for now.

At first I considered dictwatch/dictunwatch, but thought it would be better a more generic name in case we want to add support for lists later.

Using the same function for supporting makes second argument inconsistent: list does not need patterns. I do not really think that lists need anything, but a “whole list” watcher function: most of time items are appended to a list or removed from it and not assigned (even though it is possible and is rarely used). You can though make a function which watches the whole dictionary also add watchers to a whole list.

To avoid memory leaks, I don't increase refcount on the dict when a watcher is added, so when a dict is garbage collected, all its watchers are silently removed. One way to handle is trigger a special notification(without new or old values) to tell the caller if a dict is garbage collected.

? Why do you need this: when dictionary is removed watcher that is watching g: key is triggered, no need to run watcher that is watching the inner dictionary. Specifically I suggest this:

" Plugin is using g:surround_keys dictionary like currently g:surround_{number} is used:
function s:SurroundKeysChanged(dict, key, value)
  if a:dict isnot# g: | throw 'AssertionError:Invalid dictionary' | endif
  if a:key isnot# 'surround_keys' | throw 'AssertionError:Unexpected key' | endif
  if has_key(a:value, 'new')
    if type(a:value.new) == type({})
      call watcheradd(a:opts.new, function('s:SurroundKeyChanged'))
    else
      echohl ErrorMsg
      echomsg 'Invalid g:surround_keys type: expected dictionary'
      echohl NONE
    endif
  else
    " Do something on removal
  endif
endfunction
function s:SurroundKeyChanged(dict, key, value)
  if a:dict isnot# g:surround_keys | throw 'AssertionError:Invalid dictionary' | endif
  " Do something
endfunction
call dictkeywatcheradd(g:, 'surround_keys', function('s:SurroundKeysChanged'))

(BTW: note that I am also passing dictionary to make it possible to reuse one function for multiple dictionaries. Key and value pair is not enough.)

The second function is being called whenever key inside dictionary changes, but it is not called at all when dictionary is garbage-collected.

Using the same function for supporting makes second argument inconsistent: list does not need patterns. I do not really think that lists need anything, but a “whole list” watcher function: most of time items are appended to a list or removed from it and not assigned (even though it is possible and is rarely used). You can though make a function which watches the whole dictionary also add watchers to a whole list.

Considering these arguments, I'm fine with dictwatcheradd/dictwatcherdel. Let's see what @justinmk think of them.

Specifically I suggest this:

👍 makes sense, thanks

And there is another argument against patterns, especially against glob patterns: when hearing about pattern match user will expect everything to work which works in regular globs. But regular globs in Vim use shell (will be fixed if luaviml will ever make it into release), and also this will highly complicate creating the DFA. Non-pattern variant is more simple to code.

And there is another argument against patterns, especially against glob patterns: when hearing about pattern match user will expect everything to work which works in regular globs. But regular globs in Vim use shell (will be fixed if luaviml will ever make it into release), and also this will highly complicate creating the DFA. Non-pattern variant is more simple to code.

There's no need to call the shell for simple autocmd-like patterns. In fact, fileio.c already has methods for converting a glob pattern into a compiled regular expression, and match_file_pat can probably be used for matching the keys. I have avoided taking this approach because I'm not familiar with regular expression code and know it makes use of globals which can have unexpected side effects, but can have a look at it later(which would result in a backwards-compatible change)

About using a non-pattern variant to simplify having a fast implementation: In practice it will only offer an small improvement. Even if the user has 1000 plugins and each one has one watcher on g:, I doubt the performance impact will be noticeable. The convenience gains of just watching a pattern over g:(instead of a subdictionary, which still requires watching g: to detect when the subdictionary is removed) will compensate for the small loss in efficiency.

As for the DFA approach, it is not very complex. We already have it implemented in the regexp engine, but it needs to be refactored to allow better reuse in other parts of the code

@tarruda And what about my suggestion regarding new callback signature?

The dictionary is already passed as the self argument, but it may be a good
idea to pass it explicitly and accept an optional argument to be the "self"

On Sun, Nov 8, 2015, 11:14 AM Nikolai Aleksandrovich Pavlov <
notifications@github.com> wrote:

@tarruda https://github.com/tarruda And what about my suggestion
regarding new callback signature?

—
Reply to this email directly or view it on GitHub
#3603 (comment).

@tarruda Proposal was not only about the dictionary. When passed only as self it is convenient to use your variant with a:value = {'new': new, 'old': old}. When passed as an argument it is fine to use optional value, and get “new” value from the passed dictionary: though it is possible to do the same thing with dictionary in self dictionary functions are still rather uncommon and thus unfamiliar for some plugin authors.

dictionary functions are still rather uncommon and thus unfamiliar for some plugin authors.

They are also difficult to debug (:breakadd does not work, nor :Breakadd from scriptease.vim plugin).

@tarruda Proposal was not only about the dictionary. When passed only as self it is convenient to use your variant with a:value = {'new': new, 'old': old}. When passed as an argument it is fine to use optional value, and get “new” value from the passed dictionary: though it is possible to do the same thing with dictionary in self dictionary functions are still rather uncommon and thus unfamiliar for some plugin authors.

There's a reason I pass both old and new to a:value: I found it cleaner to determine the type of event":

function! Cb(a:key, a:value)
if !has_key(a:value, 'old') 
  " Key inserted
elseif !has_key(a:value, 'new')
  " Key deleted
else
  " Key updated
endif
endfunction

Without the new key:

function! Cb(a:key)
if has_key(self, a:key)
  " Key inserted or updated, need to check if an old parameter was passed
  if a:0 > 1
    " Key updated and old value in a:2
  else
    " Key inserted
  endif
else
   " Key deleted and old value in a:2
endif 
endfunction

An alternative would be to pass another argument that specifies the type of event, but this would also be unclean.

They are also difficult to debug (:breakadd does not work, nor :Breakadd from scriptease.vim plugin).

I'm not sure I understand, are you saying that any function that has the self variable set doesn't work with :breakadd?

FWIW I considered implementing #2203 as an autocommand(first @ZyX-I suggestion) and it would actually have been easier, but found it much cleaner to use callbacks with setup/remove functions. Autocommands would require at least 3 extra v: variables and there would be no way to watch arbitrary dictionaries, only top-level variables(Though most plugins just use g: variables for settings and I doubt that will change, so the usefulness of watching nested dictionaries is questionable).

There is one advantage of using autocommands though: It has a better chance of it being merged back into Vim. Since now there is an OptionSet autocommand, I don't see why Bram would argue against a VariableSet autocommand. Unlike job control(which uses callbacks now) this feature requires absolutely no support for asynchronicity.

But since we're still alpha, I will move this PR forward and if someone sends a patch to Vim and it is merged we can adapt to be more compatible.

From function named watcheradd I would expect to also watch at least lists. Or also funcrefs.

At first I considered dictwatch/dictunwatch, but thought it would be better a more generic name in case we want to add support for lists later.
Using the same function for supporting makes second argument inconsistent: list does not need patterns.

If this is the only objection, it seems not worth the cost in terms of API weight. Also, there is no Vim precedent for prefixing a function name with "dict" or "list". Existing function names to draw from:

matchadd()
matchdelete()
histadd()
histdel()

I do not like any function names which do not include dict and key, because these functions

1. Only allow watching one specific key.
2. Only allow watching dictionaries.
   From function named watcheradd I would expect to also watch at least lists.
   Or also funcrefs. From function named dictwatcheradd I would expect to allow

We should allow watching function calls in the future. What if watchadd() allowed input like exists() to specify the type of thing being watched?

getwatches(var, [key])   " error if [key] is given for non-dict types
watchadd(varname, [key]) " {varname} is formatted like the input for exists()
                         " to specify whether it is a function, variable, etc.
watchdel(var, key)       " following histdel()
clearwatches(var)

Both observe and watch are fine by me. cc @justinmk

Vim draws from unix conventions where possible (:ls, :read, :set, printf(), system()). Some shells have a watch command, which I guess is servicable for the concept here.

Without the new key:

You forgot dict in function declaration. Without it you are not going to be able to use self AFAIK.

I'm not sure I understand, are you saying that any function that has the self variable set doesn't work with :breakadd?

Not sure what exactly @justinmk meant, but I think it is the problem with anonymous and not dictionary functions. And they also do work with :breakadd: just you need to always use breakadd {linenr} {funcname} form because anonymous functions have number as a {funcname} which in a form breakadd {funcname} will be considered breakadd {linenr}.

Ah, and you have to somehow determine the number that serves as a function name. Though this is easy, but not very convenient.

@justinmk Also when talking about funcrefs I was thinking about function redifinition with function!: because this is consistent with other watches watching only write access. Though it is much less useful then watching calls.