This commit really makes me sad. Copy/pasting code is not the way to do stuff. * We have shared bundles where implementation for such components can be put * we have class inheritance so a base class containing the common functionality with children for local and remote stuff are still an option * we have class names that are supposed to be meaningful (ProviderLaunchConfigurationDelegate extends ProfileLaunchConfigurationDelegate ???) * why on earth someone has to even copy constants class and redefine even the PLUGIN_ID ?
I do welcome the work on remote tools but not at the price of screwing the codebase. Please revert this commit and try to come with one that does things in a clean way. I would better see some functionality being delayed with one release than having to do the same fix in N places. Alexander Kurtakov Red Hat Eclipse team ----- Original Message ----- > From: "Otavio Pontes (Code Review)" <ger...@eclipse.org> > To: "Otavio Pontes" <obusa...@linux.vnet.ibm.com>, "Daniel Henrique Barboza" > <danie...@br.ibm.com> > Sent: Friday, December 14, 2012 2:55:58 PM > Subject: Change in linuxtools/org.eclipse.linuxtools[master]: First version > of remote unified profile launcher > framework. > > Otavio Pontes has submitted this change and it was merged. > > Change subject: First version of remote unified profile launcher > framework. > ...................................................................... > > > First version of remote unified profile launcher framework. > > For clarity, this patch contains only the common framework to > support the remote unified launchers. The remote unified > launchers will be added in separated patches. > > The reasons why I duplicated some of the classes were: > > - The remote unified launcher won't necessarily work just like > the local unified. Some launch options (like choosing which > remote framework to use, like RSE, Remotetools, git ...) can change > the way the remote unified work in the future. However I haven't > duplicated all classes - the remote framework shares a lot of > code with the local one. > > - Most of the APIs from the local unified framework are so tight > with the classes and return types used in the local launchers that > it would require a lot of code to make it work with the remote > launchers and it would be error prone. I really didn't want to take > the risk to add bugs in working code and, as I mentioned above, > the remote framework can diverge from the local one when more > remote options and support will be added in the future. > > Change-Id: Id934a4572477dc2cb5193ff144d92af843c94173 > Reviewed-on: https://git.eclipse.org/r/9239 > Tested-by: Hudson CI > Reviewed-by: Otavio Pontes <obusa...@linux.vnet.ibm.com> > IP-Clean: Otavio Pontes <obusa...@linux.vnet.ibm.com> > Tested-by: Otavio Pontes <obusa...@linux.vnet.ibm.com> > --- > M profiling/org.eclipse.linuxtools.profiling.launch/plugin.xml > M > profiling/org.eclipse.linuxtools.profiling.launch/schema/RemoteProxyManager.exsd > A > profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/AbstractProviderPreferencesPage.java > A > profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/AbstractProviderPropertyTab.java > A > profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/ProviderLaunchConfigurationTabGroup.java > A > profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/ProviderOptionsTab.java > A > profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/ProviderProfileConstants.java > A > profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/TimingPropertyTab.java > A > profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/launch/ProviderFramework.java > A > profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/launch/ProviderLaunchConfigurationDelegate.java > M > profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/profiling/launch/ProfileLaunchConfigurationTabGroup.java > 11 files changed, 1,592 insertions(+), 3 deletions(-) > > Approvals: > Otavio Pontes: Verified; Looks good to me, approved; IP review > completed > Hudson CI: Verified > > > -- > To view, visit https://git.eclipse.org/r/9239 > To unsubscribe, visit https://git.eclipse.org/r/settings > > Gerrit-MessageType: merged > Gerrit-Change-Id: Id934a4572477dc2cb5193ff144d92af843c94173 > Gerrit-PatchSet: 2 > Gerrit-Project: linuxtools/org.eclipse.linuxtools > Gerrit-Branch: master > Gerrit-Owner: Daniel Henrique Barboza <danie...@br.ibm.com> > Gerrit-Reviewer: Hudson CI > Gerrit-Reviewer: Otavio Pontes <obusa...@linux.vnet.ibm.com> > _______________________________________________ linuxtools-dev mailing list linuxtools-dev@eclipse.org https://dev.eclipse.org/mailman/listinfo/linuxtools-dev
