Uh oh!
There was an error while loading. Please reload this page.
[NETBEANS-3440] Allow to define a palette for fontcolors XML Schema. - #1661
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } else if (name.equals(E_COLORREF)) { | ||
| String nameAttr = attributes.getValue(A_NAME); | ||
| String colorAttr = attributes.getValue(A_COLOR); | ||
| String oldRef = colorrefs.put(nameAttr, colorAttr); |
There was a problem hiding this comment.
Should null-check and check validity of nameAttr/coloAttr before putting them in the map
There was a problem hiding this comment.
Add a check that nameAttr is not, in fact, a valid color string. This could lead to very confusing results otherwise, e.g. redefining "ff0000" by accident. Perhaps require color references to use some special syntax, e.g. a color defined as "editorBackground" must be referenced "$editorBackground" (do colorrefs.put("$" + nameAttr, colorAttr) to achieve this). Perhaps also require the names to follow Java identifier requirements, e.g. start with Character.isJavaIdentifierStart and every subsequent character must be Character.isJavaIdentifierPart.
There was a problem hiding this comment.
Null check is actually done by the parser as both name and color attributes are requested. If they are missing we got a parser exception first before we could check for null.
As of masking predefined colors, I do not see any problem with that. These color definitions is local to the file where they are defined. The whole idea is coming from my attempt to port https://github.com/morhetz/gruvbox/ to NetBeans. In order to support both light and dark mode, I'd just flip the palette definitions.
As of add a distinctive syntax when referring the color definitions. I was thinking about it as well, though I'm natural on that one. The only reason for opting that out is only that less typing makes me happy.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| <!ELEMENT fontscolors ( colorref*, fontcolor* ) > | ||
| <!ATTLIST fontscolors > | ||
| <!ELEMENT colorref EMPTY > |
There was a problem hiding this comment.
As before, maybe call this colordef rather than colorref.
There was a problem hiding this comment.
colordef is better, idea accepted! Thank you!
da49c24 to
21f0cdaCompare21f0cda to
a302127Compareneilcsmith-net
commented
Nov 27, 2019
Bit tied up with other things to review properly, but +1 to the principle - great idea! |
This one contains a new DTD which probably have to be installed on netbeans.apache.org/dtds/ as well.
It might help for #1652 in the future.