t9md / atom-vim-mode-plus

vim-mode improved
https://atom.io/packages/vim-mode-plus
MIT License
1.4k stars 111 forks source link

Peektop error #174

Closed jordwalke closed 8 years ago

jordwalke commented 8 years ago

[Enter steps to reproduce below:]

  1. simply hit / to search
  2. ...

Atom Version: 1.6.0-beta4 System: Mac OS X 10.10.3 Thrown From: vim-mode-plus package, v0.21.3

Stack Trace

Uncaught TypeError: this.peekTop(...).setTarget is not a function

At /Users/me/.atom/packages/vim-mode-plus/lib/operation-stack.coffee:45

TypeError: this.peekTop(...).setTarget is not a function
  at OperationStack.process (/Users/me/.atom/packages/vim-mode-plus/lib/operation-stack.coffee:57:20)
  at OperationStack.run (/Users/me/.atom/packages/vim-mode-plus/lib/operation-stack.coffee:37:8)
  at run (/Users/me/.atom/packages/vim-mode-plus/lib/base.coffee:13:29)
  at atom-text-editor.<anonymous> (/Users/me/.atom/packages/vim-mode-plus/lib/base.coffee:200:65)
  at CommandRegistry.module.exports.CommandRegistry.handleCommandEvent (/Applications/Atom Beta.app/Contents/Resources/app.asar/src/command-registry.js:260:29)
  at __bind (/Applications/Atom Beta.app/Contents/Resources/app.asar/src/command-registry.js:3:61)
  at KeymapManager.module.exports.KeymapManager.dispatchCommandEvent (/Applications/Atom Beta.app/Contents/Resources/app.asar/node_modules/atom-keymap/lib/keymap-manager.js:544:16)
  at KeymapManager.module.exports.KeymapManager.handleKeyboardEvent (/Applications/Atom Beta.app/Contents/Resources/app.asar/node_modules/atom-keymap/lib/keymap-manager.js:363:22)
  at WindowEventHandler.module.exports.WindowEventHandler.handleDocumentKeydown (/Applications/Atom Beta.app/Contents/Resources/app.asar/src/window-event-handler.js:97:36)
  at HTMLDocument.__bind (/Applications/Atom Beta.app/Contents/Resources/app.asar/src/window-event-handler.js:3:61)

Commands

  8x -0:24.7.0 blur (input.hidden-input)
     -0:22.3.0 pane:show-previous-item (input.hidden-input)
  3x -0:22.2.0 blur (input.hidden-input)
     -0:22 pane:show-previous-item (input.hidden-input)
  3x -0:22 blur (input.hidden-input)
 30x -0:20.1.0 vim-mode-plus:move-up (input.hidden-input)
     -0:19.2.0 vim-mode-plus:search (input.hidden-input)
  5x -0:19.2.0 blur (input.hidden-input)
     -0:17.4.0 vim-mode-plus:move-down (input.hidden-input)

Config

{
  "core": {
    "audioBeep": false,
    "automaticallyUpdate": false,
    "disabledPackages": [
      "minimap-selection",
      "nuclide-toolbar",
      "nuclide-debugger-atom",
      "nuclide-debugger-hhvm",
      "nuclide-debugger-lldb",
      "nuclide-test-runner",
      "nuclide-remote-projects",
      "wrap-guide",
      "whitespace",
      "metrics",
      "hyperclick",
      "nuclide-file-tree",
      "nuclide-hack-symbol-provider",
      "nuclide-hack",
      "nuclide-buck-files",
      "tool-bar",
      "browser-plus",
      "ex-mode",
      "git-diff",
      "background-tips"
    ],
    "followSymlinks": false,
    "themes": [
      "one-dark-ui",
      "base16-ocean-dark-syntax-theme"
    ]
  },
  "vim-mode-plus": {
    "flashOnOperate": false,
    "flashOnSearch": false,
    "flashScreenOnSearchHasNoMatch": false,
    "highlightSearch": true,
    "incrementalSearch": true,
    "setCursorToStartOfChangeOnUndoRedo": true,
    "showCursorInVisualMode": false,
    "useClipboardAsDefaultRegister": true,
    "useSmartcaseForSearch": true
  }
}

Installed Packages

# User
base16-ocean-dark-syntax-theme, v0.1.9
file-icons, v1.6.14
language-ocaml, v1.1.2
markdown-preview-opener, v0.1.1
nuclide, v0.115.0
toggle-gutter, v0.2.2
vim-mode-plus, v0.21.3

# Dev
nuclide, v0.115.0
jordwalke commented 8 years ago

What's strange is that I did not upgrade vim-mode-plus yet started experiencing this. I started using the JS-syntax mode (which I hadn't done in a while which might be related?)

t9md commented 8 years ago

Might be related. You always use Atom-beta? I basically use Atom-stable and not check beta behavior actively. One big pain choosing atom is its speed of change and frequently break package's feature when updated. And bad thing is for many user(Mac user), update is done automatically. So plugin author need track both stable and beta very actively to avoid feature breaking.

t9md commented 8 years ago

I checked code, and still not clear what the cause is.. You can't reproduce issue all the time right?

jordwalke commented 8 years ago

When using the / search, the keystroke immediately after / causes the error: Here's a screenshot of the error being raised (setTarget isn't defined). screen shot 2016-02-27 at 5 57 24 pm

