Community
Participate
Working Groups
Following common convention in Eclipse development and to promote flexible extension, UI code should be refactored out of editor.core plugin. But this induces API changes so it need to be reviewed by the community.
Due to the compexity and backward compatibility considerations, moved this to Galileo to re-evaluate based on the resource.
Also, before starting the implementation if we decide to fix it, we need to provide a design doc posted to the dtp community for feedback/agreement. The spec needs to describe the detailed design and any possbile backward compatibility impacts and how we'd solve those. if not solvable by the refactored framework for a certain impacts, What would be the code migration path & step by step instructions.
Created attachment 126105 [details] Patch except org.eclipse.datatools.sqltools.tablewizard Because of CVS error, fail to retrieve org.eclipse.datatools.sqltools.tablewizard content. I will upload another patch for org.eclipse.datatools.sqltools.tablewizard later.
Created attachment 126106 [details] seperated ui plugins seperated ui plugins
Lin, we need patches, not plug-ins to review. Even with a change of this magnitude, we're going to have to know what's changed in the existing plug-in as well as whatever new plug-ins are created to spin-off the UI bits.
Hi Brian, Larry, and Linda... The folks working on this patch need your input on this one. The changes include some plugins initially contributed by IBM and they would like you guys to verify that things look copacetic. The problem is that the plug-ins are not in patch form, they're zipped up. So reviewing them is going to be tough. --Fitz
Hi Brian, I do not have the permission to share the sqltools projects to DTP CVS, so I can not generate patch for these new xxx.ui plug-ins. May I deliver this patch since there is no comment follow up?
Unfortunately we don't have any feedback yet from Brian Payton on these plug-ins, so we still need to get some kind of approval or disapproval from IBM before we can proceed. I'll ask again today at the Team Lead meeting.
Hi, I'm taking a look at it now. Sorry about the delay!
Hi, the patch (patch.txt) gets a lot of mismatch errors on various plugins when I try to apply it to the latest versions from HEAD. Was it generated from older versions of the plugins? Please re-create the patch using the latest plugins, or verify that we won't lose any of the recent plugin changes once you check in the restructured code.
Brian, the patch was prepared about a month ago and we've been waiting for the review since. We'll prepare new patch based on the current codeline.
Thanks. Reviewing was kind of hard given all the errors.
Created attachment 130784 [details] Sqltools ui/non-ui refactor patch on April 02
Created attachment 130786 [details] Sqltools ui plug-ins on April 2
Brian, any review comments? Please let us know your feedback asap, this checkin is blocking other feature checkins. Thanks.
Hi, I'll give you comments a few at a time, as I continue going through the code. Overall, nice job. In the long run it's good to have the UI stuff separated out like this. Since we're in M7, it would be good if we could make it possible for clients to pick up the change by simply updating their MANIFEST.MF files to include the new ui plugins and doing an "organize imports" to pick up the changed packages. I know that might always be possible, but it would make the transition to the new architecture smoother. I see you've changed some of the constant names, for example; if you can leave the old constant name (but mark it as deprecated), clients won't be forced to go through their code making fixes. Here are some comments on particular plugins. I'll focus on the "client" plugins first. (That is, the plugins that are not refactored, but make use of the plugins are that are being refactored.) Plugin o.e.d.sqltools.data.core: Here's an example of an API change that is not strictly necessary for this refactor, I think. In TableDataImpl, the call ResultsViewAPI.getInstance().getMaxRowPreference() has change to ResultsViewAPI.getInstance().getMaxRowCount() If this is strictly a cleanup change, you could keep getMaxRowPrefernce as a deprecated call. Plugin o.e.d.sqltools.data.ui: No comments (import changes only) Plugin o.e.d.sqltools.sqlbuilder: Here's another case where you might be able to preserve the existing API, perhaps. Calls to ResultsViewAPI.getInstance().setCheckSRV(xxx) have disappeared, and calls to ResultsViewPlugin.getDefault().getResultManager().xxxResultManagerListener( were added. Is that essential to the refactoring? Plugin o.e.d.sqltools.sqlscrapbook: SQLDevToolsConfiguration has changed to SQLDevToolsUIConfiguration. Can you keep the old class as deprecated? Plugin o.e.d.sqltools.tabledataeditor: No comments (import changes only) More to come...
Thanks Briant, you point out the aspect I did not pay enough attention to in this refactoring work. I add inline comments as following. > Plugin o.e.d.sqltools.data.core: > > Here's an example of an API change that is not strictly necessary for this > refactor, I think. In TableDataImpl, the call > ResultsViewAPI.getInstance().getMaxRowPreference() > has change to > ResultsViewAPI.getInstance().getMaxRowCount() > If this is strictly a cleanup change, you could keep getMaxRowPrefernce as a > deprecated call. I change the name of getMaxRowCount back to getMaxRowPreference. > > Plugin o.e.d.sqltools.data.ui: > > No comments (import changes only) > > Plugin o.e.d.sqltools.sqlbuilder: > > Here's another case where you might be able to preserve the existing API, > perhaps. Calls to > ResultsViewAPI.getInstance().setCheckSRV(xxx) > have disappeared, and calls to > ResultsViewPlugin.getDefault().getResultManager().xxxResultManagerListener( > were added. Is that essential to the refactoring? Actually it is essential. The purpose of setCheckSRV(xxx) is to check the result view is active, if not activate it. This couple the non-ui with ui code, here I add listener to decouple them. > Plugin o.e.d.sqltools.sqlscrapbook: > > SQLDevToolsConfiguration has changed to SQLDevToolsUIConfiguration. Can you > keep the old class as deprecated? The old version of SQLDevToolsConfiguration has been broken into 2 classes, SQLDevToolsConfiguration and SQLDevToolsUIConfiguration, to separate the ui and non-ui code. The client has to do some code modification to adapt such refactor. > Plugin o.e.d.sqltools.tabledataeditor: > > No comments (import changes only) > > More to come... >
Well, OK. I hope the clients needing to change their code will understand the benefit. Go ahead and check in your changes. Get Brian F. to alert the community that the change is coming. I want to work with you more regarding making the SQL Editor overrideable or replaceable, but that is not part of your current refactoring, so I won't hold you up for that.
If someone can get me a list of the changes, we can put something up on the newsgroup & mailing list
Samuel, please check in asap, and send to writeup to Brian Fitzpatrick. Thanks.
Code committed. Tagged as v200904211440, pending on the migration doc to close this bug.
Created attachment 133886 [details] API change document
Closed.
