[RFC] get_keymap API #6236

tjdevries · 2017-03-08T04:11:53Z

Discussion in issue #6123 . Will put more info once features have been decided on.

Two new api functions:

nvim_get_keymap(mode)

and

nvim_buf_get_keymap(buffer, mode)

justinmk · 2017-03-08T16:44:43Z

What about instead of passing in a string, you could pass in a dictionary.
This could also let us add more options in the future.

This is something I've been meaning to nail down, so we can extend API functions in a backwards-compatible way. Per msgpack RPC protocol, function parameters are sent as an array "over the wire". But in most clients, those parameters are bound to an arity. E.g., I just tried adding a String dummy parameter to nvim_get_api_info, and the python client broke.

How about this: design the API with reasonable parameters, but add a dummy dict as the last parameter for future expansion. E.g. in the case of get_keymap:

nvim_get_keymap(String mode, String scope, Dictionary reserved_, Error *err)

I'll document this pattern in :help develop.txt somewhere.

cc @bfredl

justinmk · 2017-03-08T16:46:01Z

replying to @ZyX-I comment #6123 (comment)

I do not like the idea of dummy parameters: how would you know in advance that it is (not) needed?

The contract of the "reserved" Dictionary is that any args added there in the future must be optional.

Better versioned API somewhere, was not this discussed already?

We do version the API. But if we need to extend a function we have to create a new function, we can't add optional arguments.

When initiating connection request a specific version and when altering API keep an adapter for some specified time so that dispatcher may select proper one

That would be better, but that's not implemented. And it's more complicated. Let's not let perfect get in the way of good.

tjdevries · 2017-03-08T17:45:10Z

I'm not sure I like scope being a string.

I think scope should be a dictionary and should have values like what I mentioned in my other comment:

{'global': false, 'buffer': false}  " Returns empty list
{'global': false, 'buffer': true}   " Returns list of only buffer mappings
{'global': true, 'buffer': false}   " Returns list of only global mappings.
{'global': true, 'buffer': true}    " Returns list of mappings, with possible overlapping mappings (or we could try and eliminate those)

This will make it a lot more clear about what we're doing and make it so we don't have to do any string parsing in the API call. Would that be acceptable?

tjdevries · 2017-03-08T17:45:53Z

The other items, leaving a dummy dict and adding an error seem fine with me. Seems like a good in-between solution until we have a more formal way to define adapters for different versions.

ZyX-I · 2017-03-08T17:49:33Z

The contract of the "reserved" Dictionary is that any args added there in the future must be optional.

I did not mean this. I mean that there is no such dictionary in existing API functions and we do not know whether such dictionary will (not) be needed in the future. I do not like the idea of attaching such dictionary to each and every function, this looks ugly. Just as well we may have an agreement that all arguments are only keyword arguments and never pass any argument lists in first place, dispatcher should know default values and which values are required (either through some syntax extension or just “this type always have X as the default”).

That would be better, but that's not implemented. And it's more complicated. Let's not let perfect get in the way of good.

Reserved dictionaries do not sound like anything “good”. They look like something not functioning, like dummy functions throwing NotImplementedError if called: you always add a dictionary, but you do not even know whether they will ever be allowed to be non-empty. And you can’t afford not throwing an error for dictionary with unknown arguments: who knows how significant that unknown argument was.

This will make it a lot more clear about what we're doing and make it so we don't have to do any string parsing in the API call. Would that be acceptable?

“String parsing” is easier then traversing a Dictionary.

justinmk · 2017-03-08T17:50:29Z

{'global': false, 'buffer': false}  " Returns empty list
{'global': false, 'buffer': true}   " Returns list of only buffer mappings
{'global': true, 'buffer': false}   " Returns list of only global mappings.
{'global': true, 'buffer': true}    " Returns list of mappings, with possible overlapping mappings (or we could try and eliminate those)

Why do we need the last one? And we definitely don't need the first one. The last one is not different than calling get_keymap twice, then merging the results. And it can be done in a single RPC call with nvim_call_atomic. So a string parameter avoids an unnecessarily complicated client UX.

justinmk · 2017-03-08T18:05:59Z

I do not like the idea of attaching such dictionary to each and every function, this looks ugly.

