GroovyTagScanner and IHighlightingExtender weakness

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

GroovyTagScanner and IHighlightingExtender weakness

Maxime HAMM
Hello Andrew

Few things I kept in my mind for month...

1) I think there is an issue about "additional rules" in the IHighlightingExtender extension : 
  
As far as I was testing... additional rules are never raised ! 

Here is my test case (about JSPRESSO framework) : 
All word starting with string_ or text_ followed by digits have to be displayed as JDK keywords... 
(in JSPRESSO this is a shortcut to specify field's length)

After debugging the GroovyTagScanner I understand why :
There is a default token define for the "CombinedWordRule" used by groovy eclipse to store all
keyword definitions... due to this default token, the scanner is never going further to check the
additional rules !

    @Override
    protected List<IRule> createRules() {
      ...
      // combined rule for all keywords
      JavaWordDetector wordDetector = new JavaWordDetector();
      token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector, token);

I will suggest to replace this by : 

    @Override
    protected List<IRule> createRules() {
      ...
      // combined rule for all keywords
      JavaWordDetector wordDetector = new JavaWordDetector();
      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector, Token.UNDEFINED);

Another alternative is to move the additional rules to the top of the create rules method :

    @Override
    protected List<IRule> createRules() {

      // additional rules
      if (additionalRules != null) {
          rules.addAll(additionalRules);
      }
      ...

      // combined rule for all keywords
      JavaWordDetector wordDetector = new JavaWordDetector();
      token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector, token);

A last alternative, which should cause less regressions, is to do both... I mean adding another
method "getAdditionalTopRules" in the "IHighlightingExtender"... 

What is your opinion about this ?


2) It's not possible to have both... groovy native file using standard groovy color... and one or more groovy dsl 
   with customized colors !

This was not an issue one year ago, but now JSPRESSO supports groovy for business rules ! 
Developers are a little bit embarrassed to have to choose between one or the other...

I will suggest you to make some extension point evolution to allow to parameterize extender with an extension file ?

Something like : 

<extension point="org.codehaus.groovy.eclipse.ui.syntaxHighlightingExtension">
  <highlightingExtender 
  extender="org.jspresso.contrib.sjsplugin.editor.SJSSyntaxHighlighting" 
  natureID="org.jspresso.contrib.sjsplugin.nature"
                        fileExtension="sjs">
  </highlightingExtender>
</extension>

Is it a good suggestion ?


Thank you for you attention and help,

Maxime



---------------------------------------------------------------

Here is my scanner additional rule :


  /**
   * additional rules to manage string_* and text_* 
   * @author Maxime HAMM
   */
  @Override
  public List<IRule> getAdditionalRules() {

    IPreferenceStore store = GroovyPlugin.getDefault().getPreferenceStore();
    RGB rgb = PreferenceConverter.getColor(store, PreferenceConstants.GROOVY_EDITOR_HIGHLIGHT_GJDK_COLOR);
    IToken keywordToken = new Token(new TextAttribute(new Color(Display.getCurrent(), rgb)));

    WordBeginMatcher matcher = new WordBeginMatcher();
    addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_DOMAIN);
    addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_FRONT);

    JavaWordDetector wordDetector = new JavaWordDetector();
    CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector, Token.UNDEFINED);
    combinedWordRule.addWordMatcher(matcher);

    List<IRule> rules = new ArrayList<IRule>();
    rules.add(combinedWordRule);

    return rules;
  }

  /**
   * WordBeginMatcher
   * Match word with the begining of another using underscore separator
   * @author Maxime HAMM
   */
  private static final class WordBeginMatcher extends WordMatcher {
    private final Map<CharacterBuffer, IToken> words = new HashMap<CharacterBuffer, IToken>();

    @Override
    public void addWord(String word, IToken token) {
      this.words.put(new CharacterBuffer(word), token);
    }

    @Override
    public IToken evaluate(ICharacterScanner scanner, CharacterBuffer word) {
      // look for underscore
      int i=0;
      CharacterBuffer buff = new CharacterBuffer(10);
      for (; i<word.length(); i++) {
        char c = word.charAt(i);
        if (c == '_') {
          i++;
          break;
        }
        buff.append(c);
      }
      if (i == word.length()) {
        return Token.UNDEFINED;
      }

      // check if it is a keyword
      IToken token = this.words.get(buff);
      if (token == null) {
        return Token.UNDEFINED;
      }

      // check if railer is digits only  
      for (; i<word.length(); i++) {
        char c = word.charAt(i);
        if (!Character.isDigit(c)) {
          return Token.UNDEFINED;
        }
      }
      return token;
    }

    @Override
    public void clearWords() {
      this.words.clear();
    }
  }






 


