Bug 200499 - Validation of help IDs
Validation of help IDs
Status: NEW
Product: platform
Classification: Unclassified
Component: Help System
7.1
All All
: P3 (vote)
: TBD
Assigned To: Jaroslav Havlin
issues@platform
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-28 21:49 UTC by Jesse Glick
Modified: 2012-03-27 20:53 UTC (History)
5 users (show)

See Also:
Issue Type: TASK
:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2011-07-28 21:49:41 UTC
We should not be using HelpCtx.<init>(Class) in general. See bug #200459 for an example of the problems this can cause. Ought to be deprecated, preferably with an editor hint to replace it with a String constant.

Hypothetical alternatives all look like too much work:

1. Refactoring contributor which would update the map.xml. But how could this file be found? Help sets are typically not in implementation modules or easily discoverable from there (e.g. among dependencies).

2. Refactoring contributor which would replace the Class constant with a String constant during a class move/rename. Probably possible, but seems awkward.

3. Static validation at build time. Probably impossible for the same reason as #1.

4. Some kind of batch process which scans the whole build (sources or bytecode?) for calls to HelpCtx.<init>(Class) or HelpCtx.<init>(String). Warn on nonconstant params, and warn on params not corresponding to any known map ID.
Comment 1 Jaroslav Tulach 2011-08-05 13:05:05 UTC
Y01 Basically you suggest to deprecate a method, as its users are stupid, right? I don't think replacing all usages of the Class parameter method with String, will lead to anything good (except stability during refactoring). Rather I'd suggest to hook into refactoring and rename JavaHelp references, if they exist.

Y02 Anyway, the best would be to have a commit validation test to fail if we have help topic unintentionally unreferenced from the code.
Comment 2 Tomas Mysik 2011-08-05 13:16:39 UTC
[TM01] Are there any ways to avoid

new HelpCtx(<class>.getName())

since it will cause the same problem?

I agree with Y02.
Comment 3 Jesse Glick 2011-08-05 14:04:14 UTC
Y01 - see my suggestion #1; it will not work except in the unusual case that you happen to have the module with the helpset (e.g. usersguide) open at the same time.

Y02 - so most like my #4. (Adjusting issue metadata to leave the options open.) The difficulty is in identifying code which constructs a HelpCtx. Maybe can use ASM/BCEL?

TM01 - no, but I am not sure who would do that to begin with. Most code uses either (a) new HelpCtx("id.supplied.by.helpset.authors") or (b) new HelpCtx(ThisClassWhichIHopeIsSomedayMapped.class).
Comment 4 Tomas Mysik 2011-08-06 16:29:32 UTC
(In reply to comment #3)
> TM01 - no, but I am not sure who would do that to begin with.

Agree.

> Most code uses
> either (a) new HelpCtx("id.supplied.by.helpset.authors") or (b) new
> HelpCtx(ThisClassWhichIHopeIsSomedayMapped.class).

Yes, and the simplest way to avoid deprecation is changing

new HelpCtx(ThisClassWhichIHopeIsSomedayMapped.class)

to

new HelpCtx(ThisClassWhichIHopeIsSomedayMapped.class.getName()).

That's the reason why I asked (not everyone always reads Javadoc).
Comment 5 Antonin Nebuzelsky 2011-10-05 22:19:20 UTC
Reassigning to JardaH.
Comment 6 Jesse Glick 2012-03-08 13:26:09 UTC
(In reply to comment #4)
> the simplest way to avoid deprecation is changing
> 
> new HelpCtx(ThisClassWhichIHopeIsSomedayMapped.class)
> 
> to
> 
> new HelpCtx(ThisClassWhichIHopeIsSomedayMapped.class.getName()).

The latter can still be marked with a warning badge using Jackpot, I think.

To Y01 - stability during refactoring is one goal, and I think it is better than nothing, though I agree that a check for mismatched IDs would be good.
Comment 7 Jeffrey Rubinoff 2012-03-08 14:31:16 UTC
I've started a separate discussion on this among doc writers. Am sure Ken will have something to say when he's back from holiday.
One thought: would it be useful to validate our Map.jhm/Map.xml files when javahelp is built? Specifically to check them for nonexistent targets? That would catch that a refactoring had occurred so at least someone would know of the problem. Even if only us writers were informed, we could at least track down the change.
Also, is there some SOP for refactoring? Would it be too much trouble to add steps to search the refactored package for HelpCtx and inform the writers of the refactoring, if there is a HelpCtx? I don't know how often this comes up or how much of a nuisance it would be to you guys.
Comment 8 Jesse Glick 2012-03-08 14:54:49 UTC
(In reply to comment #7)
> would it be useful to validate our Map.jhm/Map.xml files when
> javahelp is built? Specifically to check them for nonexistent targets?

Definitely, that is my #4 and Y01. It is still TBD how to implement this.

> Would it be too much trouble to add
> steps to search the refactored package for HelpCtx and inform the writers of
> the refactoring, if there is a HelpCtx?

If the code is using only string constants for IDs, which we will try to guide developers towards doing, then code refactoring would have no effect on help sets.
Comment 9 Jesse Glick 2012-03-19 22:57:29 UTC
(In reply to comment #6)
>> new HelpCtx(ThisClassWhichIHopeIsSomedayMapped.class.getName()).
> 
> The latter can still be marked with a warning badge using Jackpot

core-main #d6166c5823c9
Comment 10 Jeffrey Rubinoff 2012-03-20 12:14:03 UTC
However there are a couple reasons we used class names in help IDs in the first place:
1. It automatically meets the uniqueness requirement for help IDs.
2. Sometimes it's useful for a writer to know where the class is that declares the help ID. If the help ID=the class name, it's trivial to find the class starting from only the mapping file.
3. It's consistent.

Also, it's possible to refactor code in a way that doesn't break help or presumably other links. Jaroslav Hardin did this recently when he refactored the Find and Replace code from utilities to api.search.



(In reply to comment #8)
> (In reply to comment #7)
> > would it be useful to validate our Map.jhm/Map.xml files when
> > javahelp is built? Specifically to check them for nonexistent targets?
> 
> Definitely, that is my #4 and Y01. It is still TBD how to implement this.
> 
> > Would it be too much trouble to add
> > steps to search the refactored package for HelpCtx and inform the writers of
> > the refactoring, if there is a HelpCtx?
> 
> If the code is using only string constants for IDs, which we will try to guide
> developers towards doing, then code refactoring would have no effect on help
> sets.
Comment 11 Jeffrey Rubinoff 2012-03-20 12:32:43 UTC
Lastly, *if we keep class name as part of help ID*, would this be the correct syntax?

ClassName.class.getName()       Default activated button

null                            Greyed out help button

ClassName.class.getName()     
     + "." + booleanVariable    Creates 2 ids for cases when panel class creates 2
                                different UIs, depending on a boolean value

ClassName.class.getName()      
     + "." + enumVariable       Creates multiple ids for cases when panel class
                                creates multiple UIs, depending on an enumerator
Comment 12 Jeffrey Rubinoff 2012-03-20 12:45:11 UTC
That would be Jaroslav *Havlin*, not Hardin. Think I was confusing him with a Western outlaw :(

(In reply to comment #10)
> However there are a couple reasons we used class names in help IDs in the first
> place:
> 1. It automatically meets the uniqueness requirement for help IDs.
> 2. Sometimes it's useful for a writer to know where the class is that declares
> the help ID. If the help ID=the class name, it's trivial to find the class
> starting from only the mapping file.
> 3. It's consistent.
> 
> Also, it's possible to refactor code in a way that doesn't break help or
> presumably other links. Jaroslav Hardin did this recently when he refactored
> the Find and Replace code from utilities to api.search.
> 
> 
> 
> (In reply to comment #8)
> > (In reply to comment #7)
> > > would it be useful to validate our Map.jhm/Map.xml files when
> > > javahelp is built? Specifically to check them for nonexistent targets?
> > 
> > Definitely, that is my #4 and Y01. It is still TBD how to implement this.
> > 
> > > Would it be too much trouble to add
> > > steps to search the refactored package for HelpCtx and inform the writers of
> > > the refactoring, if there is a HelpCtx?
> > 
> > If the code is using only string constants for IDs, which we will try to guide
> > developers towards doing, then code refactoring would have no effect on help
> > sets.
Comment 13 Jaroslav Havlin 2012-03-20 12:52:44 UTC
> Also, it's possible to refactor code in a way that doesn't break help or
> presumably other links. Jaroslav Hardin [:-)] did this recently when 
> he refactored the Find and Replace code from utilities to api.search.

I'm afraid that moving whole package is a very minor case.
Comment 14 Jaroslav Havlin 2012-03-20 13:16:57 UTC
(In reply to comment #11)
> ClassName.class.getName()
>      + "." + booleanVariable    Creates 2 ids for cases when panel class
>                                 creates 2 different UIs, depending on a boolean 
>                                 value
> 
> ClassName.class.getName()
>      + "." + enumVariable       Creates multiple ids for cases when panel class
>                                 creates multiple UIs, depending on an
>                                 enumerator

I guess that a switch statement would make the code more tools-friendly:

switch (enumVariable) {
  case EnumType.C1: return new HelpCtx("org.netbeans.something.case1");
  case EnumType.C2: return new HelpCtx("org.netbeans.something.case2");
  //...
}
Comment 15 Quality Engineering 2012-03-21 12:06:53 UTC
Integrated into 'main-golden', will be available in build *201203210400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/d6166c5823c9
User: Jesse Glick <jglick@netbeans.org>
Log: #200499: warn about nonconstant HelpCtx IDs.
Comment 16 Jeffrey Rubinoff 2012-03-22 14:14:26 UTC
To clarify, in a case like this:

final class PatternResourcesSetupPanel 
       extends AbstractPanel {
    private Pattern currentPattern =
           PatternResourcesSetupPanel.Pattern.CONTAINER;

    public enum Pattern {
           CONTAINER(ContainerItemSetupPanelVisual.class),
           STANDALONE(SingletonSetupPanelVisual.class),
           CLIENTCONTROLLED(ContainerItemSetupPanelVisual.class);

        
        ...
    }

    public void setCurrentPattern(Pattern pattern) {
        if (currentPattern != pattern) {
            currentPattern = pattern;
        }
    }

the switch would be 

switch(currentPattern) {
    case CONTAINER: return new HelpCtx 
        (PatternResourcesSetupPanel.class.getName()
             + ".CONTAINER");
    case STANDALONE: return new HelpCtx
        (PatternResourcesSetupPanel.class.getName()
             + ".STANDALONE");
    case CLIENTCONTROLLED: return new HelpCtx
        (PatternResourcesSetupPanel.class.getName()
             + ".CLIENTCONTROLLED");

Is that right?

(In reply to comment #14)
> (In reply to comment #11)
> > ClassName.class.getName()
> >      + "." + booleanVariable    Creates 2 ids for cases when panel class
> >                                 creates 2 different UIs, depending on a boolean 
> >                                 value
> > 
> > ClassName.class.getName()
> >      + "." + enumVariable       Creates multiple ids for cases when panel class
> >                                 creates multiple UIs, depending on an
> >                                 enumerator
> 
> I guess that a switch statement would make the code more tools-friendly:
> 
> switch (enumVariable) {
>   case EnumType.C1: return new HelpCtx("org.netbeans.something.case1");
>   case EnumType.C2: return new HelpCtx("org.netbeans.something.case2");
>   //...
> }
Comment 17 Jeffrey Rubinoff 2012-03-22 15:20:02 UTC
Playing around with the case I mentioned, I see that one case has to be declared default, so 

public HelpCtx getHelp() {
    switch (this.currentPattern) {
        default:
        case CONTAINER:
            return new HelpCtx(PatternResourcesSetupPanel.class.getName() + ".CONTAINER");
        case STANDALONE:
            return new HelpCtx(PatternResourcesSetupPanel.class.getName() + ".STANDALONE");
        case CLIENTCONTROLLED:
            return new HelpCtx(PatternResourcesSetupPanel.class.getName() + ".CLIENTCONTROLLED");
    }
}
Comment 18 Jaroslav Havlin 2012-03-22 15:32:17 UTC
To follow all recommendations, the switch statement could look like this:

switch (this.currentPattern) {
  case CONTAINER:
    return new HelpCtx("package.PatternResourcesSetupPanel.CONTAINER");
  case STANDALONE:
    return new HelpCtx("package.PatternResourcesSetupPanel.STANDALONE");
  case CLIENTCONTROLLED:
    return new HelpCtx("package.PatternResourcesSetupPanel.CLIENTCONTROLLED");
  default:
    return new HelpCtx("package.PatternResourcesSetupPanel.SOME_DEFAULT_HELP");
}

The "default" keyword should be the last item in the switch block, to handle 
cases not covered by previous "case" items.
Comment 19 Jesse Glick 2012-03-27 20:51:29 UTC
(In reply to comment #10)
> [new HelpCtx(Class)] automatically meets the uniqueness requirement
> for help IDs.

So does picking a unique ID that the documenters have put in the map.

> If the help ID=the class name, it's trivial to find the class
> starting from only the mapping file.

It is trivial anyway: just search (Ctrl-F), which will bring you to the exact usage too.

> it's possible to refactor code in a way that doesn't break help or
> presumably other links.

Of course it is possible, but the developer has to remember to do it.

(In reply to comment #18)
> To follow all recommendations, the switch statement could look like this

Right.
Comment 20 Jesse Glick 2012-03-27 20:53:36 UTC
BTW I think the summary can be adjusted. What is missing now is some way of finding calls to new HelpCtx("constant") which do not correspond to an ID in any known map.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo