New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to unrevert: Skip over tag attributes when selecting inside tags #312

Merged
merged 7 commits into from Sep 13, 2017

Conversation

Projects
None yet
3 participants
@ungb
Contributor

ungb commented Aug 31, 2017

NOTE: this was an attempt to cherry pick changes from #302 since I had to revert these changes previous to revert a previous change.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

When selecting inside tags, grab the full range of the tag rather than just the tag name range. This change allows the attributes to be skipped, leading to the actual area between tags being highlighted.

Alternate Designs

None.

Benefits

Using the bracket-matcher:select-inside-brackets command will work correctly for tags with attributes.

Possible Drawbacks

There may be a slight performance decrease compared to before when the cursor is currently on a tag. This is because we have to re-search for the tags in order to get the full range (i.e. there is no way that I know of to grab the full range from the existing range).

Applicable Issues

Fixes #117

/cc @50Wliu @Ben3eeE

Show outdated Hide outdated lib/tag-finder.coffee Outdated
Show outdated Hide outdated lib/tag-finder.coffee Outdated

Ben3eeE added some commits Aug 31, 2017

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Aug 31, 2017

Member

Now it's just the specs for bracket-match:select-inside-brackets that are failing 🎉

Member

Ben3eeE commented Aug 31, 2017

Now it's just the specs for bracket-match:select-inside-brackets that are failing 🎉

Show outdated Hide outdated lib/tag-finder.coffee Outdated
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Sep 5, 2017

Member

@ungb The specs seems to be passing now but there is a hang from these changes that I discovered that we need to investigate before merging. I told @50Wliu about it in a PM.

After that I think we should test master carefully to ensure there are no other performance regressions.

Member

Ben3eeE commented Sep 5, 2017

@ungb The specs seems to be passing now but there is a hang from these changes that I discovered that we need to investigate before merging. I told @50Wliu about it in a PM.

After that I think we should test master carefully to ensure there are no other performance regressions.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 5, 2017

Member

endPosition = @editor.getLastCursor().getCurrentWordBufferRange({wordRegex: @endOfTagRegex}).end. I believe that change was from #304.

Member

50Wliu commented Sep 5, 2017

endPosition = @editor.getLastCursor().getCurrentWordBufferRange({wordRegex: @endOfTagRegex}).end. I believe that change was from #304.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Sep 6, 2017

Member

This looks like it's ready to go. I can test it out after #314 has been fixed so we can release both this and the self closing tags when we bump it on atom/atom.

Member

Ben3eeE commented Sep 6, 2017

This looks like it's ready to go. I can test it out after #314 has been fixed so we can release both this and the self closing tags when we bump it on atom/atom.

@Ben3eeE Ben3eeE self-assigned this Sep 6, 2017

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Sep 13, 2017

Contributor

@50Wliu @Ben3eeE I've merged #315, Are you guys ok with me merging this change after rerunning test?

Contributor

ungb commented Sep 13, 2017

@50Wliu @Ben3eeE I've merged #315, Are you guys ok with me merging this change after rerunning test?

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Sep 13, 2017

Member

@ungb I want to run some manual testing on this to ensure no regressions before merging.

Member

Ben3eeE commented Sep 13, 2017

@ungb I want to run some manual testing on this to ensure no regressions before merging.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Sep 13, 2017

Member

@50Wliu I'm seeing an exception

Atom: 1.20.0 x64
Electron: 1.6.9
OS: Microsoft Windows 10 Pro
Thrown From: bracket-matcher package 0.87.3

Stack Trace

Uncaught TypeError: Cannot read property 'startRange' of null

At I:\atom_master\bracket-matcher\lib\bracket-matcher-view.coffee:271

TypeError: Cannot read property 'startRange' of null
    at BracketMatcherView.module.exports.BracketMatcherView.selectInsidePair (I:/atom_master/bracket-matcher/lib/bracket-matcher-view.coffee:271:10)
    at HTMLElement.<anonymous> (I:/atom_master/bracket-matcher/lib/bracket-matcher-view.coffee:36:8)
    at CommandRegistry.module.exports.CommandRegistry.handleCommandEvent (~/AppData/Local/atom/app-1.20.0/resources/app/src/command-registry.js:265:35)
    at ~/AppData/Local/atom/app-1.20.0/resources/app/src/command-registry.js:3:65
    at Object.didConfirmSelection (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/command-palette/lib/command-palette-view.js:106:35)
    at SelectListView.confirmSelection (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/atom-select-list/src/select-list-view.js:313:26)
    at HTMLDivElement.core:confirm (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/atom-select-list/src/select-list-view.js:81:20)
    at CommandRegistry.module.exports.CommandRegistry.handleCommandEvent (~/AppData/Local/atom/app-1.20.0/resources/app/src/command-registry.js:265:35)
    at ~/AppData/Local/atom/app-1.20.0/resources/app/src/command-registry.js:3:65
    at KeymapManager.module.exports.KeymapManager.dispatchCommandEvent (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/atom-keymap/lib/keymap-manager.js:621:22)
    at KeymapManager.module.exports.KeymapManager.handleKeyboardEvent (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/atom-keymap/lib/keymap-manager.js:412:28)
    at WindowEventHandler.module.exports.WindowEventHandler.handleDocumentKeyEvent (~/AppData/Local/atom/app-1.20.0/resources/app/src/window-event-handler.js:100:42)
    at HTMLDocument.<anonymous> (~/AppData/Local/atom/app-1.20.0/resources/app/src/window-event-handler.js:3:65)