I mostly agree, but it's more ugly to add new functions like nvim_foo2 with slightly different arguments. And when we get the version adapters the ugly "reserved" Dictionary can be removed by that very mechanism.

OTOH the "reserved" Dictionary may be over-engineering in itself. Maybe in the meantime one should just accept the limitation and if it gets too annoying we will be motivated to implement the version adapter.

ZyX-I · 2017-03-08T18:13:43Z

This would be isolated in a source code, possibly even in a separate file, not something always visible to everyone.

justinmk · 2017-03-08T19:53:22Z

After more thought, I have great trepidation about that "version adapter" thing. Anyone who's maintained multiple versions of e.g. a REST API knows that maintaining multiple versions of an API is very difficult, complexity increases non-linearly. And having the "version adapter" will make us want to use it, and over-use it, and navigating the code will be unpleasant.

We should follow KISS approach for the time being.

Adding a Dictionary called optional isn't that ugly.

jamessan · 2017-03-08T20:33:24Z

After more thought, I have great trepidation about that "version adapter" thing. Anyone who's maintained multiple versions of e.g. a REST API knows that maintaining multiple versions of an API is very difficult, complexity increases non-linearly.

That's why you just add on to the API, instead of removing things. In many cases, the old API's implementation should be able to just call the new API, so there's not much old code to maintain. Does it really matter if there's nvim_foo and nvim_foo2? Either way (dictionary or new function), the user needs to read documentation to see what's available in which API version.

The incrementing number pattern is used quite successfully in Subversion (e.g. docs about the svn_client_checkout family of functions and their deprecated implementations).

If there's already a reason to have a dictionary argument, then sure go with that. If it's just being added as a placeholder, then it seems that just adding a new function later is a better option.

justinmk · 2017-03-08T20:42:28Z

That's why you just add on to the API, instead of removing things.

No objection there, and that's the thrust of the "API level" concept that we already follow. And the proposed optional Dictionary is for adding parameters only.

If there's already a reason to have a dictionary argument, then sure go with that. If it's just being added as a placeholder, then it seems that just adding a new function later is a better option.

Ok. But the (third) approach that I was reacting to is the "version adapter" idea, which could be used to remove and re-order parameters. It's a nice idea but could lead us into a quagmire.

Better versioned API somewhere, was not this discussed already? When initiating connection request a specific version and when altering API keep an adapter for some specified time so that dispatcher may select proper one. And dummies are also too limited: with versioned API you can decide e.g. that ordering of arguments was inconsistent and reorder them

justinmk · 2017-03-08T20:57:05Z

If there's already a reason to have a dictionary argument, then sure go with that. If it's just being added as a placeholder, then it seems that just adding a new function later is a better option.

The nice thing about optional is that it signals (to devs and client users) that the changes, if any, are:

optional
accretive

With numbered functions this signal is lost, the docs must be inspected to determine all dimensions of change.

Of course, we always have numbered functions available for cases where a new function really is needed.

ZyX-I · 2017-03-08T21:21:24Z

There is fourth approach: replace argument list with argument dictionary, make dispatcher handle it. Allows default values for all arguments if implemented properly*, adding new arguments, reordering them. But adds some overhead on processing dictionary.

* Should be relatively easy to add, including into the current dispatcher, though, of course, only for last N arguments. I think I could do anything required easily, except for writing lpeg grammar without general refactoring of the existing grammar: they managed to produce a thing which has almost nothing in common with anything else I know, not readable and with lots of visual noise. For possible “general refactoring” output check gendeclarations.lua.

ZyX-I · 2017-03-08T21:42:42Z

src/nvim/api/vim.c

+
+        Error err = ERROR_INIT;
+
+        maparg_dict = nvim_call_function(cstr_to_string("maparg"), args, &err);


For string literals there is STATIC_CSTR_TO_STRING.

Cool! Didn't know about that one.

ZyX-I · 2017-03-08T21:45:41Z

src/nvim/getchar.c

+  bool buffer_maps = (STRNCMP(scope, "buffer", 6) == 0);
+  bool global_maps = (STRNCMP(scope, "global", 6) == 0);
+
+  if (buffer_maps) {


STRNCMP should be here, and I think global maps should be first. Do not do two comparisons where one is enough.

Though why STRNCMP? This makes arguments like globalXXX valid. Should be strcmp: no macros (macros was created by Bram only because he uses char_u, this function uses char), no n.

Hadn't thought that through all the way. Didn't know the macros were Vim items.

ZyX-I · 2017-03-08T21:46:55Z

src/nvim/getchar.c

@@ -4261,3 +4261,21 @@ static bool typebuf_match_len(const uint8_t *str, int *mlen)
  *mlen = i;
  return str[i] == NUL;  // matched the whole string
 }