Reply | Threaded
Open this post in threaded view
|

Re: GroovyTagScanner and IHighlightingExtender weakness

Maxime HAMM
Hello 

About my first question above :
Here is a patch proposal for a "IHighlightingExtender2" : the new interface contains a new method "getInitialAdditionalRules" which return a list of rules... those rules are added in first position by the "GroovyTagScanner"
It is working well for Jspresso and I think it could be useful for other extenders... 

About my second question :
I've no idea about the best solution here... if you want and give me some instructions, I can try to work on it with you...

Kind regards, 
Maxime



Hello Andrew

Few things I kept in my mind for month...

1) I think there is an issue about "additional rules" in the IHighlightingExtender extension : 
  
As far as I was testing... additional rules are never raised ! 

Here is my test case (about JSPRESSO framework) : 
All word starting with string_ or text_ followed by digits have to be displayed as JDK keywords... 
(in JSPRESSO this is a shortcut to specify field's length)

After debugging the GroovyTagScanner I understand why :
There is a default token define for the "CombinedWordRule" used by groovy eclipse to store all
keyword definitions... due to this default token, the scanner is never going further to check the
additional rules !

    @Override
    protected List<IRule> createRules() {
      ...
      // combined rule for all keywords
      JavaWordDetector wordDetector = new JavaWordDetector();
      token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector, token);

I will suggest to replace this by : 

    @Override
    protected List<IRule> createRules() {
      ...
      // combined rule for all keywords
      JavaWordDetector wordDetector = new JavaWordDetector();
      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector, Token.UNDEFINED);

Another alternative is to move the additional rules to the top of the create rules method :

    @Override
    protected List<IRule> createRules() {

      // additional rules
      if (additionalRules != null) {
          rules.addAll(additionalRules);
      }
      ...

      // combined rule for all keywords
      JavaWordDetector wordDetector = new JavaWordDetector();
      token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector, token);

A last alternative, which should cause less regressions, is to do both... I mean adding another
method "getAdditionalTopRules" in the "IHighlightingExtender"... 

What is your opinion about this ?


2) It's not possible to have both... groovy native file using standard groovy color... and one or more groovy dsl 
   with customized colors !

This was not an issue one year ago, but now JSPRESSO supports groovy for business rules ! 
Developers are a little bit embarrassed to have to choose between one or the other...

I will suggest you to make some extension point evolution to allow to parameterize extender with an extension file ?

Something like : 

<extension point="org.codehaus.groovy.eclipse.ui.syntaxHighlightingExtension">
  <highlightingExtender 
  extender="org.jspresso.contrib.sjsplugin.editor.SJSSyntaxHighlighting" 
  natureID="org.jspresso.contrib.sjsplugin.nature"
                        fileExtension="sjs">
  </highlightingExtender>
</extension>

Is it a good suggestion ?


Thank you for you attention and help,

Maxime



---------------------------------------------------------------

Here is my scanner additional rule :


  /**
   * additional rules to manage string_* and text_* 
   * @author Maxime HAMM
   */
  @Override
  public List<IRule> getAdditionalRules() {

    IPreferenceStore store = GroovyPlugin.getDefault().getPreferenceStore();
    RGB rgb = PreferenceConverter.getColor(store, PreferenceConstants.GROOVY_EDITOR_HIGHLIGHT_GJDK_COLOR);
    IToken keywordToken = new Token(new TextAttribute(new Color(Display.getCurrent(), rgb)));

    WordBeginMatcher matcher = new WordBeginMatcher();
    addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_DOMAIN);
    addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_FRONT);

    JavaWordDetector wordDetector = new JavaWordDetector();
    CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector, Token.UNDEFINED);
    combinedWordRule.addWordMatcher(matcher);

    List<IRule> rules = new ArrayList<IRule>();
    rules.add(combinedWordRule);

    return rules;
  }

  /**
   * WordBeginMatcher
   * Match word with the begining of another using underscore separator
   * @author Maxime HAMM
   */
  private static final class WordBeginMatcher extends WordMatcher {
    private final Map<CharacterBuffer, IToken> words = new HashMap<CharacterBuffer, IToken>();

    @Override
    public void addWord(String word, IToken token) {
      this.words.put(new CharacterBuffer(word), token);
    }

    @Override
    public IToken evaluate(ICharacterScanner scanner, CharacterBuffer word) {
      // look for underscore
      int i=0;
      CharacterBuffer buff = new CharacterBuffer(10);
      for (; i<word.length(); i++) {
        char c = word.charAt(i);
        if (c == '_') {
          i++;
          break;
        }
        buff.append(c);
      }
      if (i == word.length()) {
        return Token.UNDEFINED;
      }

      // check if it is a keyword
      IToken token = this.words.get(buff);
      if (token == null) {
        return Token.UNDEFINED;
      }

      // check if railer is digits only  
      for (; i<word.length(); i++) {
        char c = word.charAt(i);
        if (!Character.isDigit(c)) {
          return Token.UNDEFINED;
        }
      }
      return token;
    }

    @Override
    public void clearWords() {
      this.words.clear();
    }
  }






 