Commands

     -0:23.3.0 core:delete (input.hidden-input)
 14x -0:22.4.0 core:move-right (input.hidden-input)
     -0:21.4.0 intentions:highlight (input.hidden-input)
     -0:21.1.0 command-palette:toggle (input.hidden-input)
     -0:20.3.0 core:confirm (input.hidden-input)
     -0:20.3.0 bracket-matcher:select-inside-brackets (input.hidden-input)
     -0:19.2.0 command-palette:toggle (input.hidden-input)
     -0:18.7.0 core:confirm (input.hidden-input)
     -0:18.7.0 bracket-matcher:select-inside-brackets (input.hidden-input)
     -0:14.8.0 command-palette:toggle (input.hidden-input)
     -0:14.5.0 core:confirm (input.hidden-input)
     -0:14.5.0 bracket-matcher:select-inside-brackets (input.hidden-input)
     -0:08.7.0 intentions:highlight (input.hidden-input)
     -0:08.6.0 command-palette:toggle (input.hidden-input)
     -0:08.3.0 core:confirm (input.hidden-input)
     -0:08.3.0 bracket-matcher:select-inside-brackets (input.hidden-input)

Non-Core Packages

advanced-open-file 0.16.6 
atom-beautify 0.30.5 
atom-dark-fusion-syntax 2.2.0 
atom-ide-ui 0.1.11 
atom-material-syntax 1.0.6 
atom-material-ui undefined 
autocomplete-emojis 2.5.0 
busy-signal 1.4.3 
chester-atom-syntax 0.3.0 
chestnut-light-atom-syntax 0.2.1 
color-picker 2.2.5 
editor-stats 0.17.0 
fizzy 0.21.0 
golden-ratio 0.3.0 
hey-pane 1.0.0 
highlight-selected 0.13.1 
honor-altgraph 0.1.0 
intentions 1.1.5 
language-latex 1.1.1 
language-markdown 0.25.1 
latex 0.46.0 
less-than-slash 0.17.0 
linter 2.2.0 
linter-js-standard 4.0.0 
linter-ui-default 1.6.8 
markdown-pdf 1.5.4 
minimap 4.29.6 
minimap-autohider 1.5.3 
minimap-find-and-replace 4.5.2 
minimap-highlight-selected 4.6.1 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
no-caffeine-syntax 0.18.1 
nucleus-dark-ui 0.12.3 
pastel-jelly-syntax 0.2.0 
php-cs-fixer 4.1.0 
pigments 0.40.2 
platformio-ide-terminal 2.5.5 
preview-inline 1.4.7 
remote-edit 1.9.0 
seti-syntax 1.1.3 
seti-ui 1.9.0 
Sublime-Style-Column-Selection 1.7.4 
tab-title 0.3.5 
toggler 0.3.0 
vim-mode-plus 1.3.3 
zentabs 0.8.8 
Member

Ben3eeE commented Sep 13, 2017

@50Wliu I'm seeing an exception

Atom: 1.20.0 x64
Electron: 1.6.9
OS: Microsoft Windows 10 Pro
Thrown From: bracket-matcher package 0.87.3

Stack Trace

Uncaught TypeError: Cannot read property 'startRange' of null

At I:\atom_master\bracket-matcher\lib\bracket-matcher-view.coffee:271

TypeError: Cannot read property 'startRange' of null
    at BracketMatcherView.module.exports.BracketMatcherView.selectInsidePair (I:/atom_master/bracket-matcher/lib/bracket-matcher-view.coffee:271:10)
    at HTMLElement.<anonymous> (I:/atom_master/bracket-matcher/lib/bracket-matcher-view.coffee:36:8)
    at CommandRegistry.module.exports.CommandRegistry.handleCommandEvent (~/AppData/Local/atom/app-1.20.0/resources/app/src/command-registry.js:265:35)
    at ~/AppData/Local/atom/app-1.20.0/resources/app/src/command-registry.js:3:65
    at Object.didConfirmSelection (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/command-palette/lib/command-palette-view.js:106:35)
    at SelectListView.confirmSelection (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/atom-select-list/src/select-list-view.js:313:26)
    at HTMLDivElement.core:confirm (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/atom-select-list/src/select-list-view.js:81:20)
    at CommandRegistry.module.exports.CommandRegistry.handleCommandEvent (~/AppData/Local/atom/app-1.20.0/resources/app/src/command-registry.js:265:35)
    at ~/AppData/Local/atom/app-1.20.0/resources/app/src/command-registry.js:3:65
    at KeymapManager.module.exports.KeymapManager.dispatchCommandEvent (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/atom-keymap/lib/keymap-manager.js:621:22)
    at KeymapManager.module.exports.KeymapManager.handleKeyboardEvent (~/AppData/Local/atom/app-1.20.0/resources/app/node_modules/atom-keymap/lib/keymap-manager.js:412:28)
    at WindowEventHandler.module.exports.WindowEventHandler.handleDocumentKeyEvent (~/AppData/Local/atom/app-1.20.0/resources/app/src/window-event-handler.js:100:42)
    at HTMLDocument.<anonymous> (~/AppData/Local/atom/app-1.20.0/resources/app/src/window-event-handler.js:3:65)

