RepreZen / KaiZen-OpenAPI-Editor

Eclipse Editor for the Swagger-OpenAPI Description Language
Eclipse Public License 1.0
115 stars 12 forks source link

[ZEN-4360] OAS3 Syntax Coloring: some characters colored black #474

Closed ghillairet closed 5 years ago

ghillairet commented 6 years ago

The problem seemed to be that paths were never recoginzed as keys (which they should have been). That's why paths always had a different color that other keys. I was still seeing the last closing bracket before a colon to have a slightly different color than the rest of the path. Adding a special rule to handle path, based on the KeyRule but with a modified regex to account for brackets and slashes seems to fix the problem. The PathRule will highligh paths as scalar so that paths have different color than other keys.

What I am seeing in latest build:

screen shot 2018-09-05 at 12 07 12

What I am seeing with this PR fix:

screen shot 2018-09-05 at 12 05 41

What I am seeing if paths have same color than keys:

screen shot 2018-09-05 at 12 05 27

tedepstein commented 6 years ago

@ghillairet , it's actually good that we have an option to make paths a different color. It helps to emphasize the structure of the document, since Path Items are important, high-level constructs.

Can we try mapping PathRule to the theme key field, or keyword?

I think our default Inkpot theme is Inkpot Zen 2, which would give us these color values: image

ghillairet commented 6 years ago

@tedepstein It seems possible to add a that mapping. I did try with keyword and field but kept field as it looks better on our theme.

A small issue I should probably fix is that media types seem to be also recognised as paths, so they have the same color.

Here is what it looks like with keyword:

screen shot 2018-09-05 at 17 31 42

screen shot 2018-09-05 at 17 31 48

And with field:

screen shot 2018-09-05 at 17 33 50

screen shot 2018-09-05 at 17 33 55

Also we should probably modify the color preference page to include this new preference. Now it looks like this. But it can't be done easily as we are using a class from YEdit and some method required to add a new preference is set as private, so we would need to re-create the whole preference page ourself. Maybe it would make sense then to provide preferences that more meaningless to Swagger/OpenAPI dialect.

screen shot 2018-09-05 at 17 11 27

tedepstein commented 6 years ago

@ghillairet , thanks for the screenshots. I think we should leave mapped the same as KeyRule for now, for a couple of reasons:

  1. It didn't occur to me that this same rule would also end up applying to media types, because they match the same regex.
  2. The purple field color is too dark; it doesn't stand out the way it should.
  3. I agree, if and when we do add a separate color mapping for PathRule, we should support it in color preferences, and this is not an easy thing to do right now.
ghillairet commented 6 years ago

@tedepstein @tfesenko I updated the PR to fix the key regex. It now correctly identifies media types as keys, and also references ($ref) keys that were also getting a different color.

I left the PathRule but it now uses the same color as other keys. In previous commits I added a preference for path color, maybe I can remove it from this PR or leave it for latter if we decide to provide enhance color preferences.

Swagger:

screen shot 2018-09-06 at 10 05 55

OpenAPI:

screen shot 2018-09-06 at 10 06 28

tedepstein commented 6 years ago

I left the PathRule but it now uses the same color as other keys. In previous commits I added a preference for path color, maybe I can remove it from this PR or leave it for later if we decide to provide enhance color preferences.

If you have already added that preference, you can leave it.

The Inkpot Zen 2 theme also shows these high-contrast colors for enum and constant theme keys: image

I think we added those for Xtend template strings.

image

Could we try mapping PathKeyRule to enum or constant?

ghillairet commented 6 years ago

@tfesenko We leave path mapped as keys, so this PR is ready to review.

ghillairet commented 5 years ago

@tfesenko I removed it. You can merge if it's ok for you.