Issue 86 help link

[eobrienPilz]

This is a fix for #86

[mickaelistria]

The impact on API for this unique request is relatively high.
One can already override createHelpControl, so I don't think we should open the API more than it is already.

[eobrienPilz]

But FiltersConfigurationDialog is internal so overriding createHelpControl is not an option.

[mickaelistria]

FiltersConfigurationDialog is flagged as internal, but it's still exported and public.

[eobrienPilz]

I was trying to do it in a way that would not require warning suppression for internal API usage. I want to be sure we don't have to change our implementation again if the API is removed or changed.

Also, If FiltersConfigurationDialog is being specialized then MarkerContentGenerator needs to be made available through the MarkerSupportView as its required as an argument in the constructor. This will lead to another internal API usage warning.

The pattern I have seen for configuring this dialog has been to do it through extension points, so I was following that. Changing the pattern may lead to more issues that I haven't thought of.

[eobrienPilz]

@mickaelistria
I had a look at an alternative implementation with less API impact.

This is what I came up with.

• make ExtendedMarkersView.openFiltersDialog method protected, allowing an overridden FiltersConfigurationDialog be provided.
• make ExtendedMarkersView.getGenerator method protected. It's required as an argument to overridden FiltersConfigurationDialog constructor.
• make TrayDialog.helpPressed method protected allowing custom help pages to be launched.
• make TrayDialog.createHelpImageButton protected and add a style argument, allowing the style to be changed. SWT.CHECK style works for the eclipse help that appears at the side of the dialog, but SWT.PUSH is better for help that launches externally.

No changes to any extension points.

What do you think of the suggestion ? I can supply a patch or pr.

[mickaelistria]

I've looked into more details at the code and there is another possible thing that might work and that would remove some of the changes you mention:

• the button is "selected" only if TrayDialog.getTray() returns some non-null stuff. So I suspect if you override the openTray method so that it does nothing, then the helpButton would remain un-toggled; achieving a bit of the result without need to change API.
• When help is pressed, a SWT.Help even is sent. You can most likely add a custom listener to this Help event in order to perform the right action.

As to ExtendedMarkersView, I think instead of opening several methods as API, you may better instead create a new protected FiltersConfigurationDialog createFilterConfigurationDialog(MarkerContentGenerator generator) method that would be called by openFilterConfigurationDialog so that inside this method, you could more freely perform configuration and specialization of the dialog.

[eobrienPilz]

I've found something that works just by adding the new method createFilterConfigurationDialog you suggested.

Inside overridden FiltersConfigurationDialog
- constructor calls this.setHelpAvailable(true);
- createHelpControl is overridden and implements its own createHelpImageButton method with a handler.

So just a single API change and a little extra code in the overridden ExtendedMarkersView to create its own button + toolbar

[mickaelistria]

Great then, let's just add this method as new API, it's both simple and powerful enough.

[eobrienPilz]

Will I create a new pr or just update this one ?

[github-actions]

Test Results

   918 files  +    1     918 suites  +1   49m 54s ⏱️ + 11m 12s
 7 510 tests ±    0   7 359 ✅ +    2  151 💤 ±  0  0 ❌  - 2 
23 679 runs  +1 573  23 174 ✅ +1 456  505 💤 +119  0 ❌  - 2 

Results for commit 0c0da7e. ± Comparison against base commit 8f59241.

♻️ This comment has been updated with latest results.

[eobrienPilz]

@mickaelistria I updated the pr with the change we discussed. Also added a couple of tests that demonstrate how to use the new method to add a help link.

[mickaelistria]

Thank you!