Commands

     -0:23.3.0 core:delete (input.hidden-input)
 14x -0:22.4.0 core:move-right (input.hidden-input)
     -0:21.4.0 intentions:highlight (input.hidden-input)
     -0:21.1.0 command-palette:toggle (input.hidden-input)
     -0:20.3.0 core:confirm (input.hidden-input)
     -0:20.3.0 bracket-matcher:select-inside-brackets (input.hidden-input)
     -0:19.2.0 command-palette:toggle (input.hidden-input)
     -0:18.7.0 core:confirm (input.hidden-input)
     -0:18.7.0 bracket-matcher:select-inside-brackets (input.hidden-input)
     -0:14.8.0 command-palette:toggle (input.hidden-input)
     -0:14.5.0 core:confirm (input.hidden-input)
     -0:14.5.0 bracket-matcher:select-inside-brackets (input.hidden-input)
     -0:08.7.0 intentions:highlight (input.hidden-input)
     -0:08.6.0 command-palette:toggle (input.hidden-input)
     -0:08.3.0 core:confirm (input.hidden-input)
     -0:08.3.0 bracket-matcher:select-inside-brackets (input.hidden-input)

Non-Core Packages

advanced-open-file 0.16.6 
atom-beautify 0.30.5 
atom-dark-fusion-syntax 2.2.0 
atom-ide-ui 0.1.11 
atom-material-syntax 1.0.6 
atom-material-ui undefined 
autocomplete-emojis 2.5.0 
busy-signal 1.4.3 
chester-atom-syntax 0.3.0 
chestnut-light-atom-syntax 0.2.1 
color-picker 2.2.5 
editor-stats 0.17.0 
fizzy 0.21.0 
golden-ratio 0.3.0 
hey-pane 1.0.0 
highlight-selected 0.13.1 
honor-altgraph 0.1.0 
intentions 1.1.5 
language-latex 1.1.1 
language-markdown 0.25.1 
latex 0.46.0 
less-than-slash 0.17.0 
linter 2.2.0 
linter-js-standard 4.0.0 
linter-ui-default 1.6.8 
markdown-pdf 1.5.4 
minimap 4.29.6 
minimap-autohider 1.5.3 
minimap-find-and-replace 4.5.2 
minimap-highlight-selected 4.6.1 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
no-caffeine-syntax 0.18.1 
nucleus-dark-ui 0.12.3 
pastel-jelly-syntax 0.2.0 
php-cs-fixer 4.1.0 
pigments 0.40.2 
platformio-ide-terminal 2.5.5 
preview-inline 1.4.7 
remote-edit 1.9.0 
seti-syntax 1.1.3 
seti-ui 1.9.0 
Sublime-Style-Column-Selection 1.7.4 
tab-title 0.3.5 
toggler 0.3.0 
vim-mode-plus 1.3.3 
zentabs 0.8.8 
@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Sep 13, 2017

Contributor

Wonder if this could of been caused by me merging master into this.

Contributor

ungb commented Sep 13, 2017

Wonder if this could of been caused by me merging master into this.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Sep 13, 2017

Member

I can reproduce it with the following:

<body class='markdown-preview' data-use-github-style>
    <span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span></body>

And placing the cursor on the end of the first line and running bracket-matcher:select-inside-brackets.

Member

Ben3eeE commented Sep 13, 2017

I can reproduce it with the following:

<body class='markdown-preview' data-use-github-style>
    <span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span><span></span></body>

And placing the cursor on the end of the first line and running bracket-matcher:select-inside-brackets.

Use findStartEndTags to avoid scope check
Scopes aren't fully tokenized on very long lines
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Sep 13, 2017

Member

@50Wliu @ungb This looks good now 👍 🚢

Member

Ben3eeE commented Sep 13, 2017

@50Wliu @ungb This looks good now 👍 🚢

@Ben3eeE Ben3eeE merged commit b32e162 into master Sep 13, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the pr-302-changes branch Sep 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment