Community
Participate
Working Groups
I20060216-2000 The Help button should not show up in dialogs without help context ID. Examples in this build are the 'Source > Clean Up...' wizard or 'plugin context menu > PDE Tools > Organize Manifests...'.
This is not as easy as it seems. We would need to check the wizard control and recursively try all the children to check for help IDs. Some wizards can completely ignore static help ID and instead show help by adding the help listener to controls. We would prefer that ALL dialogs have help content. FOr years our context help was spotty - we want this to change. Help button just makes this more obvious.
...so the workaround to the bug is to add useful context help to all your dialogs :-)
I know about the "workaround" and I would also prefer that the world was full of up-to-date help text. However, I'm not sure all eclipse plugin providers will be able to spend the resources it needs to add help to every little dialog. IMO, its a bit a bold decision to force all downstream plugins to add help. Does this enforcement have PMC approval? Does that mean that every little developer writing a plugin for Eclipse now has to ship with full-fledged help just to not look stupid?
Products can turn off the "show help everywhere" flag if they desire, so we are not forcing it on downstream products - we just turned it on for the sdk.
But if it's enabled in the Eclipse SDK product, then the button still shows up in every dialog of every plugin I download from sourceforge and add to my install, right? That's not too small a burden on every participant in the Eclipse ecosystem. Note: I can accept that we want to try to become help-complete in the SDK, but I think this obligation should not be imposed on all third-party plugins as well.
I think I have a simple solution to this problem that would not require any breaking changes: Right now, the decision to show the help button is based on the following logic: // create help control if needed if (isHelpAvailable() || isDialogHelpAvailable()) { where 'isHelpAvailable()' is local variable for each dialog instance, and 'isDialogHelpAvailable()' is static and is currently turned on for the SDK. I suggest the following change: 1) Prime the initial value of the 'helpAvailable' field by calling 'isDialogHelpAvailable()' static method. 2) Change the logic to only check the local help flag: // create help control if needed if (isHelpAvailable()) { Scenarios: 1) Global flag is 'false'. By default, local flag will be false as well. Help button will NOT show unless 'setHelpAvailable(true)' is called by the dialog that extends TrayDialog. 2) Global flag is 'true'. Local flag is initially set to true. Help button WILL show unless 'setHelpAvailable(false)' is called by the dialog that extends TrayDialog. I think this change will give us the ballance we are looking for. For RCPs, nothing changes since the logic in 3.1. For SDK, the button will be shown by default (this is what we want) but clients can programmatically turn it off by calling 'setHelpAvailable(false)'. This is easier for us than trying to scan the client and try to predict if context help is provided (this is costly and cannot be 100% accurate).
Note also that 'every litle participant' should still strive to play by the rules of the product it tries to extend. Eclipse SDK sets the standard that every nontrivial dialog (i.e. preference page, wizard etc.) must provide meaningful context help. With the change I propose above, they have a choice of providing this help (the desired outcome), or calling 'setHelpAvailable(false)' as a declaration that they don't want to or don't have time to do so. A formal declaration like this is 100% accurate and has no performance penalties. A note (in case this was not obvious): 'setHelpAvailable(false)' must be called in the constructor i.e. before the contents is created.
I agree that we do not want to be crawling controls to determine if any of them have a help context - that would be really slow. I am not sure that I am crazy about having another field added to the dialog for this support. First off we should limit this to the TrayDialog as that is the only type where this is relevant. Also the tray may be used for more than help - perhaps we should think more in terms of "isTrayAvailable()" and not limit it to the help system (although that is how we will use it).
Tod, my proposal does not involve adding more fields or methods. It only changes the logic used to determine when to show the help button and how the EXISTING field is initialized.
My mistake - I was looking in Dialog not TrayDialog. It sounds fine to me then.
Markus, what's your take? Do you think that calling 'setHelpAvailable(false)' is reasonable for SDK plug-in authors that don't have time or resources to provide context help.
With the changes proposed in comment 6, clients would have the possibility to opt out of getting a help button. That's certainly better than what we have today. Still, plug-ins that have been written for < 3.2 M5 will get the button unless they release a new version. I'm still not convinced that showing a non-functional button is the best way to urge developers to provide context help. While it may be helpful in the development phase, I think the switch should be turned off in the final release. Dialog providers should explicitly opt in to have the button enabled.
The reason we want to do it is is because the availability of 'F1' (or Ctrl+F1 on Linus or 'Help' key on Mac) was not self-evident up to now. We had an accelerator for a function that was not available in a first-class way. Our feedback indicates that people cannot stop pressing the button :-) and that it made them avare of the wealth of information they have in the new help tray. Curtis, perhaps what we can do it attach a default help Id to the tray dialog so that even if the client itself does not have it, we will open the help tray with some generic blurb (similar to the workbench where it opens the 'catch all' help about workbench parts). At least this way users can switch to the search page and look for stuff, or browse help topics, and the button will not appear to be broken.
+1 for a generic blurb. And if the blurb was a wiki-style editable help page, users could add missing help contents and send us a patch for it. ;-)
Upon further investigation, I see that the current default behavior of traversing the widget tree to find the first help listener does not stop at the dialog shell but continues to the parent workbench shell. So in many cases where you have dialogs that are missing context help, it still shows up with the generic workbench help. The two examples in the bug description are actually happening for two other reasons: Organize Manifests... - This one does not show help because it is using a context help id that is not defined anywhere. This case can be handled in general by modifying the workbench help listener to show default help content if it can't find the context id definition. Clean up... - This one does not show help because WizardDialog always adds a help listener to the shell that delegates to the pages' performHelp(). However the default is to do nothing here. Unfortunately I can't just use the API to summon a default context help because that's in workbench, and we're in jface. I would need to add API for some sort of default help listener. I also can't change the default behavior in DialogPage#performHelp to continue traversing the widget tree because the javadoc says the default implementation does nothing. I can, however, override this in WizardPage to keep crawling up the tree. I don't think this change breaks any API contract... Dejan and Tod, do you have any concerns about this approach?
Other than it is the opposite of what Dejan suggested no <grin>. We should not be traversing anything - we will be in conflict with with SWT who already do that with the API for setting help listeners. I think the idea of the generic blurb is fine but the only changes we really want to make should involve whether or not we show the ? button - we should not try and fight the SWT behaviour. I suspect I am missing something here. Dejan or Curtis could you outline what you are thinking of doing now?
I guess I should clarify my solution is on how to get the generic blurb. Dejan suggested adding a context help id to TrayDialog but we can't do this because it's in jface, and contributing context help to a control is more than just putting an id.. you have to add a listener that does something concrete.. and there's nothing concrete we can do at the jface level. Also, we are already traversing in TrayDialog for the help button. See helpPressed(). This is necessary to emulate the OS's behavior of F1/Ctrl-F1/Help. We need to send a help event on something that has a listener, otherwise our event is ignored. Thus we traverse up the tree..
To further clarify :-), all discussion about crawling is IN ADDITION to my proposal. My proposal still stands and is for people who want to explicitly make the help button go away. This covers plug-ins currently developing on top of 3.2. 'Generic id' idea is just a twist to this to cover plug-ins that have already been developed and have been dropped in 3.2's plug-ins directory. They have no context help and didn't have a chance to call 'setHelpAvailable(false)'. For them we want to show something, anything really in order to avoid a 'broken' feel when you press the Help button and nothing happens.
My idea is still hazy but we can add a Shell listener to the Display in the workbench and attach a Help listener to all dialogs that extend TrayDialog.
Dejan why don't you try and hack something up so that we are clearer on what you are doing. It sounds like you are going to use the SWT API which is I think the right decision but it would help to see code so that we can understand it.
Created attachment 35145 [details] patch Here's a patch for you to look at. Note - not fully tested or complete yet.
Curtis, we could save one call to 'setHelpAvailable' in the TrayDialog constructor if you initialize 'helpAvailable' field directly in declaration: boolean helpAvailable = isDialogHelpAvailable(); It is OK to make this call because 'isDialogHelpAvailable()' is static.
After discussing with Dejan, I've come to the conclusion that we can't solve this for all cases without touching API. The case we can't solve is with multi-page dialogs like wizards and preferences. Please ignore my last patch, I will post a new one soon.
Created attachment 35151 [details] revised patch Tod, can you submit the jface and workbench parts of this patch? This patch fixes: - Context help id is set on widget but not defined anywhere (e.g. PDE Tools -> Organize Manifests) - Allows dialogs to explicitly turn off help button This patch does not fix: - Missing context help on multi-page dialogs (e.g. Source -> Clean Up...)
Tod, can you apply? (see previous comment)
The SelectionStatusDialog part of the patch is out of date so I can't apply it. One thing I am not sure about - in createButtonBar of TrayDialog if (isHelpAvailable() became if (isHelpAvailable() || isDialogHelpAvailable()) shouldn't this be an && - this condition is actually less restrictive than before
Created attachment 35685 [details] updated patch Here's the updated patch. Tod, the change actually goes in the opposite way, i.e.: if (isHelpAvailable() || isDialogHelpAvailable()) becomes if (isHelpAvailable() This way, you can set help to false for the dialog and it will not show the help button.
JFace and Workbench portions have been released for build >20050306
Benno, Markus, or Daniel, can you submit the search portion of this patch? I'm hoping one of you has access..
Darin, could you submit the debug.ui portion of this patch? It's a one-line change to react to the change in TrayDialog that allows you to turn off the help button.
(In reply to comment #29) > Benno, Markus, or Daniel, can you submit the search portion of this patch? I'm > hoping one of you has access.. Too bad ;-). Only Dani would have access, but he's not around. CCing Martin as the last available Search committer.
Applied debug portion.
released the search part of the patch > 20060307
All the pieces are in.. marking as fixed. Thanks everyone
