Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update diagnostics gutter icons and change default to "sign" #2086

Merged
merged 4 commits into from
Oct 14, 2022
Merged

Update diagnostics gutter icons and change default to "sign" #2086

merged 4 commits into from
Oct 14, 2022

Conversation

jwortmann
Copy link
Member

This is a suggestion to ...

  1. Change the default diagnostic icon style to "sign", to let users who doesn't bother with looking through and adjusting the setting values to enjoy the superior icon style by default :)
  2. Replace the old icons with GitHub's Octicons, which imo look better and seem a little bit more sharp at the smallest icon size (they use a larger stroke width and I made them a little bit (1px) larger than the old icons).
    Bonus: they use a much simpler license (MIT)
  3. Never show the gutter icon for the "hint" severity (also for "dot" and other styles), which according to Hidden DiagnosticSeverity? microsoft/language-server-protocol#325 (comment) and other comments in that thread shouldn't really be rendered as diagnostic in the editor, but for example to fade out unused code. VSCode doesn't even draw the underline or show them in the problems panel, but here I kept the dotted underline due to the lack of a good ST API for foreground color dimming. At the moment, there is no visible difference between "info" and "hint" severity in Sublime LSP.

Comparison (on Full HD display with font size 10.5, and on the bottom with UI scale 2.0)

Before:

before
before2

After:

after
after2

@rchl
Copy link
Member

rchl commented Oct 12, 2022

The new icons do overlap the line number a bit here. Possibly because of being 1px larger

Before:

Screenshot 2022-10-12 at 21 34 02

After:

Screenshot 2022-10-12 at 21 32 27

@jwortmann
Copy link
Member Author

Okay, I reverted to the old size, which is ~13x13px for the icon, 1px for (transparent) left & top border and 2px for right and bottom border. I think they still look slightly better than the old icons, due to the bigger stroke width (and the lightbulb follows the same design as the diagnostic icons).

old
new

rchl
rchl approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@rchl rchl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but I would like to ask @rwols @predragnikolic if they have something against changes default value of diagnostics_gutter_marker.

icons/convert.bat
inkscape --export-type=png --export-area=-1:-1:18:18 -w 16 -h 16 --export-filename=warning.png warning.svg
inkscape --export-type=png --export-area=-1:-1:18:18 -w 32 -h 32 --export-filename=warning@2x.png warning.svg
inkscape --export-type=png --export-area=-1:-1:18:18 -w 48 -h 48 --export-filename=warning@3x.png warning.svg
inkscape --export-type=png --export-area=-1.23:-1.23:18.46:18.46 -w 16 -h 16 --export-filename=error.png error.svg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think WIndows users working on LSP are in minority but I guess those commands can easily be copied and executed in the terminal.

Though if you have a setup that allows running shell scripts then consider that instead. :)

predragnikolic
predragnikolic approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@predragnikolic predragnikolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@predragnikolic
Copy link
Member

Yeah, looks better.

Not related to this PR,
I wish that the "dot", "circle", "bookmark" options were removed entirely :D (maybe leave dot or circle)

@rchl rchl changed the title Update gutter icons and change default to "sign" Update diagnostics gutter icons and change default to "sign" Oct 14, 2022
@rchl rchl merged commit fbbfa5a into sublimelsp:main Oct 14, 2022
@jwortmann jwortmann deleted the gutter-icons branch October 14, 2022 14:34
rchl added a commit that referenced this pull request Oct 18, 2022
@rchl
Merge remote-tracking branch 'origin/main' into fix/windows-global
2ac298a 
* origin/main:
  Update diagnostics gutter icons and change default to "sign" (#2086)
  Fix short color box rendering bug after color presentation change (#2087)
rchl added a commit that referenced this pull request Jan 16, 2023
@rchl
Merge branch 'main' into feat/diag-annotations
f9f1c50 
* main:
  Focus symbol closest to selection on showing document symbols
  Properly handle disabling of the LSP package (#2085)
  fix issues with restarting servers (#2088)
  Add `$line` and `$character` expansion variables (#2092)
  Only enable Goto Diagnostic commands if diagnostic with configured severity exists (#2091)
  Update diagnostics gutter icons and change default to "sign" (#2086)
  Fix short color box rendering bug after color presentation change (#2087)
@jwortmann jwortmann mentioned this pull request Jan 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rchl rchl rchl approved these changes

@predragnikolic predragnikolic predragnikolic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jwortmann @rchl @predragnikolic