Here's the thing returned for peekTop: screen shot 2016-02-27 at 5 57 30 pm It does not have a setTarget method.

t9md commented 8 years ago

I couldn't reproduce with1.6-beta3. You can always reproduce this issue?

t9md commented 8 years ago

The keyatroke immediately after / shoud go into search input mini-editor in normal situation. So issue might be happen when you input two keystroke such as /a in verry shoort period. It might be related your key repeat rate and your computer processing speed. I will try to reproduce it. Bu additional thought and info is always welcome, needed.

t9md commented 8 years ago

I could reproduce same peekTop().setTarget is not function error by invoking vmp command when focus is on search mini editor. But to create this situation I have to set abnormal keymap like blow.

'atom-text-editor.vim-mode-plus-search':
  'ctrl-w': 'vim-mode-plus:move-to-next-word'

But in normal situation move-to-next-word is bound to w and w in search-mini-editor never invoke move-to-next-word since keymap scope doesn't match. So to I have to explicitly set strange keymap like I did in above configuration.

So still not sure the condition of this issue. Key repeat and Delay until repeat is not related I believe. I couldn't reproduce it even if making these value very short, and when you input /, it immediately focus to mini-editor synchronously.

t9md commented 8 years ago

I think I can fix and have some idea, but before adding guard code blindly. I want the people who can reproduce this issue very hight success rate, and want let them evaluate fixed code.

jordwalke commented 8 years ago

So what happens reliably, is that I press /, and whatever keystroke I hit next causes failure (it doesn't have to be w). Also, the mini search buffer never shows up at all.

jordwalke commented 8 years ago

Please let me know if there's a specific test case you want me to try/log. My delay/key repeat rate is high, but it's never been a problem with vmp in the past and it's happening every time. Perhaps something is preventing the search mini editor from popping up (but not throwing an error) and that ruins some assumptions.

t9md commented 8 years ago

Pls answer EACH question one by one to clarify issue. Im not good at english. I still can't understand how this issue is like.

1 you can reproduce this 100% or is it occasional issue? 2 If you can repro every time. Pls put minimum procedure and condition to repro. 3 is it happen in stable verion of Atom?

I couldn't reproduce issue with beta and stable. So I want you to narrow condition by trying stable and beta atom version, disabling package except vmp, removing keymap, trying to default config, reseting key repeat rate to OS's default.

Yes this is BUG and should be fix and want to fix, But want to know condition. What prevent search panel popps up.

jordwalke commented 8 years ago

Okay I will try each of those and report the findings.

jordwalke commented 8 years ago
  1. I can reproduce it every time, and I'm on the latest public release of vmp.
  2. Minimum repro procedure: Hit /, then type any letter. Very simple. The mini editor does not even show on /, yet the key binding resolver shows that vpm is handling the /.
  3. Stable and beta both. (I will try rolling back to a previous version of vmp).

I reset to very slow default key repeat rate using Karabiner. I tried resetting my keymap.cson to be empty.

I think something is making it so vmp cannot show the mini editor. I tried disabling every other package except vmp too. Only thing I haven't tried is rolling back to previous version of vmp. Here's my config for vmp:

  "vim-mode-plus":
    flashOnOperate: false
    flashOnSearch: false
    flashScreenOnSearchHasNoMatch: false
    highlightSearch: true
    incrementalSearch: true
    setCursorToStartOfChangeOnUndoRedo: true
    showCursorInVisualMode: false
    useClipboardAsDefaultRegister: true
    useSmartcaseForSearch: true
t9md commented 8 years ago

Thanks for investigation. So you didn't update vmp and atom, but you suddenly search mini editor become not popping up and its always happen after 1st occaision right?

When you didn't update version but issue suddenly appear, the cause should be normaly your environment change..

Can you open search mini editor with ?.

How about invoking vmp:search from command palette?

Can you use mark feature ma to mark a then 'a to jump to mark line.Since mark also use mini-editor input.

jordwalke commented 8 years ago

I even tried reverting to a previous version and even that didn't solve it! It must be something about my setup that prevents the mini editor from showing. What could it be and how could I debug what the cause is.

How can I debug why the mini editor is not showing up? What part can I step debug?

t9md commented 8 years ago

Yes very strange phenomena. I think I can fix error itself. But showing mini-editor things is no idea for now. Why this error throwed is 2nd operation is pushed on operationStack before 1st search motion completed(processed) and removed from operation stack I believe. Normaly when operation remaining to stack must operator to combine 2nd motion of text-object, in this case Operator::setTarget is always available.

How is command-palette invocation?

jordwalke commented 8 years ago

Oh, the command pallet invocation is the same - it doesn't bring up the mini search editor and the following keystroke cause the error.

t9md commented 8 years ago

So its not keybinding issue. Officiak vim-mode also use search mini-editor and my another package like lazy-motion use mini-editor with similar code to vmp. Maybe trying these package to check mini-editor might give hint(if it worked!).

jordwalke commented 8 years ago

Found out what the error was! I disabled the shadow DOM in Atom's settings. Must we use the shadow DOM?

t9md commented 8 years ago

Yes shadow-dom is enabled and config parameter would be removed in future(IFAIRC) and I'm assuming enabling shadow-dom on every package I maintain. I wanted to point it out in earlier response. Anyway congrats!!