+
+mapblock_T * get_maphash(int index, char *scope)


const char *const. And FUNC_ATTR_NONNULL_ALL FUNC_ATTR_PURE. Also no space after asterisk.

ZyX-I · 2017-03-08T21:54:14Z

src/nvim/api/vim.c

+        maparg_dict = nvim_call_function(cstr_to_string("maparg"), args, &err);
+
+        if (err.set) {
+          ADD(mappings, STRING_OBJ(cstr_to_string("this one didn't work")));


I would use assert(false) here: missing description for an existing map means that something is broken, better raise the possibility it will be reported.

Though I do not like the idea of using maparg function in first place: you just need to copy and refactor ~15 mostly trivial lines from get_maparg, this is not enough to bother with reusing code.

I plan on abstracting out a few of the things from maparg to be a separate function. I want them to use the same thing for when we add descriptions or potentially other items.

I just haven't gotten around to it. I pushed to the PR so that we could have a discussion about the API level items, I'm still working on a lot of the implementation and testing.

Thanks for the comments thus far though!

Refactored the function out so that it could be used in both places without all the extra checking.

brcolow · 2017-03-09T19:37:08Z

test/functional/api/map_spec.lua

+      nvim('command', 'nnoremap foo bar')
+      -- Only one mapping available
+      -- Should be the same as the dictionary we supplied earlier
+      -- and the dicationary you would get from maparg


typo: dicationary => dictionary

Fixed... unless I am blind again ;)

bfredl · 2017-03-15T16:52:01Z

src/nvim/api/vim.c

+///
+/// @param mode
+/// @param scope
+Array nvim_get_keymap(String mode, String scope)


When rebasing on latest master FUNC_API_SINCE(2) will be needed.

bfredl · 2017-03-15T16:54:45Z

test/functional/api/map_spec.lua

