Community
Participate
Working Groups
The Core class LaunchBarManager is in heavy need of cleanup and separation. It is very difficult readability-wise, and making meaningful contributions is hard. The interface ILaunchBarManager is also fairly sparse. The UI classes all use the implementation of LaunchBarManager rather than the interface, because the interface is severely lacking. The inner interface "Listener" is poorly named and deserves its own class. The launch configuration listener should be separated out into its own class to handle the logic involved for additions or removals of launch configurations. There could also be a 'model' class holding the backing information / maps / collections / etc, while the LaunchBarManager should be in charge of 'doing' stuff... loading and storing preferences, responding to API requests, handling listeners. UI should be able to accomplish 100% of its goals through the interface. The use of Pair<String, String> in two different contexts is a bit confusing as well. There's also a need for more comments / documentation where possible. Luckily, I have a patch ;) 90% of the upcoming gerrit request is simply moving / re-organizing code, adding interface methods, and providing separation of concerns. There's also a lot more utility methods with clear names, rather than pulling from maps and lists directly. I may have also changed a private internal variable's name or two for clarity, and added two private inner types to replace the dual usage of Pair<String, String>. There are few to no implementation changes that I can see, and all unit tests appear to pass locally. My local smoketests of my own enhancements also pass. The thought had occurred to me that you may have only wanted the actual launchbar UI to use the setters (set active mode / launch / etc) so I made a second interface which is internal and shared with friend plugins (such as launchbar.ui). I think the most important change here is the enhancement of the interfaces, which make the code much more usable, and the separation out, which makes it more quickly grokkable ;)
New Gerrit change created: https://git.eclipse.org/r/47562 WARNING: this patchset contains 1054 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
The current architecture is on purpose. The UI is tightly coupled to the core. The UI is actually the main component. It is the LaunchBar, a UI component. The core is there to serve non UI aspects of the LaunchBar and provide a minimalistic interface to the rest of the world. I really want to keep the API small and really think hard before adding to it and creating complexity we probably don't need. If there's APIs you need, let me know. LaunchBar Manager is getting big, but a simple restructuring might help with that. I think there's some complexity there we could clean up.
> LaunchBar Manager is getting big, but a simple restructuring might help with that. I think there's some complexity there we could clean up. Honestly, I think Core, aside from LaunchBarManager, is pretty clean and well-ordered. I may disagree with some of the API, but I think my patch on this issue accomplishes the small restructure needed to make things more clear. I can add more comments, I guess ;) > I really want to keep the API small and really think hard before adding to it and creating complexity we probably don't need. Most of my additions to the ILaunchManager interface were simply making existing methods into API, but really I'm not married to that. I could just as easily move most of those methods to the internal interface for launchbar.ui to use (IAuthorizedLaunchBarManager) which has access to the setters etc. Again, if we go that route, then this patch is more just cleanup of LaunchBarManager and it's all internal changes. If we decide some of these methods actually do belong in the primary interface, I don't think it's a horrible extension or complication of the API. I mean, just look at the methods that I've added, and you'll see they're basic getters: // Descriptors public ILaunchDescriptor getActiveLaunchDescriptor(); public ILaunchDescriptor[] getLaunchDescriptors(); // Modes public ILaunchMode getActiveLaunchMode(); public ILaunchMode[] getLaunchModes() throws CoreException; // Targets public IRemoteConnection getActiveLaunchTarget(); public List<IRemoteConnection> getLaunchTargets(ILaunchDescriptor descriptor) throws CoreException; // Launch configs public ILaunchConfiguration getActiveLaunchConfiguration() throws CoreException; public ILaunchConfiguration getLaunchConfiguration(ILaunchDescriptor descriptor, IRemoteConnection target) throws CoreException; public ILaunchConfigurationType getLaunchConfigurationType(ILaunchDescriptor descriptor, IRemoteConnection target) throws CoreException; public ILaunchDescriptorType getDefaultDescriptorType(); // Listeners public void addListener(ILaunchBarManagerListener listener); public void removeListener(ILaunchBarManagerListener listener); public String getDescriptorTypeId(ILaunchDescriptorType type); These methods, I feel, don't add extra complexity at all. They're all methods that were there already. All the other changes were small: clean up UI to use the new interface for setters, fix unit tests, separate out the launch listener (a purely internal detail), etc. If you'd like, I can submit a patch with all the above interface methods added to the private internal interface rather than the public API one... but this patch is predominantly a code-cleanup patch.
