eclipse-platform
DefaultHelpUI.displayContext() overwrites IContext method parameter (regression from bug 533828)
Current Situation
The DefaultHelpUI has a regression from Bug 533828. While the displayContext(IContext, int, int, boolean) method allowed to show contents according to the IContext passed to the method, it is now only able to show contents according to the help attached to the currently focussed control, if available.
The following example project adds a toolbar button to the problems view, which is supposed to open context-sensitive help for that view.
However, the current functionality will always open the general help attached to the problems view:
The old implementation properly considers the passed context:
The example is a derivation of the one provided for concerns within comment 24 of the original bug report
Expected Behavior
The DefaultHelpUI is expected to consider the passed context like before the changes for bug 533828. Still, it should incorporate the fix for the addressed issue of bug 533828.
Additional Information
This is a copy of bug Bug 572804 that already documented the regression from Bug 533828. I repost it here because it's a pressing issue for us, which we will address soon.
The DefaultUI class simply overwrites the passed IContext in org.eclipse.help.ui.internal.DefaultHelpUI.displayContext(IContext, int, int, boolean) line 333.
This behaviour is a major change of the simple function of "simply displaying" the passed context.
This change was introduced by Bug ID 533828 and it does not seem to be right. Even though the way to get the desired context using the IContextProvider.getContext(Object target) may be correct it is not the task of this method to retrieve it and overwrite the original passed IContext. This change made it impossible to display custom build help contexts when there is a context provider present for the active part.
Further, the help context is only overwritten when displayed within the workbench. When the user sets the IHelpBaseConstants.P_KEY_WINDOW_INFOPOP in the preference store the original passed context is displayed in an extra window (Line 325). This seems to be inconsitent to overwrite the context in one case an d display the original context in another.
@jcompagner Since you have provided the original fix for bug 533828, I would like to contact you as soon as we make progress on this to ensure that the original bug remains fixed for you. In case you have an idea how deal with this regression since you have already worked on the issue, I would of course appreciate your input 🙂
yes fine, i can have a look (was on holiday)
its been a long time ago that i spend time on this, and the current help system works the way we want. (because i think wat did happen was that there was constantly a internmediate index page with one link and thats not what we want) as an example:
that is a dialog that we have that displays context help I think without my patch that wouldn't be displayed as is, so my patch was i think that if there is 1 entry we directly show that entry, so really it is context sensitive, that it would show an intermediate page with 1 url link where users needs to click on was very UX unfriendly
I am currently also looking again at something that i see in the help, we have external links (you can see that in the above picture) in our help (<a href=xxx target = blank) but those always opens for me in the old IE browser which doesn't work of course, no idea what that is ..
i quickly checked the code:
https://git.eclipse.org/c/platform/eclipse.platform.ua.git/commit/?id=92bbc9d49d2378e04ec24fe15840374b7c23dc3c
so what was there before and according to the comments which where also already there:
/*
- If the context help has no description text and exactly one
- topic, go straight to the topic and skip context help. */
Eclipse tried todo what we wanted, but always failed because the context was not correct for us.
So the patch was that it really needed to go to the active part (i guess here the dialog) and get the context from there if it had context and show really the context help of what the user now was seeing..
So why does that fail for you i wonder?
debugged it again and if i look at the original context that has 3 topics one of them for example being:
[href="/org.eclipse.platform.doc.user/concepts/concepts-4.htm", label="Perspectives"]
which is absolutely for us incorrect because the context help is an editor of us that has 1 topic... and should be shown directly as is. (so we want the help to show directly the actual help that our editor says it should have)
because our editors (and other views) have an implementation in getAdaper() of:
if (getContextId() != null && adapter.equals(IContextProvider.class))
{
return new ViewPartHelpContextProvider(getContextId());
}
so that returns the a pointer to the context help for that view/editor.
and we only want to show that directly not an intermediate page
So the patch was that it really needed to go to the active part (i guess here the dialog) and get the context from there if it had context and show really the context help of what the user now was seeing..
That exactly is the problem: if I understand the correctly, the changed code always takes the context according to the active part and the current focus control. If existing, this will always override the IContext given to the method, so no chance to display help according to the actual desired context anymore. You can see this in the provided example, where a specific help context is provided, which then is overriden by the generic help context of the text editor (which is the focus control in the active part). In our tool, we, for example, provide a specific context when clicking explicit help buttons, but they do not work, as they take the context from the focus control (which is not the button, as that cannot have focus).
Maybe I misunderstand what DefaultHelpUI.displayContext(...) is supposed to do. But my understanding is that it will consider the given IContext and not simply replace it by something else (under certain conditions).
And please don't get me wrong: I think I understand your original issue but the fix should not break other (valid) use cases. So I would like to find some solution that addresses both concerns (or understand why the broken use case is invalid, if that's the case).
but the thing is for us the context is always wrong.... the context we get from the system is not the context, its a way bigger context or something, not the actual context of the selected part where the user actually clicked on...
But for the current code to work the actual part also really need to return a context is that the problem? because would it be possible that it returns null? ( i find it a bit weird there is no null check there)
ah wait there is a null check that is the IContextProvider adapter = activePart.getAdapter(IContextProvider.class); looking for the adapter, if there is an adapter we do expect that that gives a result.
So in your case the activePart does give something? what part is that?
That sounds to me as if the context passed to the method should rather be a proper one than fixing the context inside the method?
In our examples (both the minimal one I posted for this issue and the actual one we have in our tool), the part has a context provider adapter and that one provides a help context for the focus control. In the example for this issue, the part is the problems view and that view has a context attached. But if some other, more specific context is passed to the displayContext(...) method (by clicking a button in that view that provides a more specific context), that one will be ignored.
Yes, the regression still exists and is still an issue for us that we can only solve by readding the old implementation to our product. I simply lost track of the issue as the workaround via using the old implementation makes the issue less severe for us. Still this is a bug/regression, which should be fixed. I will put the topic into my backlog again.