+  describe('original maparg', function()
+    it('should return a dictionary', function()
+      nvim('command', 'nnoremap foo bar')
+      eq('bar', nvim('call_function', 'maparg', {'foo'}))


can use funcs.maparg('foo')

tjdevries · 2017-03-24T19:17:51Z

I updated the first comment for a description, but here's the status:

New api function: nvim_get_keyamp(String mode, String scope)

Returns a list of mappings for the mode ('n', 'i', ...) or scope ('buffer', 'global')

I have not implemented the reserved or error additions, since I wasn't sure if that's going to be the case of how we're doing things.

ZyX-I · 2017-03-24T19:22:45Z

src/nvim/api/vim.c

+    while (current_maphash) {
+      // Check for correct mode
+      if ((int_mode & current_maphash->m_mode) != 0) {
+        typval_T *dict = xmalloc(sizeof(typval_T));


You don’t neet to allocate typval_T with xmalloc, all functions which deal with typval_T* normally expect that you pass something allocated on the stack because most of time this is exactly what is being done. But you actually need no typval_T: I would suggest to

Allocate (dict_T *const dict = dict_alloc();) and free (dict_free(dict);) out of the cycle.

Reuse allocated dictionary by using dict_clear(dict) in the cycle.

Since all functions, but vim_to_object expect dict_T* you may construct typval_T right in the function call: vim_to_object((typval_T[]) { { .v_type = VAR_DICT, .vval.v_dict = dict } }) if I am not mistaking.

Also I see that you allocate dict, but I do not see you freeing it, not to mention unreferencing the dictionary. There should be a memory leak.

ZyX-I · 2017-03-24T19:33:02Z

src/nvim/api/vim.c

+      continue;
+    }
+
+    while (current_maphash) {


Should not it be

for (mapblock_T *current_maphash = get_maphash(i, scope.data); current_maphash; current_maphash = current_maphash->m_next)

? And you definitely need no if() with continue at all, even with your code: while cycle just will not be executed. Also current_maphash variable declaration must be in the cycle, not at the start of the function.

ZyX-I · 2017-03-24T19:33:17Z

src/nvim/api/vim.c

+  }
+
+  int i = 0;
+  for (i = 0; i < MAX_MAPHASH; i++) {


Remove the above line, for (int i = 0.

ZyX-I · 2017-03-24T19:34:40Z

src/nvim/api/vim.c

+  // Convert the string mode to the integer mode
+  // that is stored within each mapblock
+  int int_mode = get_map_mode((char_u **)(&(mode.data)), 0);
+  int buffer_local = 0;


This is boolean: const bool buffer_local = strcmp(scope.data, "buffer") == 0;.

ZyX-I · 2017-03-24T19:35:46Z

src/nvim/eval.c

    }
  }
 }

+void mapblock_fill_dict(dict_T *dict, mapblock_T *mp, int buffer_local)


FUNC_ATTR_NONNULL_ALL and buffer_local is a clear boolean.

Additionally I would suggest using dict_T *const and const mapblock_T *const, especially the second (state that you are not going to modify the mapblock_T, const after * in this case is largerly irrelevant because it only states that variable will not be reassigned inside a function).

ZyX-I · 2017-03-24T19:42:19Z

src/nvim/eval.c

+  dict_add_nr_str(dict, "expr",    mp->m_expr    ? 1L : 0L, NULL);
+  dict_add_nr_str(dict, "silent",  mp->m_silent  ? 1L : 0L, NULL);
+  dict_add_nr_str(dict, "sid",     (long)mp->m_script_ID, NULL);
+  dict_add_nr_str(dict, "buffer",  (long)buffer_local, NULL);


@justinmk How about making that a buffer number or zero?

Note that this would be backwards-incompatible. Fine when checking if mp.buffer, not fine when checking actual value. Help explicitly states that "buffer" is 1 for a buffer local mapping.

Probably worth doing this for get_keymap, leaving maparg() as-is.

ZyX-I · 2017-03-24T19:42:35Z

@justinmk Also some API refactoring: remove scope argument from nvim_get_keymap (function will always return global mappings), add nvim_buf_get_keymap (obviously, always returns buffer mappings, but for the specified buffer and not for the current one).

ZyX-I · 2017-03-24T19:49:00Z

I think that scope argument should be inconsistent with the API: if you need something entity-local (buffer-, window-, etc) you go to nvim_buf API with specific argument, never have some argument like scope. Currently there are only APIs for variables and options with this property (BTW, missing set_current_dir, that is only global), let’s continue.

Additionally the workflow “change to buffer, get keymap, change back” is not something you want to do constantly: too many side-effects when buffers are changed.

tjdevries · 2017-04-26T00:55:20Z

Fixed comments, and rebased onto master.

tjdevries · 2017-05-11T15:29:54Z

Rebased onto master and bumped API version of the two functions made here.

nvim_get_keymap(mode) nvim_buf_get_keymap(buffer, mode)

tjdevries · 2017-05-23T03:59:05Z

@justinmk I just rebased and pushed again, I think tests should pass. Anything that needs to get done for this to be merged?

tjdevries · 2017-05-24T02:10:30Z

got the green check marks :)

bfredl · 2017-05-24T05:42:55Z

src/nvim/api/buffer.c

+/// @param[out]  err   Error details, if any
+/// @returns An array of maparg() like dictionaries describing mappings
+ArrayOf(Dictionary) nvim_buf_get_keymap(Buffer buffer, String mode, Error *err)
+  FUNC_API_SINCE(3)


4 spaces (and similarly below)

bfredl · 2017-05-24T05:43:57Z

Otherwise LGTM

tjdevries · 2017-05-24T12:25:35Z

I think I got it, had some struggles with rebasing to fixup the indentation commit

justinmk · 2017-05-31T10:10:33Z

If I'm reading QB correctly, keymap_spec is resulting in consistent failures since this was merged. Maybe some interaction with the intervening commits.

06:00:14,564 INFO  - # test/functional/api/keymap_spec.lua @ 215: get_keymap returns script numbers for global maps

tjdevries · 2017-05-31T14:36:46Z

Hmm, the only reason I can think that would be is if it returns a different number for the SID value there than on my machine. I'm not sure how to get the current SID value.

The tests all pass locally for me. How can I debug this?

justinmk · 2017-05-31T15:28:31Z

I'm a bit mystified by the QB failure, it is not clear. Perhaps we should create a PR that reverts this one, to see if that makes QB happy, then proceed from there.

ZyX-I · 2017-05-31T15:28:35Z

SIDs are incremented, starting from one. To make that robust you just need to call [helpers.]clear() so it would restart Neovim and thus restart a counter.

ZyX-I · 2017-05-31T15:31:00Z

Which you actually do in before_each().

This reverts commit 45626de. Conflicts: src/nvim/eval.c

justinmk · 2017-06-06T00:13:22Z

Doesn't look like this PR is the cause of QB hang, rather f4fddbf.

This reverts commit 5e9ac68.

tjdevries · 2017-06-06T12:28:57Z

:) Okay. Glad to hear it's working fine then.

