onivim / oni

Oni: Modern Modal Editing - powered by Neovim
https://www.onivim.io
MIT License
11.34k stars 298 forks source link

Opening Undotree window causes pixelToScreen crash #1295

Closed akinsho closed 6 years ago

akinsho commented 6 years ago

Apologies for the very vague issue description, but I've noticed since I believe the layers/window changes which I haven't looked at were integrated whenever I try to open the plugin undotree or use a custom fn I have to toggle quickfix or loclist I get the following error

screen shot 2018-01-08 at 20 52 58

the e.screenToPixel function seems to fail I can dig a little deeper and report back but I can consistenly reproduce it using undotree or my custom fn not sure what exactly screen to pixel does or whether or not either of the triggers opens a buffer in a special way that causes this error.

A thought the error message also raises is whether it might be worth adding a react Error Boundary at a higher level component and take advantage of componentDidCatch so the app isn't taken down potentially not sure this is a 100% catch all or that recovery from certain errors is possible.

bryphe commented 6 years ago

Ouch, that's rough!

which I haven't looked at were integrated whenever I try to open the plugin undotree or use a custom fn I have to toggle quickfix or loclist I get the following error

Would you mind sharing out the function you use here?

the e.screenToPixel function seems to fail I can dig a little deeper and report back but I can consistenly reproduce it using undotree or my custom fn not sure what exactly screen to pixel does or whether or not either of the triggers opens a buffer in a special way that causes this error.

Interesting, I think we rely on windows being 'entered' to get some of this information. That function is used to help the UI layer (error adorners, etc) know how to go from a "screen" (cell) position to a pixel position - important for rendering this in the right place! I'm wondering if there are cases where windows open but aren't 'entered', and we aren't getting this information correctly.

A thought the error message also raises is whether it might be worth adding a react Error Boundary at a higher level component and take advantage of componentDidCatch so the app isn't taken down potentially not sure this is a 100% catch all or that recovery from certain errors is possible.

Great point. Yes, definitely should have some error boundaries here. A good spot might be at the window manager / component - it'd be helpful if each of our Oni 'windows' were wrapped in this - that way at least each of the errors are localized to the particular region and the whole app doesn't die.

akinsho commented 6 years ago

Here's the function in question I think what you mention re windows not being 'entered' is the likely culprit here as the both of those commands are aimed at opening windows without moving the cursor to them (though i'm not doing anything special re cursor in the fn below), also nerdtree does the same but that isn't causing any errors, maybe it might also relate to the fact that its a qf buffer not sure if oni handles that differently somehere although doing a regular l:open works fine.

function! ToggleList(bufname, pfx)
  let l:buflist = GetBufferList()
  for l:bufnum in map(filter(split(l:buflist, '\n'), 'v:val =~ "'.a:bufname.'"'), 'str2nr(matchstr(v:val, "\\d\\+"))')
    if bufwinnr(l:bufnum) != -1
      exec(a:pfx.'close')
      return
    endif
  endfor
  if a:pfx ==# 'l' && len(getloclist(0)) ==# 0
      echohl ErrorMsg
      echo 'Location List is Empty.'
      return
  endif
  let l:winnr = winnr()
  exec(a:pfx.'open')
  if winnr() != l:winnr
    wincmd p
  endif
endfunction
akinsho commented 6 years ago
screen shot 2018-01-15 at 21 20 21

On further digging it seems a third window without the requisite method is created, i'm not sure if this relates to the mapStateToProps in NeovimLayersView? Also an error boundary works really nicely, stopping the app from collapsing it remains functional but not sure where a good place for would be since putting it in the window manager still kills the neovim editor instance but leaves the app up and responsive but would essentially be the same as having to restart

christo-auer commented 6 years ago

Same for taglist and, as you've already observed, anything related to quickfix or loclist (syntastic in my case). Also on macOS and Windows.

Eagerly waiting for a new release :-)

bryphe commented 6 years ago

On further digging it seems a third window without the requisite method is created, i'm not sure if this relates to the mapStateToProps in NeovimLayersView

Thanks for your help investigating here! Ya, what happens is we need some info about the buffer (like which line is visible at the top / bottom) in order to generate the screenToPixel function. However, in the case where we create the window but jump back, we never get that info, so the function is never created. I started a fix for it.

Also an error boundary works really nicely, stopping the app from collapsing it remains functional but not sure where a good place for would be since putting it in the window manager still kills the neovim editor instance but leaves the app up and responsive but would essentially be the same as having to restart

Ya, an error boundary would be great! I could see two potential places for error boundaries:

Having the error boundaries + unit tests to validate each of those cases would be a useful next step here.