Community
Participate
Working Groups
Implement an extension point for contributors to add to the repository properties wizard page UI. This new feature will facilitate adding configuration UI for properties that are not a part of the repository-specific plug-in, such as the task editor extension.
I'm okay with the current mechanism that allows additional contributions through the menu. Having a setting in the properties dialog would have the advantage of making all settings available in a single place though. David, do you have ideas how this could work UI wise? Would extensions be able to contribute additional expandable sections?
(In reply to comment #1) > I'm okay with the current mechanism that allows additional contributions through > the menu. Having a setting in the properties dialog would have the advantage of > making all settings available in a single place though. Users should be able to make all settings in one place. Having a menu though easier to implement makes for a confusing UI, since users will have to remember to go to one place for some settings and another place for other settings. > David, do you have ideas how this could work UI wise? Would extensions be able > to contribute additional expandable sections? We could take two tracks on this one: # an extension point for contributing expandable sections to the existing wizard page # an extension point for contributing new pages to the wizard I like the idea of having a single page for the properties dialog, so I prefer option 1. I've already made good headway in this direction, and I expect to have a patch ready sometime in the next couple of days. It's conceivable that option 2 would be desirable in the case of a large set of complex options being contributed, however I don't see a use case for that right now. Currently we only have the need to add a few simple settings. Let me know what you think!
Created attachment 110475 [details] a first draft of an extension point and implementation Attached a first draft of an extension point for contributions to the task repository settings page. The extension point is implemented for LocalRepositorySettingsPage only. The extension point is implemented to provide Task Editor Extension settings in the sandbox ui. With the patch you should be able to right-click the Local task repository in the Task Repositories view and see a Task Editor Extension section in the settings wizard. If the patch looks good, the java class AbstractRepositorySettingsPage could extend the new java class AbstractExtensibleRepositorySettingsPage, which would enable the extensibility for all repositories.
Created attachment 110476 [details] mylyn/context/zip
Created attachment 110514 [details] screenshot showing Bugzilla settings see the screenshot of the Bugzilla settings with the 'Task Editor Extension' section plugged in via the new extension point. Settings are expanded here, but collapsed when the dialog first opens.
Created attachment 110515 [details] screenshot of Local task repository settings local task repository settings with 'Task Editor Extension' section plugged in. section is collapsed by default when the editor opens.
Created attachment 110516 [details] patch evolved to include AbstractRepositorySettingsPage evolved the patch, now extension-based sections should appear for all repository pages.
Created attachment 110517 [details] mylyn/context/zip
This is looking good David. A couple of thoughts: * What is the reason for making AbstractRepositorySettingsPage extend AbstractExtensibleRepositorySettingsPage? I am not sure if there could be another use case for extending AbstractExtensibleRepositorySettingsPage so it would be easier to have all code in a single class. * Renaming validateSettings() to validateServerSettings() breaks binary compatibility. * All API additions should be tagged with an @since 3.1 comment. * ITaskRepositoryPageContribution seems very similar to IWizardPage except that it does not have access to the wizard and wizard container. Instead it uses a listener interface to communicate with the parent class. It makes the interface nice reusable by not coupling it to IWizard but on the other hand gives the contribution less flexibility, e.g. for manipulation the wizard message. Are there other examples in JFace or SWT where a parent registers a listener with a child to get notified to trigger validation? I have a preference to follow the design of the platform to make it as easy as possible for integrators to understand the API.
(In reply to comment #9) > This is looking good David. A couple of thoughts: Thanks for the feedback > * What is the reason for making AbstractRepositorySettingsPage extend > AbstractExtensibleRepositorySettingsPage? I am not sure if there could be > another use case for extending AbstractExtensibleRepositorySettingsPage so it > would be easier to have all code in a single class. The local repository settings page doesn't extend AbstractRepositorySettingsPage since it has much different behaviour. Keeping them seperate is a good separation of concerns, since AbstractRepositorySettingsPage makes lots of assumptions about what's on the page (such as there being a server with a URL) > * Renaming validateSettings() to validateServerSettings() breaks binary > compatibility. Agreed, that should be fixed in a new patch > * All API additions should be tagged with an @since 3.1 comment. Agreed, though when I tried to do that the PDE plug-in created an error saying that it should be @since 3.0. I knew that 3.0 was wrong, so I left the @since tags out until I could get some advice from you. > * ITaskRepositoryPageContribution seems very similar to IWizardPage except that I also saw that similarity and started out by using IWizardPage. I determined that IWizardPage was not a good fit for the following reasons: IWizardPage has lots of API that we don't want, for example: * performHelp * image descriptor * visibility * getNextPage/getPreviousPage * reference to wizard and is missing some API that we do want: * ability to participate in validation * ability to apply settings to a repository Ultimately in this case it makes sense to have a purpose-specific API, otherwise we'll carry some 'not-a-perfect-fit' baggage. > Are there other examples in JFace or SWT where a parent registers a listener with a > child to get notified to trigger validation? I haven't seen any, but on the other hand I haven't seen this kind of extensible UI either. > I have a preference to follow the > design of the platform to make it as easy as possible for integrators to > understand the API. I fully agree with this last statement. I also believe that fit-for-purpose APIs are better than trying to fit a square peg into a round hole. I'm open to any design ideas that you might have. This is a first stab at the problem, and I'm not married to the design. The listener mechanism (for example) is a way of the contribution letting the page know that it needs to be validated. If you have better/different ideas about how this might be done, please let me know.
> The local repository settings page doesn't extend AbstractRepositorySettingsPage > since it has much different behaviour. Keeping them seperate is a good > separation of concerns, since AbstractRepositorySettingsPage makes lots of > assumptions about what's on the page (such as there being a server with a URL) Agreed. I missed that the local page extends AbstractExtensibleRepositorySettingsPage which makes perfect sense. From the naming it sounds like it has more functionality than the existing class but that's what we will be stuck with to maintain backwards compatibility (maybe AbstractBaseRepositorySettingsPage would make that more clear). > > * All API additions should be tagged with an @since 3.1 comment. > > Agreed, though when I tried to do that the PDE plug-in created an error saying > that it should be @since 3.0. I knew that 3.0 was wrong, so I left the @since > tags out until I could get some advice from you. Interesting. I have had to setup an base line that contained all plug-ins of the Eclipse SDK and all Mylyn plug-ins to fix similar errors. > I also saw that similarity and started out by using IWizardPage. I determined > that IWizardPage was not a good fit for the following reasons: [...] > Ultimately in this case it makes sense to have a purpose-specific API, otherwise > we'll carry some 'not-a-perfect-fit' baggage. Very good points. I agree that we'll need a custom API in this case and the listener approach seems like a good way of coupling the parts. As long as we can encapsulate the boilerplate code in an abstract class this should work well for implementors.
(In reply to comment #11) Glad to see that we're on the same page so far. I'll iterate again on the patch to get rid of binary incompatibility issues. > Interesting. I have had to setup an base line that contained all plug-ins of the > Eclipse SDK and all Mylyn plug-ins to fix similar errors. Okay -- does this need to be a seperate installation or can it be the same as that used to develop?
> Okay -- does this need to be a seperate installation or can it be the same as > that used to develop? Not sure, if have always used a separate instance but I can't see a reason why it wouldn't work to point it at your Eclipse plug-ins directory.
Created attachment 110730 [details] re-cut patch with various changes changed as follows: * eliminated binary incompatibilities introduced by previous patch * added @since tags and other javadoc * moved AbstractExtensibleRepositorySettingsPage to public API * cleaned up UI contributions so that they appear above the 'Validate Settings' button * 'properties' now enabled on context menu of local repository
Created attachment 110731 [details] mylyn/context/zip
Looking for some feedback on this one.
Created attachment 112848 [details] re-cut patch against head, added 'Wiki Links' configuration item
Created attachment 112849 [details] screenshot showing extension (collapsed)
Created attachment 112850 [details] screenshot showing extension (expanded)
Some suggestions, mostly on making the names less implementation-centric: * "Task Editor Extension" -> "Comment Editor" * "None" -> "Plain Text" * What's the special "Bugzilla Dialect"? * "Wiki Links" is not self-explanatory, what is it?
(In reply to comment #20) > Some suggestions, mostly on making the names less implementation-centric: > * "Task Editor Extension" -> "Comment Editor" Sounds good... but it's not just for comments. (Description too) How about 'Text Editor'? > * "None" -> "Plain Text" Yes > * What's the special "Bugzilla Dialect"? This is related to bug 237120, bug 242939 and bug 245476 Basically it's a few enhancements to Textile to make it more optimal for use with Bugzilla > * "Wiki Links" is not self-explanatory, what is it? It's the 'wiki link pattern', that is the URL to the wiki associated with the issue tracking system. See bug 237678 for details Perhaps 'Wiki URL Pattern' would be better?
(In reply to comment #21) > (In reply to comment #20) > > * "Wiki Links" is not self-explanatory, what is it? > > It's the 'wiki link pattern', that is the URL to the wiki associated with the > issue tracking system. See bug 237678 for details > Perhaps 'Wiki URL Pattern' would be better? > I've had some thoughts on this one: do we actually have a use case where the wiki URL cannot be derived from the connector URL, or retrieved via the connector itself? Perhaps the UI for this configuration item should go away.
I think the original use case was to link bugs.eclipse.org with wiki.eclipse.org.
(In reply to comment #23) > I think the original use case was to link bugs.eclipse.org with > wiki.eclipse.org. Yes, however this is not useful if the markup language used does not support notation for 'internal' links. Furthermore, these links would only be visible within the Mylyn client, not the web application. I believe that the extra complexity to the end user outweighs the usefulness of this feature.
I think we are almost there :). Thoughts from reviewing the latest patch: AbstractExtensibleRepositorySettingsPage.findApplicableContributors() should check for an empty string rather than "*" to be more consistent with other Mylyn extension points. Why does ContributionComparator compare the hashCode() if titles of contributions are equal instead of returning 0? Instead of using IStatus[] I would prefer if methods only returned a single IStatus and a MultiStatus was used to aggregate status objects. Instead of adding ITaskRepositoryPageContribution and AbstractTaskRepositoryPageContribution to the API the abstract class should be sufficient (see https://www.blogger.com/comment.g?blogID=17547394&postID=6323554618318168270&pli=1 comment 5). The names of ITaskRepositoryPageContributor and AbstractTaskRepositoryPageContribution are very similar. At best I would like to avoid requiring a factory implementation at all and only keep the abstract class. How about adding an init(TaskRepository) method to AbstractTaskRepositoryPageContribution instead of passing the repository in the constructor? To allow contributions to programatically check if the contribution applies for that task repository it could return a boolean. Generally I would like to start moving these checks into xml that is expressed in the extensions to avoid loading classes for contributions if they are not needed similar to how enablement works for command contributions in the platform. I am still not entirely sold on the listener concept. Why not give AbstractTaskRepositoryPageContribution access to the ITaskRepositoryPage, e.g. through the init() method? That way contributions could call other methods on the wizard and the container, e.g. for running jobs or accessing the form toolkit held by the page.
(In reply to comment #25) > I think we are almost there :). Thoughts from reviewing the latest patch: Thanks for getting to this one, I'll iterate on it again. > > AbstractExtensibleRepositorySettingsPage.findApplicableContributors() should > check for an empty string rather than "*" to be more consistent with other Mylyn > extension points. Sounds good. > > Why does ContributionComparator compare the hashCode() if titles of > contributions are equal instead of returning 0? Contributions may not be the same even if their titles are. The code should have been comparing the contributions themselves when the strings are equal, not the titles. I'll fix it. > > Instead of using IStatus[] I would prefer if methods only returned a single > IStatus and a MultiStatus was used to aggregate status objects. Sounds good. > > Instead of adding ITaskRepositoryPageContribution and > AbstractTaskRepositoryPageContribution to the API the abstract class should be > sufficient (see > https://www.blogger.com/comment.g?blogID=17547394&postID=6323554618318168270&pli=1 > comment 5). No problem. As an asside, it seems odd to me that the 'Abstract' prefix is used for abstract classes that are API for which there is no interface. Usually the 'Abstract' prefix is used to indicate that it's an abstract default implementation of something else. In this case there is no 'something else'. People may find this confusing. What are your thoughts? > > The names of ITaskRepositoryPageContributor and > AbstractTaskRepositoryPageContribution are very similar. At best I would like to > avoid requiring a factory implementation at all and only keep the abstract > class. How about adding an init(TaskRepository) method to > AbstractTaskRepositoryPageContribution instead of passing the repository in the > constructor? To allow contributions to programatically check if the contribution > applies for that task repository it could return a boolean. The factory design pattern has advantages over the init with boolean idea: * factory design pattern is well-known * the Eclipse platform makes use of the factory design pattern for other similar cases (see IAdapterFactory, AbstractServiceFactory, ITabbedPropertySheetPageContributor, AbstractTaskEditorPageFactory, etc.) * it provides a nice sparation of concerns * it's simple > Generally I would > like to start moving these checks into xml that is expressed in the extensions > to avoid loading classes for contributions if they are not needed similar to how > enablement works for command contributions in the platform. That's a great idea! > I am still not entirely sold on the listener concept. Why not give > AbstractTaskRepositoryPageContribution access to the ITaskRepositoryPage, e.g. > through the init() method? That way contributions could call other methods on > the wizard and the container, e.g. for running jobs or accessing the form > toolkit held by the page. While giving the contribution access to the ITaskRepositoryPage may be a good idea for other reasons, the contribution itself needs to have a way to inform the task repository page that its state has changed. There is currently no API available to do this on ITaskRepositoryPage. Listeners are a way to solve this problem without changing the API on ITaskRepositoryPage. Do you have any other suggestions? Thanks again for your feedback, I'll iterate on this one again and create a new patch.
> No problem. As an asside, it seems odd to me that the 'Abstract' prefix is used > for abstract classes that are API for which there is no interface. Usually the > 'Abstract' prefix is used to indicate that it's an abstract default > implementation of something else. In this case there is no 'something else'. > People may find this confusing. What are your thoughts? I agree that the Abstract prefix is superfluous in this case and there are examples in the Eclipse SDK for omitting it (e.g. Job, WizardPage). Mylyn has been fairly consistent in using the prefix with few exceptions (e.g. TaskDataCollector, SubmitJob) but I am definitely in favor of more concise naming. Mik, what are your thoughts on this? > > The names of ITaskRepositoryPageContributor and > > AbstractTaskRepositoryPageContribution are very similar. At best I would like > to > > avoid requiring a factory implementation at all and only keep the abstract > > class. How about adding an init(TaskRepository) method to > > AbstractTaskRepositoryPageContribution instead of passing the repository in > the > > constructor? To allow contributions to programatically check if the > contribution > > applies for that task repository it could return a boolean. > > The factory design pattern has advantages over the init with boolean idea: > * factory design pattern is well-known > * the Eclipse platform makes use of the factory design pattern for other similar > cases (see IAdapterFactory, AbstractServiceFactory, > ITabbedPropertySheetPageContributor, AbstractTaskEditorPageFactory, etc.) > * it provides a nice sparation of concerns > * it's simple Good arguments. Let's have a quick chat about this on the next call. > While giving the contribution access to the ITaskRepositoryPage may be a good > idea for other reasons, the contribution itself needs to have a way to inform > the task repository page that its state has changed. There is currently no API > available to do this on ITaskRepositoryPage. Listeners are a way to solve this > problem without changing the API on ITaskRepositoryPage. Do you have any other > suggestions? I was thinking of something like this but didn't realize that getContainer() is protected: getPage().getContainer().updateButtons(); I just realized that validation is more complicated than I first thought. The settings page support two different ways of validating: User input validation (immediate) and connection validation (long running). Is there a use case thar requires extensions to hook into connection validation?
(In reply to comment #27) > Is there a use case that requires extensions to hook into connection validation? Not that I'm aware of.
Created attachment 115338 [details] reworked patch Steffen, Re-worked the patch as per your suggestions and Mik's naming issues. I think that I got them all. Notable changes: * it's against head after the taskEditorExtension was moved out of the sandbox, so two classes have been moved * naming follows Mik's suggestions * Uses abstract class instead of interface * removed Wiki Links configuration (we can add it back in later if necessary) * uses IStatus and MultiStatus instead of IStatus[] * re-enables default taskEditorExtension repositoryAssociations Please look it over and let me know what you think.
Created attachment 115339 [details] mylyn/context/zip
(In reply to comment #27) > > The factory design pattern has advantages over the init with boolean idea: > > * factory design pattern is well-known > > * the Eclipse platform makes use of the factory design pattern for other > similar > > cases (see IAdapterFactory, AbstractServiceFactory, > > ITabbedPropertySheetPageContributor, AbstractTaskEditorPageFactory, etc.) > > * it provides a nice sparation of concerns > > * it's simple > > Good arguments. Let's have a quick chat about this on the next call. > Steffen, now that we've had a discussion about this what is your position?
I'm fine with either solution. I do have a slight preference for creating instances through the extension point since it requires less API and a factory can still be added later.
(In reply to comment #32) > I'm fine with either solution. I do have a slight preference for creating > instances through the extension point since it requires less API and a factory > can still be added later. Sound good to me. I'll re-cut the patch. Is there anything else with this patch that I should address before it gets applied?
Created attachment 116062 [details] re-cut the patch removed contributor in favor of contribution Re-cut the patch with the following changes: * renamed taskRepositoryPageContributor to taskRepositoryPageContribution * removed ITaskRepositoryPageContributor * javadoc, exsd doc, minor fixes
Created attachment 116063 [details] mylyn/context/zip
As discussed validation should not run in the container, it should run in the UI thread. I'll iterate on this one again.
Created attachment 116387 [details] validation on UI thread Iterated again, this time with one minor change: validation now occurs on UI thread with no progress monitor.
(In reply to comment #37) > Iterated again, this time with one minor change: validation now occurs on UI > thread with no progress monitor. Steffen, this one should be ready for you.
I am getting close to committing the patch. I will only include the API bits and we can iterate on the other changes on bug 253115 and bug 235439. There will be minor changes so it's best to wait with changes until this patch has been committed.
Created attachment 116778 [details] updated patch
Thanks for your efforts and patience David. The API looks very good now and it's great how well documented it is! I have done minor renamings in the updated patch: AbstractExtensibleRepositorySettingsPage -> AbstractTaskRepositoryPage, addContributions() -> createContributionControls() and removed the coupling to FormToolkit. I noticed a few remaining nits in the implementation that should be addressed before the patch is applied: * The contribution extension point should require an id for each extension. This helps to diagnose problems in case instantiation of an extension fails. * All invocations of contributions need to be wrapped using SafeRunnable to prevent a contribution from causing the whole settings page to fail. * As far as I can tell validation of contributions is not invoked by AbstractRepositorySettingsPage. AbstractRepositorySettingsPage.isPageComplete() should probably call super? * AbstractRepositorySettingsPage.repository is never assigned a value. It should be trivial to add an assignment in the constructor which I would prefer over making the field in the super class protected. It would be great if there was a test case that checked if a (mock) contribution was actually added to he dialog. Since you have already done a lot on this patch I can do the remaining work but I probably won't be able to get to it soon. Let me know if you want to iterate on the implementation again.
(In reply to comment #41) > Thanks for your efforts and patience David. The API looks very good now and it's > great how well documented it is! No problem, and thanks! Documentation is key. > I noticed a few remaining nits in the implementation that should be addressed > before the patch is applied: > > * The contribution extension point should require an id for each extension. This > helps to diagnose problems in case instantiation of an extension fails. sounds good. > * All invocations of contributions need to be wrapped using SafeRunnable to > prevent a contribution from causing the whole settings page to fail. makes sense to me. Should that include the initialization methods or just validation? > * As far as I can tell validation of contributions is not invoked by > AbstractRepositorySettingsPage. AbstractRepositorySettingsPage.isPageComplete() > should probably call super? validation of contributions occurs when contributions fire the appropriate event. but yes, AbstractRepositorySettingsPage should call super. > * AbstractRepositorySettingsPage.repository is never assigned a value. It should > be trivial to add an assignment in the constructor which I would prefer over > making the field in the super class protected. sounds fine to me > It would be great if there was a test case that checked if a (mock) contribution > was actually added to he dialog. Yes, I agree. I don't really have the bandwidth to do that. > Since you have already done a lot on this patch I can do the remaining work but > I probably won't be able to get to it soon. Let me know if you want to iterate > on the implementation again. I'll iterate again.
Created attachment 116865 [details] steffen's patch with requested modifications Steffen, I've made the changes that you requested.
Created attachment 116890 [details] final patch
Thanks David. I have fixed two of the copyright headers and wrapped the contribution control construction in a safe runnable as well. Patch committed! Feel free to resolve if we are done here.
(In reply to comment #45) > Thanks David. I have fixed two of the copyright headers and wrapped the Thanks, for some reason my copyright headers are not being created from the project settings. > Patch committed! Feel free to resolve if we are done here. Done. Thanks for getting this one in. I'll try to get the other bits of the patch working on head and reopen this one if we've missed anything critical.
Repository property pages do not expand to the full size of the properties dialog anymore when the dialog is resized.
Created attachment 117734 [details] patch that resolves layout issue with dialog Try this patch: it solves the layout issue with the dialog being resized.
Thanks!