FEATURES: 0e873a3 Lua(Jit) built-in neovim#4411 5b32bce Windows: `:terminal` neovim#7007 7b0ceb3 UI/API: externalize cmdline neovim#7173 b67f58b UI/API: externalize wildmenu neovim#7454 b23aa1c UI: 'winhighlight' neovim#6597 17531ed UI: command-line coloring (`:help input()-highlight`) neovim#6364 244a1f9 API: execute lua directly from the remote api neovim#6704 45626de API: `get_keymap()` neovim#6236 db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` neovim#7082 dc68538 menu_get() function neovim#6322 9db42d4 :cquit : take an error code argument neovim#7336 9cc185d job-control: serverstart(): support ipv6 neovim#6680 1b7a9bf job-control: sockopen() neovim#6594 6efe84a clipboard: fallback to tmux clipboard neovim#6894 6016ac2 clipboard: customize clipboard with `g:clipboard` neovim#6030 3a86dd5 ruby: override ruby host via `g:ruby_host_prog` neovim#6841 16cce1a debug: $NVIM_LOG_FILE neovim#6827 0cba3da `:checkhealth` built-in, validates $VIMRUNTIME neovim#7399 FIXES: 105d680 TUI: more terminals, improve scroll/resize neovim#6816 cb912a3 :terminal : handle F1-F12, other keys neovim#7241 619838f inccommand: improve performance neovim#6949 04b3c32 inccommand: Fix matches for zero-width neovim#7487 60b1e8a inccommand: multiline, other fixes neovim#7315 f1f7f3b inccommand: Ignore leading modifiers in the command neovim#6967 1551f71 inccommand: fix 'gdefault' lockup neovim#7262 6338199 API: bufhl: support creating new groups neovim#7414 541dde3 API: allow K_EVENT during operator-pending 8c732f7 terminal: adjust for 'number' neovim#7440 5bec946 UI: preserve wildmenu during jobs/events neovim#7110 c349083 UI: disable 'lazyredraw' during ui_refresh. neovim#6259 51808a2 send FocusGained/FocusLost event instead of pseudokey neovim#7221 133f8bc shada: preserve unnamed register on restart neovim#4700 1b70a1d shada: avoid assertion on corrupt shada file neovim#6958 9f534f3 mksession: Restore tab-local working directory neovim#6859 de1084f fix buf_write() crash neovim#7140 7f76986 syntax: register 'Normal' highlight group neovim#6973 6e7a8c3 RPC: close channel if stream was closed neovim#7081 85f3084 clipboard: disallow recursion; show hint only once neovim#7203 8d1ccb6 clipboard: performance, avoid weird edge-cases neovim#7193 01487d4 'titleold' neovim#7358 01e53a5 Windows: better path-handling, separator (slash) hygiene neovim#7349 0f2873c Windows: multibyte startup arguments neovim#7060 CHANGES: 9ff0cc7 :terminal : start in normal-mode neovim#6808 032b088 lower priority of 'cursorcolumn', 'colorcolumn' neovim#7364 2a3bcd1 RPC: Don't delay notifications when request is pending neovim#6544 023f67c :terminal : Do not change 'number', 'relativenumber' neovim#6796 1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets neovim#6915 6720fe2 help: `K` tries Vim help instead of manpage neovim#3104 7068370 help, man.vim: change "outline" map to `gO` neovim#7405

oni-link · 2018-06-13T14:50:54Z

src/nvim/getchar.c

