Bugzilla – Full Text Bug Listing
| Summary: | Cannot extend the GEF Palette | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Anthony Hunter <ahunter.eclipse> | ||||
| Component: | GEF-Legacy GEF (MVC) | Assignee: | Anthony Hunter <ahunter.eclipse> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | ahunter.eclipse, cbmcgee, hudsonr, ppshah, remy.suen, thatnitind | ||||
| Version: | 3.4 | ||||||
| Target Milestone: | 3.4.0 (Ganymede) M3 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Anthony Hunter
In order to preserve the history of these files, I copied org.eclipse.gef.internal.ui.palette.editparts.PaletteEditPart org.eclipse.gef.internal.ui.palette.editparts.PaletteAnimator; org.eclipse.gef.internal.ui.palette.editparts.PaletteToolbarLayout; in CVS to org.eclipse.gef.ui.palette.editparts. I had to make a few fixes to the classes in org.eclipse.gef.internal.ui.palette.editparts now that the supporting classes were public. Created attachment 80706 [details]
Patch for GEF extensibility
Hi Chris, can you check out the attached patch. This fix seems to have caused some issues with the latest CVS HEAD. The three files are in the org.eclipse.gef.ui.palette.editparts package now but they will not compile as their declared package is still internal. These files also aren't being tagged with @since 3.4 even though they are new API. (In reply to comment #4) > This fix seems to have caused some issues with the latest CVS HEAD. The three > files are in the org.eclipse.gef.ui.palette.editparts package now but they will > not compile as their declared package is still internal. Yes, since I hate loosing history, I copied the modules in the CVS hierarchy. I will fix tomorrow, but not sure why anyone would be developing with source in the workspace from the CVS repository. Did you need to do this for a reason? (In reply to comment #5) > I will fix tomorrow, but not sure why anyone would be developing with source in > the workspace from the CVS repository. Did you need to do this for a reason? No, I didn't _need_ to do this, but I just happen to have the GEF sources checked out because I need it to compile some stuff from ECF. Is it so odd for a non-developer to have code from CVS HEAD in their workspace? :) (In reply to comment #5) > I will fix tomorrow Fixes committed to HEAD Where are the uses cases that describe why the palette should be made API. What is wrong with leaving the palette as internal, but exporting it so that brave clients can extend it? One alternative to opening up the API is just to incorporate the enhancement in GEF instead of at the client. Maybe that's possible here? (In reply to comment #8) > Maybe that's possible here? Not possible. I'm all in favor of exposing interface while keeping it internal, so clients can tweak as they need to, while GEF retains the flexibility to change the code as it evolves. I think this applies more so to the palette-related code, since it's quite fragile. In general, making API non-internal should require a good case, since it would mean an additional load on the GEF team to support and maintain that code. This change request hasn't mentioned what that case is yet. You can be certain that clients will try to use the PaletteToolbarLayout elsewhere, and request enhancements. Anthony, you should make sure that the benefit justifies the burden. Wasn't there talk about refreshing the palette? GEF would lose the ability to arbitrarily change the behaviour/interface of the non-internalized classes. (In reply to comment #10) > Wasn't there talk about refreshing the palette? GEF would lose the ability to > arbitrarily change the behaviour/interface of the non-internalized classes. Yes, the palette work is Bug 204631 and is going to be done in 3.4. So what was the fix? (In reply to comment #12) > So what was the fix? > The fix to this bug was: > We would like the following in the public package: > > org.eclipse.gef.internal.ui.palette.editparts.PaletteEditPart > org.eclipse.gef.internal.ui.palette.editparts.PaletteAnimator; > org.eclipse.gef.internal.ui.palette.editparts.PaletteToolbarLayout; > These three are now public: org.eclipse.gef.ui.palette.editparts.PaletteEditPart org.eclipse.gef.ui.palette.editparts.PaletteAnimator; org.eclipse.gef.ui.palette.editparts.PaletteToolbarLayout; We should really consider reverting these changes. I think in this case the desired behavior can be obtained using existing techniques. Also, there are other concerns which haven't been covered, such as the PaletteViewerKeyHandler, which is not designed to function in the presence of unknown editparts. I concur. As mentioned in comment 10, this will make it much harder to change the palette's operation in the future and there's still not a clear usage case of how these three classes would be used to extend the palette. (In reply to comment #14) > We should really consider reverting these changes. We can talk off-line. |