groovy-eclipse.ihighlightingExtender2.patch (28K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: GroovyTagScanner and IHighlightingExtender weakness

Andrew Eisenberg
Hi Maxime,

Thanks for the patch. I'm having trouble applying it.  Can you
recreate this as a pull request?  This would have the added benefit of
keeping yourself as the author of these changes (good for proper
attribution).

Both #1 and #2 sound reasonable.  I'll have a look at the pull request
when it is available.

On Mon, Jul 8, 2013 at 12:59 PM, Maxime HAMM <[hidden email]> wrote:

> Hello
>
> About my first question above :
>
> Here is a patch proposal for a "IHighlightingExtender2" : the new interface
> contains a new method "getInitialAdditionalRules" which return a list of
> rules... those rules are added in first position by the "GroovyTagScanner"
> It is working well for Jspresso and I think it could be useful for other
> extenders...
>
>
> About my second question :
>
> I've no idea about the best solution here... if you want and give me some
> instructions, I can try to work on it with you...
>
>
> Kind regards,
> Maxime
>
>
>
> Hello Andrew
>
> Few things I kept in my mind for month...
>
> 1) I think there is an issue about "additional rules" in the
> IHighlightingExtender extension :
>
>
> As far as I was testing... additional rules are never raised !
>
> Here is my test case (about JSPRESSO framework) :
> All word starting with string_ or text_ followed by digits have to be
> displayed as JDK keywords...
> (in JSPRESSO this is a shortcut to specify field's length)
>
> After debugging the GroovyTagScanner I understand why :
> There is a default token define for the "CombinedWordRule" used by groovy
> eclipse to store all
> keyword definitions... due to this default token, the scanner is never going
> further to check the
> additional rules !
>
>     @Override
>     protected List<IRule> createRules() {
>       ...
>       // combined rule for all keywords
>       JavaWordDetector wordDetector = new JavaWordDetector();
>       token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
>       CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> token);
>
> I will suggest to replace this by :
>
>     @Override
>     protected List<IRule> createRules() {
>       ...
>       // combined rule for all keywords
>       JavaWordDetector wordDetector = new JavaWordDetector();
>       CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> Token.UNDEFINED);
>
> Another alternative is to move the additional rules to the top of the create
> rules method :
>
>     @Override
>     protected List<IRule> createRules() {
>
>       // additional rules
>       if (additionalRules != null) {
>           rules.addAll(additionalRules);
>       }
>       ...
>
>       // combined rule for all keywords
>       JavaWordDetector wordDetector = new JavaWordDetector();
>       token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
>       CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> token);
>
> A last alternative, which should cause less regressions, is to do both... I
> mean adding another
> method "getAdditionalTopRules" in the "IHighlightingExtender"...
>
> What is your opinion about this ?
>
>
> 2) It's not possible to have both... groovy native file using standard
> groovy color... and one or more groovy dsl
>    with customized colors !
>
> This was not an issue one year ago, but now JSPRESSO supports groovy for
> business rules !
> Developers are a little bit embarrassed to have to choose between one or the
> other...
>
> I will suggest you to make some extension point evolution to allow to
> parameterize extender with an extension file ?
>
> Something like :
>
> <extension
> point="org.codehaus.groovy.eclipse.ui.syntaxHighlightingExtension">
>   <highlightingExtender
>   extender="org.jspresso.contrib.sjsplugin.editor.SJSSyntaxHighlighting"
>   natureID="org.jspresso.contrib.sjsplugin.nature"
>                         fileExtension="sjs">
>   </highlightingExtender>
> </extension>
>
> Is it a good suggestion ?
>
>
> Thank you for you attention and help,
>
> Maxime
>
>
>
> ---------------------------------------------------------------
>
> Here is my scanner additional rule :
>
>
>   /**
>    * additional rules to manage string_* and text_*
>    * @author Maxime HAMM
>    */
>   @Override
>   public List<IRule> getAdditionalRules() {
>
>     IPreferenceStore store = GroovyPlugin.getDefault().getPreferenceStore();
>     RGB rgb = PreferenceConverter.getColor(store,
> PreferenceConstants.GROOVY_EDITOR_HIGHLIGHT_GJDK_COLOR);
>     IToken keywordToken = new Token(new TextAttribute(new
> Color(Display.getCurrent(), rgb)));
>
>     WordBeginMatcher matcher = new WordBeginMatcher();
>     addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_DOMAIN);
>     addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_FRONT);
>
>     JavaWordDetector wordDetector = new JavaWordDetector();
>     CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> Token.UNDEFINED);
>     combinedWordRule.addWordMatcher(matcher);
>
>     List<IRule> rules = new ArrayList<IRule>();
>     rules.add(combinedWordRule);
>
>     return rules;
>   }
>
>   /**
>    * WordBeginMatcher
>    * Match word with the begining of another using underscore separator
>    * @author Maxime HAMM
>    */
>   private static final class WordBeginMatcher extends WordMatcher {
>     private final Map<CharacterBuffer, IToken> words = new
> HashMap<CharacterBuffer, IToken>();
>
>     @Override
>     public void addWord(String word, IToken token) {
>       this.words.put(new CharacterBuffer(word), token);
>     }
>
>     @Override
>     public IToken evaluate(ICharacterScanner scanner, CharacterBuffer word)
> {
>       // look for underscore
>       int i=0;
>       CharacterBuffer buff = new CharacterBuffer(10);
>       for (; i<word.length(); i++) {
>         char c = word.charAt(i);
>         if (c == '_') {
>           i++;
>           break;
>         }
>         buff.append(c);
>       }
>       if (i == word.length()) {
>         return Token.UNDEFINED;
>       }
>
>       // check if it is a keyword
>       IToken token = this.words.get(buff);
>       if (token == null) {
>         return Token.UNDEFINED;
>       }
>
>       // check if railer is digits only
>       for (; i<word.length(); i++) {
>         char c = word.charAt(i);
>         if (!Character.isDigit(c)) {
>           return Token.UNDEFINED;
>         }
>       }
>       return token;
>     }
>
>     @Override
>     public void clearWords() {
>       this.words.clear();
>     }
>   }
>
>
>
>
>
>
>
>
>
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: GroovyTagScanner and IHighlightingExtender weakness