+mapblock_T *get_maphash(int index, buf_T *buf)
+    FUNC_ATTR_PURE
+{
+  if (index > MAX_MAPHASH) {


The comparison should be >=, otherwise an index of MAX_MAPHASH (= 256) could access an element after the maphash arrays.

Thanks @oni-link ! Fixed on master.

Patch-by: oni-link <knil.ino@gmail.com> ref: #6236 (comment)

marvim added the WIP label Mar 8, 2017

justinmk mentioned this pull request Mar 8, 2017

Descriptive Maps #6123

Closed

justinmk added api libnvim, Nvim RPC API needs:design needs a clear design proposal labels Mar 8, 2017

ZyX-I reviewed Mar 8, 2017

View reviewed changes

ZyX-I suggested changes Mar 8, 2017

View reviewed changes

teto mentioned this pull request Mar 9, 2017

API: command "palette": nvim_get_keymap(), etc. #6240

Closed

tjdevries force-pushed the get_keymap branch from fa9275b to ac4162a Compare March 9, 2017 16:31

brcolow reviewed Mar 9, 2017

View reviewed changes

bfredl reviewed Mar 15, 2017

View reviewed changes

tjdevries force-pushed the get_keymap branch from ac4162a to c613fd0 Compare March 24, 2017 19:15

ZyX-I suggested changes Mar 24, 2017

View reviewed changes

ZyX-I reviewed Mar 24, 2017

View reviewed changes

tjdevries force-pushed the get_keymap branch from 3db9969 to e6eca76 Compare April 26, 2017 00:54

tjdevries force-pushed the get_keymap branch from e6eca76 to 1fd5ce2 Compare May 11, 2017 15:29

tjdevries mentioned this pull request May 18, 2017

IndexError: list index out of range tjdevries/descriptive_maps.vim#1

Closed

Add api function get keymap

360124e

nvim_get_keymap(mode) nvim_buf_get_keymap(buffer, mode)

tjdevries force-pushed the get_keymap branch from 1fd5ce2 to edf2f19 Compare May 23, 2017 03:26

bfredl reviewed May 24, 2017

View reviewed changes

Bump API version of keymap functions

eeb6212

tjdevries force-pushed the get_keymap branch from 06eab3e to eeb6212 Compare May 24, 2017 12:24

bfredl merged commit 45626de into neovim:master May 25, 2017

jszakmeister removed the RFC label May 25, 2017

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 5, 2017

Revert "get_keymap API (neovim#6236)"

5e9ac68

This reverts commit 45626de. Conflicts: src/nvim/eval.c

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 6, 2017

Revert "Revert "get_keymap API (neovim#6236)""

da3a229

This reverts commit 5e9ac68.

tjdevries mentioned this pull request Oct 13, 2017

[RFC] News neovim/neovim.github.io#153

Merged

justinmk mentioned this pull request Nov 8, 2017

NVIM v0.2.1 #7505

Merged

oni-link reviewed Jun 13, 2018

View reviewed changes

justinmk added a commit that referenced this pull request Jun 15, 2018

get_maphash: fix off-by-one

c990d65

Patch-by: oni-link <knil.ino@gmail.com> ref: #6236 (comment)


		Error err = ERROR_INIT;

		maparg_dict = nvim_call_function(cstr_to_string("maparg"), args, &err);

Uh oh!

[RFC] get_keymap API #6236

[RFC] get_keymap API #6236

Uh oh!

Conversation

tjdevries commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Mar 8, 2017

Uh oh!

justinmk commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tjdevries commented Mar 8, 2017

Uh oh!

tjdevries commented Mar 8, 2017

Uh oh!

ZyX-I commented Mar 8, 2017

Uh oh!

justinmk commented Mar 8, 2017

Uh oh!

justinmk commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZyX-I commented Mar 8, 2017

Uh oh!

justinmk commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamessan commented Mar 8, 2017

Uh oh!

justinmk commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZyX-I commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjdevries commented Mar 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tjdevries commented Mar 8, 2017 •

edited

Loading

justinmk commented Mar 8, 2017 •

edited

Loading

justinmk commented Mar 8, 2017 •

edited

Loading

justinmk commented Mar 8, 2017 •

edited

Loading

justinmk commented Mar 8, 2017 •

edited

Loading

justinmk commented Mar 8, 2017 •

edited

Loading

bfredl May 24, 2017 •

edited

Loading