Community
Participate
Working Groups
Provide extension points for UI connector definitions, this in order to allow custom wizards so the user can select the focus of a change set. i.e. a change set based compare wizards.
initial draft implementation under branch "versionsIntegration"
ScmUiException added
Alvaro, what is the purpose of ScmUiException? I would generally recommend to use CoreException. Please use your full name in @author tags.
CoreException should be fine as well although the application may have a hard time to distinguish the origin among CoreExceptions. I could really go either way but I would propose a quick vote so we consider the opinion from the current consumers :-). Kilian, Sebastien /Alvaro
Is there a programmatic way of recovering, for which the ScmUIException would be helpful or is it more like - ok something bad happened - just notify the user about it. In the later case using a CoreException should be my preferred way. I also noticed that in ScmUiConnnector, there is a swing ProgressMonitor used instead of a IProgressMonitor. Is that intentionally?
Nope, it does not offer a programatic way to recover, coreException has the addition of IStatus which might be helpful, the only possible inconvenience of using CoreException from different API's would be to distinguish the origin.. if this is not relevant to the receiving application we shall then use CoreException.
As for the ProgressMonitor, you are right it should be o.e.c.runtime.IProgressMonitor thanks Kilian
(In reply to comment #7) > As for the ProgressMonitor, you are right it should be > o.e.c.runtime.IProgressMonitor thanks Kilian I have pretty much the same comments i.e. if we treat CoreException and ScmUiException the same way, then there is no need for ScmUiException. /Sebas
Ok, I have replaced ScmUiException for CoreException, and it's committed in the repo. The Status needed in CoreException is nice since it includes the plugin id, which can be used as some sort of origin. ProgressMonitor was also replaced as IProgressMonitor the @author is also reflecting my full name thanks Steffen for bringing this up /Alvaro
Please change the class from an interface to an abstract class. It's intended to be extended by clients and we will likely want to add additional classes in the future. Also please add a sentence to the JavaDoc comment of the class and remove the empty @param tags or add content. The format of the copyright header should follow the standard format defined in the code template for the Versions project (see ScmUi for example). It should say "Copyright (c) 2011 Ericsson and others". Please list the copyright holder in the contributors section (I assume that it should be Ericsson and not you personally). We use the @author tags of classes to ensure credit is attributed to individuals.
> --- Comment #10 from Steffen Pingel <steffen.pingel@tasktop.com> 2011-03-31 20:27:51 EDT --- > Please change the class from an interface to an abstract class. It's intended > to be extended by clients and we will likely want to add additional classes in > the future. I can see advantages of using an interface since the implementation class can extend another class and still carry the type imposed by this extension.. Could you please extend on the advantages to use an Abstract class instead. > Also please add a sentence to the JavaDoc comment of the class and > remove the empty @param tags or add content. I saw an empty @return and information will be added > The format of the copyright header should follow the standard format defined in > the code template for the Versions project (see ScmUi for example). It should > say "Copyright (c) 2011 Ericsson and others". OK > Please list the copyright holder > in the contributors section (I assume that it should be Ericsson and not you > personally). We use the @author tags of classes to ensure credit is attributed > to individuals. Wow, this one affects all R4E classes, we have been using this format since my participation in LinuxTools project and continued on the new classes with R4E. I sure can do it for Mylyn Versions, and will need to do it later for R4E. Question: If there a place where we can give credit to multiple individual authors /contributors
(In reply to comment #11) > > --- Comment #10 from Steffen Pingel <steffen.pingel@tasktop.com> 2011-03-31 > 20:27:51 EDT --- > > Please change the class from an interface to an abstract class. It's intended > > to be extended by clients and we will likely want to add additional classes in > > the future. > > I can see advantages of using an interface since the implementation class can > extend another class and still carry the type imposed by this extension.. Could > you please extend on the advantages to use an Abstract class instead. Yes, that's a possible alternative. You would need to mark the interface as @noextend in this case and provide an abstract class. Since API clients shouldn't use SPI classes directly but go through an abstraction provided by the API package I see don't see any advantage in having the extra interface in this case? > > Please list the copyright holder > > in the contributors section (I assume that it should be Ericsson and not you > > personally). We use the @author tags of classes to ensure credit is attributed > > to individuals. > > Wow, this one affects all R4E classes, we have been using this format since my > participation in LinuxTools project and continued on the new classes with R4E. > I sure can do it for Mylyn Versions, and will need to do it later for R4E. I am not concerned about other projects but we should use a consistent format for the versions project. > Question: If there a place where we can give credit to multiple individual > authors /contributors You can add multiple entries in the copyright header and the class header. See org.eclipse.mylyn.internal.tasks.core.TaskRepositoryManager for an example.
> --- Comment #12 from Steffen Pingel <steffen.pingel@tasktop.com> 2011-04-01 12:31:36 EDT --- > Yes, that's a possible alternative. You would need to mark the interface as > @noextend in this case and provide an abstract class. Since API clients > shouldn't use SPI classes directly but go through an abstraction provided by > the API package I see don't see any advantage in having the extra interface in > this case? > It's still not clear to me, The methods in the interface are all abstract.. and the plugin does not have an api package but spi. So the clients would either access an abstract class or an interface (with abstract methods) from the spi package. What triggers the need to mark the interfaces in an spi package as @noextend (?) > > I am not concerned about other projects but we should use a consistent format > for the versions project. > OK > > You can add multiple entries in the copyright header and the class header. See > org.eclipse.mylyn.internal.tasks.core.TaskRepositoryManager for an example. > Thanks it's nice to see a visual example.
(In reply to comment #13) > It's still not clear to me, > The methods in the interface are all abstract.. and the plugin does not have an > api package but spi. So the clients would either access an abstract class or an > interface (with abstract methods) from the spi package. What triggers the need > to mark the interfaces in an spi package as @noextend (?) In terms of maintaining binary compatibility the same rules that apply to api packages also apply to spi packages. The only difference between SPI and API is really who consumes and extends it. I highly recommend this document which has detailed information how to evolve Java based APIs: http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods . In this particular case ScmUiConnector is intended to be implemented by clients (connectors). If we ever wanted to add a another method we would either need to bump the major version number of the bundle or create an ScmUiConnector2 interface. This is easily avoidable by using an abstract class instead of an interface. We have successfully used that pattern in Mylyn 3.0 and generally only exposed interfaces that were not intended to be implemented by clients. These interfaces provide an API abstraction over internal classes (e.g. ITask) to hide implementation detail and methods only intended for framework consumption. The Eclipse API tooling uses @noimplement (sorry, forgot to mention that) and @noextend JavaDoc annotations to indicate that.
Alvaro, it's important that we get this right before the API freeze. Have you considered comment#14?
(In reply to comment #15) > Alvaro, it's important that we get this right before the API freeze. Have you > considered comment#14? The change is now implemented and committed, thanks for the clarification it's important to realize that all implementations of an interface will break as soon as you add a new method but not necessarily while extending and abstract class..
Thanks! Are we done here?
No more comments pending, Marked as fixed.
Sorry, I just got around to taking another look at the API and noticed a few things: The method signature includes a progress monitor and throws a CoreException. This generally indicates that the method is expected to be invoked from a background operation (and not the the UI thread). This is contrary to the description of the method which indicates that it may open a dialog which requires it to be invoked on the UI thread. Also, the signature does not include details such as a parent shell are necessary for proper parenting of dialogs. On a higher level I have a difficult time seeing how clients would use that method. The API is very abstract and the description very broad leaving a lot of room in the implementation and few constrains in term of what the expected result is. Alvaro, please take a look at AbstractRepositoryConnectorUi for an examplatory connector UI API. My sense is that it would be more useful to clients if a wizard page was returned that allowed a user to select a change set. This page could be embedded by clients in various scenarios. Alternatively, a method in the connector core that returns a list of change sets and a generic dialog that supports selecting from that list would provide the same generic functionality but have potiential for reuse in other scenarios as well.
(In reply to comment #19) > Sorry, I just got around to taking another look at the API and noticed a few > things: > > The method signature includes a progress monitor and throws a CoreException. > This generally indicates that the method is expected to be invoked from a > background operation (and not the the UI thread). This is contrary to the > description of the method which indicates that it may open a dialog which > requires it to be invoked on the UI thread. Also, the signature does not > include details such as a parent shell are necessary for proper parenting of > dialogs. > > On a higher level I have a difficult time seeing how clients would use that > method. The API is very abstract and the description very broad leaving a lot > of room in the implementation and few constrains in term of what the expected > result is. Alvaro, please take a look at AbstractRepositoryConnectorUi for an > examplatory connector UI API. > > My sense is that it would be more useful to clients if a wizard page was > returned that allowed a user to select a change set. This page could be > embedded by clients in various scenarios. > A wizard is what we had to implement for an internal Clear Case connector, however for Git, we implemented a dialogue as you can see on the o.e.mylyn.git.ui, my impression is that similar dialogues would be needed for cvs, svn. > Alternatively, a method in the connector core that returns a list of change > sets and a generic dialog that supports selecting from that list would provide > the same generic functionality but have potiential for reuse in other scenarios > as well. Yes, that was an alternative which I think was discussed in the mailing list before creating the UI connector interface. however separating core and ui seems like a good thing, although I agree that the UI interface does not offer a UI structure like a shell or wizard and it's in fact not a background job but it's expected to spawn a modal dialogue/wizard. So we could e.g. remove the monitor and keep the exception as it is needed to report the reason for unsuccessful return of change sets. Calling connector dialogues and wizards from a UI connector make sense to me, would you rather move them to the core connector ?
(In reply to comment #20) > > My sense is that it would be more useful to clients if a wizard page was > > returned that allowed a user to select a change set. This page could be > > embedded by clients in various scenarios. > > > A wizard is what we had to implement for an internal Clear Case connector, > however for Git, we implemented a dialogue as you can see on the > o.e.mylyn.git.ui, my impression is that similar dialogues would be needed for > cvs, svn. Sounds like a wizard or a wizard page would capture the requirements and provide a more specific abstraction than the current API? Note that the current implementation in GetChangeSetDialog invokes background operations on the UI thread (using NullProgressMonitor). This is *never* a good idea. Wizards have a nice API for running background operations and show progress embedded in the UI which has worked well in the past and is used in many places throughout the tasks framework. > > Alternatively, a method in the connector core that returns a list of change > > sets and a generic dialog that supports selecting from that list would provide > > the same generic functionality but have potiential for reuse in other > scenarios > > as well. > > Yes, that was an alternative which I think was discussed in the mailing list > before creating the UI connector interface. however separating core and ui seems > like a good thing, although I agree that the UI interface does not offer a UI > structure like a shell or wizard and it's in fact not a background job but it's > expected to spawn a modal dialogue/wizard. You can also do both, provide an API for UI component and make the core functionality available in the core. > So we could e.g. remove the monitor and keep the exception as it is needed to > report the reason for unsuccessful return of change sets. What is the purpose of the exception? Since the API provides a UI component it should also provide proper error handling, e.g. through the title area of a wizard page. A client invoking the API has no good way of dealing with the exception. > Calling connector dialogues and wizards from a UI connector make sense to me, > would you rather move them to the core connector ? The UI component should be accessible from the UI but if a portion of the UI implementation can be moved to the core that makes sense to me. Looking at the implementation it's already done that way: The dialog invokes connector.getChangeSet() so I don't think that needs to change.
Created attachment 197054 [details] suggested change Alvaro, do you have objections to applying this change?
(In reply to comment #22) > Created attachment 197054 [details] > suggested change > > Alvaro, do you have objections to applying this change? I am ok with removing the progress monitor, but I am not comfortable replacing the checked CoreException with the unchecked RunTimeException. For the the Git UI connector we use it only in one place where it seems quite unlikely to happen. However for the internal CC connector, the exception is triggered in several places i.e. argument validation, resolution of core connector, and consistency between project and resolved connector. while the cases are unexpected, we shall keep the application from crashing with no chance to implement notifications and recovery mechanisms. Other types of errors/exceptions shall then be handled internally by the UI and not escalated back to the application. What are the drawbacks of having a checked exception in the api ?
(In reply to comment #23) > I am ok with removing the progress monitor, but I am not comfortable replacing > the checked CoreException with the unchecked RunTimeException. > For the the Git UI connector we use it only in one place where it seems quite > unlikely to happen. However for the internal CC connector, the exception is > triggered in several places i.e. argument validation, resolution of core > connector, and consistency between project and resolved connector. If argument validation fails it usually indicates a programming error that is not recoverable and should be indicated through a RuntimeException. Failure to resolve the connector core indicates a provisioning problem that should be handled on a different level. If you are able to access the UI connector it should be safe to assume that the Core connector is also available otherwise you end up duplicating error handling code in every single operation. Regarding consistency between project and resolved connector this sounds like input validation to me. In this case error handling depends on how you specify the API contract: If it's invalid to pass a resource that is not managed in Git this should trigger an IllegalArgumentException. If however it is supported to pass any resource a defined value should be returned, e.g. null, indicating that the connector can not provide change sets for the given resource. > while the cases are unexpected, we shall keep the application from crashing > with no chance to implement notifications and recovery mechanisms. Agreed. I don't see how a user could recover from a CoreException in this case though. > Other types of errors/exceptions shall then be handled internally by the UI > and not escalated back to the application. > What are the drawbacks of having a checked exception in the api ? I still don't understand how a client would handle a CoreException in a good way? Looking at the R4E code the CoreException is simply logged and the command silently fails. This is no different than relying on the platform exception handling which will catch RuntimeExcpetions and handle it in the same way. When forcing exception handling through a checked exception you basically end up with more code but the same user experience.
(In reply to comment #24) Ok, It's fine to replace it to RunTimeException. I will go ahead and commit the changes. Steffen, shall we need a manual trigger of the mylyn-release build to pick up the changes from the the updated mylyn versions ?
(In reply to comment #25) > (In reply to comment #24) > Ok, It's fine to replace it to RunTimeException. I will go ahead and commit the > changes. Great! Thanks. > Steffen, shall we need a manual trigger of the mylyn-release build to pick up > the changes from the the updated mylyn versions ? Yes, I can do another build as soon as the changes are in head.
(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > Ok, It's fine to replace it to RunTimeException. I will go ahead and commit the > > changes. > > Great! Thanks. > > > Steffen, shall we need a manual trigger of the mylyn-release build to pick up > > the changes from the the updated mylyn versions ? > > Yes, I can do another build as soon as the changes are in head. Changes are in head now, for versions and reviews.
Thanks for committing the changes!