Maxime HAMM
Hi Andrew

I was adding the "pull request"... It's the first time i'm using git and github... hope I did it correctly !

I was also updating tests :
- a new one to check that the rules are raised
- I made a little fix for "SemanticHighlightingTests" because the tests was dependent one the "GROOVY_SEMANTIC_HIGHLIGHTING" preference value...
- I also try to add a tests to check that the "initial rules" takes the priority on other groovy-native rules... but I had difficulty to manage it... may be you will do if this test is necessary ? 

Thanks a lot, 

kind regards,
Maxime

PS : I'm not able to 

Le 8 juil. 2013 à 22:32, Andrew Eisenberg <[hidden email]> a écrit :

Hi Maxime,

Thanks for the patch. I'm having trouble applying it.  Can you
recreate this as a pull request?  This would have the added benefit of
keeping yourself as the author of these changes (good for proper
attribution).

Both #1 and #2 sound reasonable.  I'll have a look at the pull request
when it is available.

On Mon, Jul 8, 2013 at 12:59 PM, Maxime HAMM <[hidden email]> wrote:
Hello

About my first question above :

Here is a patch proposal for a "IHighlightingExtender2" : the new interface
contains a new method "getInitialAdditionalRules" which return a list of
rules... those rules are added in first position by the "GroovyTagScanner"
It is working well for Jspresso and I think it could be useful for other
extenders...


About my second question :

I've no idea about the best solution here... if you want and give me some
instructions, I can try to work on it with you...


Kind regards,
Maxime



Hello Andrew

Few things I kept in my mind for month...

1) I think there is an issue about "additional rules" in the
IHighlightingExtender extension :


As far as I was testing... additional rules are never raised !

Here is my test case (about JSPRESSO framework) :
All word starting with string_ or text_ followed by digits have to be
displayed as JDK keywords...
(in JSPRESSO this is a shortcut to specify field's length)

After debugging the GroovyTagScanner I understand why :
There is a default token define for the "CombinedWordRule" used by groovy
eclipse to store all
keyword definitions... due to this default token, the scanner is never going
further to check the
additional rules !

   @Override
   protected List<IRule> createRules() {
     ...
     // combined rule for all keywords
     JavaWordDetector wordDetector = new JavaWordDetector();
     token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
     CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
token);

I will suggest to replace this by :

   @Override
   protected List<IRule> createRules() {
     ...
     // combined rule for all keywords
     JavaWordDetector wordDetector = new JavaWordDetector();
     CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
Token.UNDEFINED);

Another alternative is to move the additional rules to the top of the create
rules method :

   @Override
   protected List<IRule> createRules() {

     // additional rules
     if (additionalRules != null) {
         rules.addAll(additionalRules);
     }
     ...

     // combined rule for all keywords
     JavaWordDetector wordDetector = new JavaWordDetector();
     token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
     CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
token);

A last alternative, which should cause less regressions, is to do both... I
mean adding another
method "getAdditionalTopRules" in the "IHighlightingExtender"...

What is your opinion about this ?


2) It's not possible to have both... groovy native file using standard
groovy color... and one or more groovy dsl
  with customized colors !

This was not an issue one year ago, but now JSPRESSO supports groovy for
business rules !
Developers are a little bit embarrassed to have to choose between one or the
other...

I will suggest you to make some extension point evolution to allow to
parameterize extender with an extension file ?

Something like :

<extension
point="org.codehaus.groovy.eclipse.ui.syntaxHighlightingExtension">
 <highlightingExtender
 extender="org.jspresso.contrib.sjsplugin.editor.SJSSyntaxHighlighting"
 natureID="org.jspresso.contrib.sjsplugin.nature"
                       fileExtension="sjs">
 </highlightingExtender>
</extension>

Is it a good suggestion ?


Thank you for you attention and help,

Maxime



---------------------------------------------------------------

Here is my scanner additional rule :


 /**
  * additional rules to manage string_* and text_*
  * @author Maxime HAMM
  */
 @Override
 public List<IRule> getAdditionalRules() {

   IPreferenceStore store = GroovyPlugin.getDefault().getPreferenceStore();
   RGB rgb = PreferenceConverter.getColor(store,
PreferenceConstants.GROOVY_EDITOR_HIGHLIGHT_GJDK_COLOR);
   IToken keywordToken = new Token(new TextAttribute(new
Color(Display.getCurrent(), rgb)));

   WordBeginMatcher matcher = new WordBeginMatcher();
   addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_DOMAIN);
   addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_FRONT);

   JavaWordDetector wordDetector = new JavaWordDetector();
   CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
Token.UNDEFINED);
   combinedWordRule.addWordMatcher(matcher);

   List<IRule> rules = new ArrayList<IRule>();
   rules.add(combinedWordRule);

   return rules;
 }

 /**
  * WordBeginMatcher
  * Match word with the begining of another using underscore separator
  * @author Maxime HAMM
  */
 private static final class WordBeginMatcher extends WordMatcher {
   private final Map<CharacterBuffer, IToken> words = new
HashMap<CharacterBuffer, IToken>();

   @Override
   public void addWord(String word, IToken token) {
     this.words.put(new CharacterBuffer(word), token);
   }

   @Override
   public IToken evaluate(ICharacterScanner scanner, CharacterBuffer word)
{
     // look for underscore
     int i=0;
     CharacterBuffer buff = new CharacterBuffer(10);
     for (; i<word.length(); i++) {
       char c = word.charAt(i);
       if (c == '_') {
         i++;
         break;
       }
       buff.append(c);
     }
     if (i == word.length()) {
       return Token.UNDEFINED;
     }

     // check if it is a keyword
     IToken token = this.words.get(buff);
     if (token == null) {
       return Token.UNDEFINED;
     }

     // check if railer is digits only
     for (; i<word.length(); i++) {
       char c = word.charAt(i);
       if (!Character.isDigit(c)) {
         return Token.UNDEFINED;
       }
     }
     return token;
   }

   @Override
   public void clearWords() {
     this.words.clear();
   }
 }












---------------------------------------------------------------------
To unsubscribe from this list, please visit:

   http://xircles.codehaus.org/manage_email



Reply | Threaded
Open this post in threaded view
|

Re: GroovyTagScanner and IHighlightingExtender weakness

Andrew Eisenberg
Thanks for the pull request.  Will take a look.

On Tue, Jul 9, 2013 at 2:22 PM, Maxime HAMM <[hidden email]> wrote:

> Hi Andrew
>
> I was adding the "pull request"... It's the first time i'm using git and
> github... hope I did it correctly !
>
> I was also updating tests :
> - a new one to check that the rules are raised
> - I made a little fix for "SemanticHighlightingTests" because the tests was
> dependent one the "GROOVY_SEMANTIC_HIGHLIGHTING" preference value...
> - I also try to add a tests to check that the "initial rules" takes the
> priority on other groovy-native rules... but I had difficulty to manage
> it... may be you will do if this test is necessary ?
>
> Thanks a lot,
>
> kind regards,
> Maxime
>
> PS : I'm not able to
>
> Le 8 juil. 2013 à 22:32, Andrew Eisenberg <[hidden email]> a écrit :
>
> Hi Maxime,
>
> Thanks for the patch. I'm having trouble applying it.  Can you
> recreate this as a pull request?  This would have the added benefit of
> keeping yourself as the author of these changes (good for proper
> attribution).
>
> Both #1 and #2 sound reasonable.  I'll have a look at the pull request
> when it is available.
>
> On Mon, Jul 8, 2013 at 12:59 PM, Maxime HAMM <[hidden email]>
> wrote:
>
> Hello
>
> About my first question above :
>
> Here is a patch proposal for a "IHighlightingExtender2" : the new interface
> contains a new method "getInitialAdditionalRules" which return a list of
> rules... those rules are added in first position by the "GroovyTagScanner"
> It is working well for Jspresso and I think it could be useful for other
> extenders...
>
>
> About my second question :
>
> I've no idea about the best solution here... if you want and give me some
> instructions, I can try to work on it with you...
>
>
> Kind regards,
> Maxime
>
>
>
> Hello Andrew
>
> Few things I kept in my mind for month...
>
> 1) I think there is an issue about "additional rules" in the
> IHighlightingExtender extension :
>
>
> As far as I was testing... additional rules are never raised !
>
> Here is my test case (about JSPRESSO framework) :
> All word starting with string_ or text_ followed by digits have to be
> displayed as JDK keywords...
> (in JSPRESSO this is a shortcut to specify field's length)
>
> After debugging the GroovyTagScanner I understand why :
> There is a default token define for the "CombinedWordRule" used by groovy
> eclipse to store all
> keyword definitions... due to this default token, the scanner is never going
> further to check the
> additional rules !
>
>    @Override
>    protected List<IRule> createRules() {
>      ...
>      // combined rule for all keywords
>      JavaWordDetector wordDetector = new JavaWordDetector();
>      token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
>      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> token);
>
> I will suggest to replace this by :
>
>    @Override
>    protected List<IRule> createRules() {
>      ...
>      // combined rule for all keywords
>      JavaWordDetector wordDetector = new JavaWordDetector();
>      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> Token.UNDEFINED);
>
> Another alternative is to move the additional rules to the top of the create
> rules method :
>
>    @Override
>    protected List<IRule> createRules() {
>
>      // additional rules
>      if (additionalRules != null) {
>          rules.addAll(additionalRules);
>      }
>      ...
>
>      // combined rule for all keywords
>      JavaWordDetector wordDetector = new JavaWordDetector();
>      token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
>      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> token);
>
> A last alternative, which should cause less regressions, is to do both... I
> mean adding another
> method "getAdditionalTopRules" in the "IHighlightingExtender"...
>
> What is your opinion about this ?
>
>
> 2) It's not possible to have both... groovy native file using standard
> groovy color... and one or more groovy dsl
>   with customized colors !
>
> This was not an issue one year ago, but now JSPRESSO supports groovy for
> business rules !
> Developers are a little bit embarrassed to have to choose between one or the
> other...
>
> I will suggest you to make some extension point evolution to allow to
> parameterize extender with an extension file ?
>
> Something like :
>
> <extension
> point="org.codehaus.groovy.eclipse.ui.syntaxHighlightingExtension">
>  <highlightingExtender
>  extender="org.jspresso.contrib.sjsplugin.editor.SJSSyntaxHighlighting"
>  natureID="org.jspresso.contrib.sjsplugin.nature"
>                        fileExtension="sjs">
>  </highlightingExtender>
> </extension>
>
> Is it a good suggestion ?
>
>
> Thank you for you attention and help,
>
> Maxime
>
>
>
> ---------------------------------------------------------------
>
> Here is my scanner additional rule :
>
>
>  /**
>   * additional rules to manage string_* and text_*
>   * @author Maxime HAMM
>   */
>  @Override
>  public List<IRule> getAdditionalRules() {
>
>    IPreferenceStore store = GroovyPlugin.getDefault().getPreferenceStore();
>    RGB rgb = PreferenceConverter.getColor(store,
> PreferenceConstants.GROOVY_EDITOR_HIGHLIGHT_GJDK_COLOR);
>    IToken keywordToken = new Token(new TextAttribute(new
> Color(Display.getCurrent(), rgb)));
>
>    WordBeginMatcher matcher = new WordBeginMatcher();
>    addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_DOMAIN);
>    addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_FRONT);
>
>    JavaWordDetector wordDetector = new JavaWordDetector();
>    CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> Token.UNDEFINED);
>    combinedWordRule.addWordMatcher(matcher);
>
>    List<IRule> rules = new ArrayList<IRule>();
>    rules.add(combinedWordRule);
>
>    return rules;
>  }
>
>  /**
>   * WordBeginMatcher
>   * Match word with the begining of another using underscore separator
>   * @author Maxime HAMM
>   */
>  private static final class WordBeginMatcher extends WordMatcher {
>    private final Map<CharacterBuffer, IToken> words = new
> HashMap<CharacterBuffer, IToken>();
>
>    @Override
>    public void addWord(String word, IToken token) {
>      this.words.put(new CharacterBuffer(word), token);
>    }
>
>    @Override
>    public IToken evaluate(ICharacterScanner scanner, CharacterBuffer word)
> {
>      // look for underscore
>      int i=0;
>      CharacterBuffer buff = new CharacterBuffer(10);
>      for (; i<word.length(); i++) {
>        char c = word.charAt(i);
>        if (c == '_') {
>          i++;
>          break;
>        }
>        buff.append(c);
>      }
>      if (i == word.length()) {
>        return Token.UNDEFINED;
>      }
>
>      // check if it is a keyword
>      IToken token = this.words.get(buff);
>      if (token == null) {
>        return Token.UNDEFINED;
>      }
>
>      // check if railer is digits only
>      for (; i<word.length(); i++) {
>        char c = word.charAt(i);
>        if (!Character.isDigit(c)) {
>          return Token.UNDEFINED;
>        }
>      }
>      return token;
>    }
>
>    @Override
>    public void clearWords() {
>      this.words.clear();
>    }
>  }
>
>
>
>
>
>
>
>
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>    http://xircles.codehaus.org/manage_email
>
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: GroovyTagScanner and IHighlightingExtender weakness

