facebookarchive / atom-ide-ui

A collection of user interfaces for Atom IDE.
http://ide.atom.io
Other
934 stars 79 forks source link

On-save result markers are re-applied with on-update results #70

Closed Arcanemagus closed 6 years ago

Arcanemagus commented 7 years ago

Description

The markers in the editor from linter providers that only update when a file is saved are being re-applied when results come in from providers that update with the document.

Expected Behavior

Markers should only update when their associated message changes, or Atom invalidates the marker due to the underlying text changing.

Actual Behavior

Markers are re-applied on every update of messages based on their original ranges, regardless of how accurate that is.

2017-10-03_14-39-29

Note how the wavy underline (linter-ui-default) follows the original text and doesn't update while atom-ide-diagnostics-ui's markers are re-applied on the text when the on-update linter returns new results.

Versions

Additional Details

Installed packages ``` Dev Packages (102) C:\Users\Landon Abney\.atom\dev\packages ├── atom-languageclient@0.1.0 ├── intentions@1.1.2 (disabled) ├── linter@2.2.0 ├── linter-ansible-lint@0.0.7 ├── linter-ansible-linting@1.2.3 ├── linter-ansible-syntax@1.1.3 ├── linter-bailey@0.2.2 ├── linter-bootlint@1.1.0 ├── linter-clang@4.1.2 ├── linter-clang-check@4.1.2 ├── linter-clojure@1.2.0 ├── linter-codeclimate@0.2.5 ├── linter-codscriptizer@0.2.0 ├── linter-coffeelint@1.3.1 ├── linter-cppcheck@0.2.2 ├── linter-cpplint@2.0.1 ├── linter-csslint@2.0.0 ├── linter-dartanalyzer@0.3.2 ├── linter-docker@0.2.1 ├── linter-elixirc@1.7.0 ├── linter-erb@1.1.0 ├── linter-erlc@0.4.0 ├── linter-eslint@8.3.2 ├── linter-flake8@2.2.1 ├── linter-flexpmd@0.1.9 ├── linter-flint@1.0.1 ├── linter-flow@5.6.1 ├── linter-foodcritic@0.5.4 ├── linter-gettext@0.1.3 ├── linter-gfortran@0.5.0 ├── linter-gjslint@1.5.2 ├── linter-glsl@2.1.4 ├── linter-golinter@1.2.2 ├── linter-gotype@0.1.0 ├── linter-govet@0.1.5 ├── linter-haml@2.0.2 ├── linter-harbour@6.6.1 ├── linter-hlint@2.0.0 ├── linter-htmlhint@1.3.4 ├── linter-icinga-validate@1.0.0 ├── linter-javac@1.9.4 ├── linter-jolie@0.7.1 ├── linter-js-yaml@1.2.8 ├── linter-jscs@4.1.3 (disabled) ├── linter-jshint@3.1.6 ├── linter-jsl@0.0.1 ├── linter-jsonlint@1.3.0 ├── linter-kibit@0.0.1 ├── linter-lintr@1.1.4 ├── linter-lsc@2.4.0 ├── linter-lua@1.0.2 ├── linter-luacheck@2.0.1 ├── linter-markdown@5.2.0 ├── linter-moonscript@1.1.1 ├── linter-oclitnt@0.0.3 ├── linter-packer-validate@1.0.1 ├── linter-perl@0.8.1 ├── linter-perlcritic@2.0.0 ├── linter-php@1.5.1 ├── linter-phpcs@1.6.7 ├── linter-phplint@0.1.0 ├── linter-phpmd@2.0.0 ├── linter-proselint@3.3.0 ├── linter-pug@1.3.1 ├── linter-puppet-lint@0.8.3 ├── linter-puppet-parsing@1.0.4 ├── linter-pycodestyle@2.1.3 ├── linter-pydocstyle@0.4.6 ├── linter-pyflakes@0.3.2 ├── linter-pylama@0.9.2 ├── linter-pylint@2.1.0 ├── linter-reek@2.2.2 ├── linter-roodi@1.1.0 ├── linter-rst@0.0.3 ├── linter-rubocop@2.1.1 ├── linter-ruby@1.2.6 ├── linter-ruby-lint@1.2.0 ├── linter-rust@0.8.0 ├── linter-sass-lint@1.7.6 ├── linter-scalac@1.4.4 ├── linter-scalastyle@1.4.1 ├── linter-scss-lint@3.1.1 ├── linter-shellcheck@1.4.6 ├── linter-slim@1.0.0 ├── linter-spell@0.13.2 ├── linter-spell-html@0.6.1 ├── linter-spell-javascript@0.7.1 ├── linter-spell-latex@0.9.1 ├── linter-spell-project@0.1.3 ├── linter-spell-ruby@0.3.1 ├── linter-stylelint@4.0.2 ├── linter-stylint@2.2.8 ├── linter-swagger@0.4.2 ├── linter-swiftc@2.0.0 ├── linter-swiftlint@1.2.2 ├── linter-terraform-syntax@1.1.1 ├── linter-tidy@2.3.1 ├── linter-tslint@1.8.1 ├── linter-ui-default@1.6.10 ├── linter-xmllint@1.4.1 ├── minimap-linter@2.1.1 └── squirrel-linter@0.3.3 Community Packages (102) C:\Users\Landon Abney\.atom\packages ├── atom-beautify@0.30.5 ├── atom-ide-ui@0.4.0 ├── atom-jade@0.3.0 ├── atom-material-syntax@1.0.6 ├── atom-material-syntax-dark@1.0.0 ├── atom-material-syntax-light@0.4.6 ├── atom-material-ui@2.0.4 ├── atom-typescript@11.0.9 (disabled) ├── autocomplete-lua@0.9.0 ├── busy-signal@1.4.3 (disabled) ├── docblockr@0.11.0 ├── editorconfig@2.2.2 ├── file-watcher@1.2.6 ├── fizzy@0.21.0 ├── flow-ide@1.8.1 (disabled) ├── highlight-selected@0.13.1 ├── hyperclick@0.1.5 (disabled) ├── ide-flowtype@0.17.4 ├── ide-php@0.6.9 ├── ide-python@0.2.1 ├── ide-typescript@0.6.1 ├── intentions@1.1.5 (disabled) ├── language-ansible@0.2.1 ├── language-babel@2.75.0 (disabled) ├── language-chef@0.10.0 ├── language-cjson@0.0.1 ├── language-docker@1.1.8 ├── language-elixir@0.20.3 ├── language-erlang@3.2.0 ├── language-fortran@2.1.6 ├── language-gettext@0.6.1 ├── language-glsl@2.0.2 ├── language-haml@0.25.2 ├── language-haskell@1.14.2 ├── language-icinga2@0.3.0 ├── language-ini@1.19.0 ├── language-jade@0.7.2 ├── language-jolie@0.5.0 ├── language-kotlin@0.5.0 ├── language-livescript@0.0.3 ├── language-lua@0.9.11 ├── language-moonscript@1.7.1 ├── language-postcss@1.3.1 ├── language-powershell@4.0.0 ├── language-puppet@0.23.0 ├── language-r@0.4.2 ├── language-reg@0.0.0 ├── language-rust@0.4.12 ├── language-swift@0.5.0 ├── language-terraform@0.8.1 ├── language-vue@0.23.1 ├── linter@2.2.0 ├── linter-alex@4.0.0 ├── linter-clang@4.1.2 ├── linter-coffeelint@1.3.1 ├── linter-csslint@2.0.0 ├── linter-eslint@8.3.2 ├── linter-flake8@2.2.1 ├── linter-htmlhint@1.3.4 ├── linter-js-standard@4.0.2 ├── linter-js-yaml@1.2.8 ├── linter-jshint@3.1.6 ├── linter-jsonlint@1.3.0 ├── linter-markdown@5.2.0 ├── linter-perl@0.8.1 ├── linter-perlcritic@2.0.0 ├── linter-php@1.5.1 ├── linter-phpcs@1.6.7 ├── linter-phpmd@2.0.0 ├── linter-proselint@3.3.0 ├── linter-pylama@0.9.4 ├── linter-pylint@2.1.0 ├── linter-rails-best-practices@0.2.2 (disabled) ├── linter-reek@2.2.2 ├── linter-rubocop@2.2.0 ├── linter-ruby@1.2.6 ├── linter-stylelint@4.0.2 ├── linter-tidy@2.3.1 ├── linter-ui-default@1.6.10 ├── linter-ui-plus@0.3.2 (disabled) ├── mapfile-grammar@0.1.0 ├── merge-conflicts@1.4.5 (disabled) ├── minimap@4.29.7 ├── minimap-git-diff@4.3.1 ├── minimap-highlight-selected@4.6.1 ├── minimap-linter@2.1.1 ├── minimap-selection@4.5.0 ├── monokai@0.24.0 ├── nord-atom-syntax@0.9.1 ├── nord-atom-ui@0.11.0 ├── pigments@0.40.2 ├── preview-inline@1.4.7 ├── sequential-number@0.5.0 ├── seti-syntax@1.1.3 ├── seti-ui@1.9.0 ├── sort-lines@0.15.0 ├── Stylus@3.1.1 ├── svn@0.0.13 (disabled) ├── tab-control@0.6.10 ├── toggle-quotes@1.0.1 ├── town-crier@0.2.1 └── trailing-spaces@0.4.0 ```
Arcanemagus commented 7 years ago

