This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
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.
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.
[TM01] Are there any ways to avoid new HelpCtx(<class>.getName()) since it will cause the same problem? I agree with Y02.
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).
(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).
Reassigning to JardaH.
(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.
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.
(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.
(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
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.
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
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.
> 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.
(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"); //... }
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.
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"); > //... > }
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"); } }
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.
(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.
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.