Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#52 toggle occurrences toolbar button

closed
nobody
None
5
2006-06-19
2006-06-08
Jae Gangemi
No

i've expanded upon RFE:
http://sourceforge.net/tracker/index.php?func=detail&aid=1476317&group_id=75859&atid=545277
and added support for a toolbar button to toggle
marking occurrences (like in the java perspective).

i also fixed a couple minor bugs related to parsing
text to hightlight and installing/uninstalling the
handlers.

there is a patch.txt file to be applied, as well as a
new java file and icon contained in the
org.epic.perleditor directory structure.

Discussion

  • Jae Gangemi
    Jae Gangemi
    2006-06-09

    Logged In: YES
    user_id=11978

    i figured out how to have the icon automatically appear
    whenever a perl editor is selected, and have re-attached an
    updated zip file.

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-09

    patch file, java class, image

     
    Attachments
  • Jan Ploski
    Jan Ploski
    2006-06-11

    Logged In: YES
    user_id=86907

    I have trouble incorporating your patch because of the
    reformatting in OccurrencesUpdater. Can you split your patch
    for this file into two steps: 1. reformatting and 2. actual
    changes, so that I can see the actual delta?

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-12

    Logged In: YES
    user_id=11978

    sorry about that... i attached a diff w/ the formatting
    changes, and then the actual .java file itself b/c i don't
    think eclipse lets me create diffs against the local history.

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-12

    patch, java class

     
    Attachments
  • Jan Ploski
    Jan Ploski
    2006-06-13

    Logged In: YES
    user_id=86907

    Jae, is there any particular reason why the new
    ToggleMarkOccurrences action is not integrated with the
    editor just like all the other actions (i.e. subclass
    PerlEditorAction, update class PerlActionsIds,
    PerlEditorCommandIds etc.)?

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-13

    Logged In: YES
    user_id=11978

    i believe that toolbar items that should only appear when
    that type of editor is active (instead of being a "global"
    toolbar item) need to inherit from the TextEditorAction
    class instead of the generic Action class (btw: if you don't
    see the highligher icon appear automatically, you need to
    reset the eclipse perspective).

    i used the jdt version of toggle occurrences as an example
    when i implemented this, and more or less followed what they
    did.

     
  • Jan Ploski
    Jan Ploski
    2006-06-13

    Logged In: YES
    user_id=86907

    Making the visibility of your action dependent on the active
    editor would work ok within the current design. However, a
    closer inspection of the current EPIC code reveals some
    flaws in how the global action handlers are currently mapped
    to the editor-specific action instances. Specifically, the
    method PerlEditorAction.setActiveEditor is never invoked,
    nor is the default constructor (both of which came as
    surprises to me). So the immediate problem which surfaces
    during an attempt to incorporate the
    ToggleMarkOccurrencesAction into the current design is that
    it is given no opportunity to initialize its enabled/
    disabled status.

    I can also see how your action differs from all the other
    editor actions implemented so far. Unlike them, it has no
    ties to an editor instance. The visibility only depends on
    the editor type. Apart from that, the action simply
    manipulates a global preference, and all open editors must
    react to its change. It would not make much sense to keep a
    separate instance of ToggleMarkOccurrencesAction in each
    editor, while it seems quite natural for the other actions.
    BTW, for the same reason your method PerlEditor.
    isMarkOccurrence also appears out-of-place (apart from the
    somewhat unfortunate name).

    Anyway, I am going to look into your patch again after some
    rethinking and corrections to the actions code.

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-13

    Logged In: YES
    user_id=11978

    i don't see why the ToggleMarkOccurrences needs to
    initialize any statuses, it is just relies upon a preference
    value - so if the preference is enabled when the editor is
    openned, the icon is toggled on, otherwise, it is toggled
    off. as far as i can tell, all editors are responding if i
    toggle the preferences on/off via the toolbar button (i
    believe this is b/c i wired the button up in the "init"
    method, and not the "setActiveEditor" method.

    like i said before, i implemented this more or less in the
    same way that the jdt functionality is implemented (although
    there, they have even more logic contained in the editor
    which i think should be refactored out, but i digress). that
    code seems to keep a seperate instance of their toggle class
    per editor (unless the editor contributor is actually held
    as a singleton by eclipse, which might make sense b/c there
    is no wiring up in the constructor - at least in the java
    editor version of ToggleMarkOccurrences)

    these are the classes i modelled off of:

    org.eclipse.jdt.internal.ui.javaeditor.ToggleMarkOccurrencesAction
    org.eclipse.jdt.internal.ui.javaeditor.JavaEditor
    org.eclipse.jdt.internal.ui.javaeditor.BasicJavaEditorActionContributor

    in any case, it looks like i'm going to be writing a lot
    more perl code in the near future, so i'm interested in
    contributing more to the project.

     
  • Jan Ploski
    Jan Ploski
    2006-06-14

    Logged In: YES
    user_id=86907

    Jae, just to make clear: my remarks about design flaws and
    not being able to initialize refer to the current (old)
    design around PerlEditorAction, not to your (new)
    contribution.

    So what I am saying is that the old code needs corrections
    because it just does not work the way I imagined. I thought
    there would be one global PerlEditorAction instance as well
    as one PerlEditorAction per editor, with the former one
    delegating to the latter. However, the "global"
    PerlEditorAction is not instantiated at all.

    Again: your code works, the old code is suspect, and I
    should use this opportunity to iron out some wrinkles.

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-14

    Logged In: YES
    user_id=11978

    ahh - my bad - i misunderstood your last message, all good
    though. :)

    i'm still confused about what you mean reguarding the
    PerlEditorAction - where is this global reference that you
    mention? why wouldn't each editor have it's own instance of
    an editor action? i would think that there could be state
    held in the action classes, so you would need an instance
    per editor you have open.

    this is my second forarray into eclipse plugins, and i'm
    still trying to wrap my mind around how the apis, etc work.

    on a side note, are you the only one actively working on code?

     
  • Jan Ploski
    Jan Ploski
    2006-06-14

    Logged In: YES
    user_id=86907

    Here is what I found out so far:
    1. One action instance per editor (instantiated in
    createActions) is good for those actions that read/change
    state of a single editor. All current PerlEditorActions
    belong to this category.
    2. By this definition, your ToggleMarkOccurrencesAction is
    not a PerlEditorAction. Even if one opens 5 editors, there
    should still be just 1 instance of this action. Note that
    this action does not need to update itself when the active
    editor changes. The action does not keep editor-specific
    state. It only needs to initialize its 'checked' property
    once, before it becomes visible. This can happen when the
    action is created.
    3. PerlActionContributor is created by Eclipse once for the
    entire plug-in, not once for each editor. So it makes
    perfect sense to let it own an instance of
    ToggleMarkOccurrencesAction there (as you did).
    4. For each action specified in the plug-in manifest,
    Eclipse creates a RetargetAction instance behind the scenes.
    Each such RetargetAction listens to editor activations.
    Whenever an editor becomes active, PerlActionContributor is
    first given an opportunity to associate action IDs with
    action instances (kept in the activated editor, or
    elsewhere). PAC creates the associations by invoking bars.
    setGlobalActionHandler(id, action). In turn, the above
    mentioned RetargetActions bind themselves to the
    corresponding actions, which they retrieve by calling bars.
    getGlobalActionHandler(id).

    Regarding your extra question: yes, I think I am the only
    one working on EPIC code right now (also committing
    improvements programmed by my assistant - for example, the
    Mark Occurrences feature is hers).

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-14

    Logged In: YES
    user_id=11978

    so does that mean that epic is really doing the proper thing
    w/ reguard to the editor actions?

    i've hooked up the next/prev toolbar buttons (this was just
    a change to the plugin.xml - i removed the "styledText"
    attributes b/c i couldn't figure out what they were for, and
    the jdt config doesn't have them). just replace the
    extension point reference w/ the one attached.

    that should be all for enhancements to this feature, now on
    to something else.

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-14

    image, plugin.xml entry

     
    Attachments
  • Jae Gangemi
    Jae Gangemi
    2006-06-16

    Logged In: YES
    user_id=11978

    hrm - it seems i forgot to include the additional code that
    is part of the editor contributor class. i'll just hold off
    on that until the original patches have been comitted to
    cvs, and then i'll just submit another patch that hooks
    everything up to the navigation buttons in the toolbar.

    if you want that code sooner, let me know.

     
  • Jan Ploski
    Jan Ploski
    2006-06-16

    Logged In: YES
    user_id=86907

    I just committed your patch with some modifications
    (although without your latest next/prev-related additions).

    Here are a few programming style remarks from my review:
    1. Please avoid writing code which permits silent failures.
    When in doubt whether a reference can or cannot be null at
    some point, make a best effort to figure it out from the
    documentation and/or by analyzing the possible control and
    data flows. Use assertions to clarify your assumptions.
    Statements like "if (obj == null) return;" are generally a
    bad idea. It is preferable for bug activations to result in
    exceptions rather than in suppressed behaviour. If you know
    that the reference may be null for a legitimate reason,
    please comment why.
    2. Avoid filler "documentation" (i.e., generated comments,
    such as "Creates an XYZ object." for an XYZ constructor). In
    other words, don't repeat yourself.
    3. Please capitalize the first letter in javadocs. It seems
    that everyone else does.
    4. Please use third-person descriptions in javadocs ("Does
    this and that" rather than "Do this and that"). Both Java
    and Eclipse documentation use this convention.

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-17

    Logged In: YES
    user_id=11978

    cool - it appers you did not check in the OpenDeclaration or
    PerlDocAction classes b/c they still had the no-args
    constructor (at least locally for me).

    to respond to your past comments...

    1) noted - i think the eclipse ppl have some bad habits as
    well :)

    2) jalopy does that for me automatically, but i'll just
    create another profile for epic

    3) this one will be hard, but i'll try my best.

    4) i'll work on this one as well, but it'd be helpful if you
    pointed out some examples of this.

    is there some direction you would like me to attempt to move
    in next? i had thought about trying to work on the build
    path property page, but having looked at the jdt version of
    this, they have a lot of custom widgets working on that
    page, and it looks like a good deal of work to port that
    over (perhaps it could be done by relying on the jdt
    internal libs and asking the eclipse folks to refactor out
    that code for others).

    we can continue this through email, just contact me via the
    email listed here.

    i'll also create a patch for the next/prev functionality as
    well, it's pretty trival.

     
  • Jae Gangemi
    Jae Gangemi
    2006-06-19

    • status: open --> closed