This points to a deeper issue with how atom-ide-diagnostics-ui must be working, since Markers should never be being re-applied like this regardless of the provider type. You should be relying on Atom to keep them associated with the text and only removing them when the diagnostics provider tells you the message is no longer applicable.

hansonw commented 7 years ago

Yeah I think I've noticed some wonkiness with datatips over old diagnostic markers too... will try to look into it (or maybe @matthewwithanm will)

Arcanemagus commented 7 years ago

Looked into this a little bit and it seems that Markers are currently being managed here, so that logic would likely need to be moved to atom-ide-diagnostics-ui as a start.

hansonw commented 6 years ago

Revisiting this since it's become more noticeable for us - I guess the problem is that we destroy all markers and re-create them based on the known diagnostic state after every update in https://github.com/facebook-atom/atom-ide-ui/blob/master/modules/atom-ide-ui/pkg/atom-ide-diagnostics-ui/lib/gutter.js.

You were onto something with the MessageRangeTracker though - we only use that to track fix locations but it has exactly the same problem.

It would probably be reasonable to track marker <-> diagnostic mappings centrally so that we only destroy/create markers as necessary - and then we use that as the single source of truth for diagnostic locations. It may also be sensible to do things like invalidate a diagnostic if its marker becomes invalidated.

hansonw commented 6 years ago

Fixed in d4fdb80e1bc8d3c4f53c6be51100143109c9d7ad!