Andrew Eisenberg
In reply to this post by Maxime HAMM
And congratulations on your first pull request!  It's never too late
to learn to use git. :-)

On Tue, Jul 9, 2013 at 2:22 PM, Maxime HAMM <[hidden email]> wrote:

> Hi Andrew
>
> I was adding the "pull request"... It's the first time i'm using git and
> github... hope I did it correctly !
>
> I was also updating tests :
> - a new one to check that the rules are raised
> - I made a little fix for "SemanticHighlightingTests" because the tests was
> dependent one the "GROOVY_SEMANTIC_HIGHLIGHTING" preference value...
> - I also try to add a tests to check that the "initial rules" takes the
> priority on other groovy-native rules... but I had difficulty to manage
> it... may be you will do if this test is necessary ?
>
> Thanks a lot,
>
> kind regards,
> Maxime
>
> PS : I'm not able to
>
> Le 8 juil. 2013 à 22:32, Andrew Eisenberg <[hidden email]> a écrit :
>
> Hi Maxime,
>
> Thanks for the patch. I'm having trouble applying it.  Can you
> recreate this as a pull request?  This would have the added benefit of
> keeping yourself as the author of these changes (good for proper
> attribution).
>
> Both #1 and #2 sound reasonable.  I'll have a look at the pull request
> when it is available.
>
> On Mon, Jul 8, 2013 at 12:59 PM, Maxime HAMM <[hidden email]>
> wrote:
>
> Hello
>
> About my first question above :
>
> Here is a patch proposal for a "IHighlightingExtender2" : the new interface
> contains a new method "getInitialAdditionalRules" which return a list of
> rules... those rules are added in first position by the "GroovyTagScanner"
> It is working well for Jspresso and I think it could be useful for other
> extenders...
>
>
> About my second question :
>
> I've no idea about the best solution here... if you want and give me some
> instructions, I can try to work on it with you...
>
>
> Kind regards,
> Maxime
>
>
>
> Hello Andrew
>
> Few things I kept in my mind for month...
>
> 1) I think there is an issue about "additional rules" in the
> IHighlightingExtender extension :
>
>
> As far as I was testing... additional rules are never raised !
>
> Here is my test case (about JSPRESSO framework) :
> All word starting with string_ or text_ followed by digits have to be
> displayed as JDK keywords...
> (in JSPRESSO this is a shortcut to specify field's length)
>
> After debugging the GroovyTagScanner I understand why :
> There is a default token define for the "CombinedWordRule" used by groovy
> eclipse to store all
> keyword definitions... due to this default token, the scanner is never going
> further to check the
> additional rules !
>
>    @Override
>    protected List<IRule> createRules() {
>      ...
>      // combined rule for all keywords
>      JavaWordDetector wordDetector = new JavaWordDetector();
>      token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
>      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> token);
>
> I will suggest to replace this by :
>
>    @Override
>    protected List<IRule> createRules() {
>      ...
>      // combined rule for all keywords
>      JavaWordDetector wordDetector = new JavaWordDetector();
>      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> Token.UNDEFINED);
>
> Another alternative is to move the additional rules to the top of the create
> rules method :
>
>    @Override
>    protected List<IRule> createRules() {
>
>      // additional rules
>      if (additionalRules != null) {
>          rules.addAll(additionalRules);
>      }
>      ...
>
>      // combined rule for all keywords
>      JavaWordDetector wordDetector = new JavaWordDetector();
>      token = getToken(PreferenceConstants.GROOVY_EDITOR_DEFAULT_COLOR);
>      CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> token);
>
> A last alternative, which should cause less regressions, is to do both... I
> mean adding another
> method "getAdditionalTopRules" in the "IHighlightingExtender"...
>
> What is your opinion about this ?
>
>
> 2) It's not possible to have both... groovy native file using standard
> groovy color... and one or more groovy dsl
>   with customized colors !
>
> This was not an issue one year ago, but now JSPRESSO supports groovy for
> business rules !
> Developers are a little bit embarrassed to have to choose between one or the
> other...
>
> I will suggest you to make some extension point evolution to allow to
> parameterize extender with an extension file ?
>
> Something like :
>
> <extension
> point="org.codehaus.groovy.eclipse.ui.syntaxHighlightingExtension">
>  <highlightingExtender
>  extender="org.jspresso.contrib.sjsplugin.editor.SJSSyntaxHighlighting"
>  natureID="org.jspresso.contrib.sjsplugin.nature"
>                        fileExtension="sjs">
>  </highlightingExtender>
> </extension>
>
> Is it a good suggestion ?
>
>
> Thank you for you attention and help,
>
> Maxime
>
>
>
> ---------------------------------------------------------------
>
> Here is my scanner additional rule :
>
>
>  /**
>   * additional rules to manage string_* and text_*
>   * @author Maxime HAMM
>   */
>  @Override
>  public List<IRule> getAdditionalRules() {
>
>    IPreferenceStore store = GroovyPlugin.getDefault().getPreferenceStore();
>    RGB rgb = PreferenceConverter.getColor(store,
> PreferenceConstants.GROOVY_EDITOR_HIGHLIGHT_GJDK_COLOR);
>    IToken keywordToken = new Token(new TextAttribute(new
> Color(Display.getCurrent(), rgb)));
>
>    WordBeginMatcher matcher = new WordBeginMatcher();
>    addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_DOMAIN);
>    addShortcutsKeywords(matcher, keywordToken, SJSMeta.KIND_FRONT);
>
>    JavaWordDetector wordDetector = new JavaWordDetector();
>    CombinedWordRule combinedWordRule = new CombinedWordRule(wordDetector,
> Token.UNDEFINED);
>    combinedWordRule.addWordMatcher(matcher);
>
>    List<IRule> rules = new ArrayList<IRule>();
>    rules.add(combinedWordRule);
>
>    return rules;
>  }
>
>  /**
>   * WordBeginMatcher
>   * Match word with the begining of another using underscore separator
>   * @author Maxime HAMM
>   */
>  private static final class WordBeginMatcher extends WordMatcher {
>    private final Map<CharacterBuffer, IToken> words = new
> HashMap<CharacterBuffer, IToken>();
>
>    @Override
>    public void addWord(String word, IToken token) {
>      this.words.put(new CharacterBuffer(word), token);
>    }
>
>    @Override
>    public IToken evaluate(ICharacterScanner scanner, CharacterBuffer word)
> {
>      // look for underscore
>      int i=0;
>      CharacterBuffer buff = new CharacterBuffer(10);
>      for (; i<word.length(); i++) {
>        char c = word.charAt(i);
>        if (c == '_') {
>          i++;
>          break;
>        }
>        buff.append(c);
>      }
>      if (i == word.length()) {
>        return Token.UNDEFINED;
>      }
>
>      // check if it is a keyword
>      IToken token = this.words.get(buff);
>      if (token == null) {
>        return Token.UNDEFINED;
>      }
>
>      // check if railer is digits only
>      for (; i<word.length(); i++) {
>        char c = word.charAt(i);
>        if (!Character.isDigit(c)) {
>          return Token.UNDEFINED;
>        }
>      }
>      return token;
>    }
>
>    @Override
>    public void clearWords() {
>      this.words.clear();
>    }
>  }
>
>
>
>
>
>
>
>
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>    http://xircles.codehaus.org/manage_email
>
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email