Community
Participate
Working Groups
The current API for LinearUndoViolationUserApprover assumes that this approver is installed on an editor. (IEditorPart). Internally, this type is used mainly as an IWorkbenchPart, with the only editor-specific usage being to use the editor input name for the message dialog name. It is conceivable that clients would want to provide instead an IWorkbenchPart, since this could apply to views. The part title could be used instead.
mvm - this is not critical for the operation of RC2. However, it is a simple API change that could prevent adding another constructor in a future release to generalize the part parameter for a LinearUndoViolationUserApprover. It does not affect current clients. Changing the constructor parameter extends this user approver to be used for views and editors, not just editors. The use of IEditorPart is a legacy of function that was removed from this class and moved into another class. When the move was done, I didn't realize at that time that I could generalize the part type. Just realized this while reviewing another bug. The API change would be from public LinearUndoViolationUserApprover(IUndoContext context, IEditorPart part) to public LinearUndoViolationUserApprover(IUndoContext context, IWorkbenchPart part) thoughts? do you think this is worth surfacing now or dealing with later?
Well the only thing that scares me about chaging this API is handling the cases we have not yet tested. I.e. passing in all other types of IWorkbenchPart. But if the coded added a test for instanceOf IEditorpart to protect itself for 3.1 this seems in principle to be a good idea. Who will be affected by this change? cc'ing Jim for the api request.
API change approved.
In looking at this further, another API question is whether we should use IWorkbenchPart or IWorkbenchPart2 in the method signature, since the part name is what is needed. What is the best practice? To use the newer interface or to use the older one and do an instanceof check and use the newer interface when present? The only SDK client affected is AbstractTextEditor. It passes itself as the part, so it would not be affected by a change in the signature to IWorkbenchPart2. I don't know of any other clients currently. However using IWorkbenchPart2 could theoretically break a user currently using IEditorPart outside of the EditorPart hierarchy. The part is used as follows: part.setFocus() part.getSite() part.getPartName() (IWorkbenchPart2) or part.getTitle() (IWorkbenchPart) - Used to be part.getEditorInput.getName() In the SDK code, we believe that this approver will only be invoked in unexpected error cases. In theory, other user approvers installed by the workbench will catch this case before this approver is invoked, although there have been some reported (currently unreproducible) cases where this approver is invoked. I would suggest if we make the change, we do not limit the use with an instanceof check. If the check is put in, clients can only use it on editors. If the check is not put in, new clients will be able to install on views, and if there are problems found, the bugs will be fixed (no worse than waiting for the initial implementation?). Clients outside of AbstractTextEditor have to specifically install this approver, it won't "magically" appear anywhere. That said, I defer to your judgment. I see these open questions: - IWorkbenchPart2 or IWorkbenchPart? - Limit the use to editors for 3.1? If so, how?
I think you want the lowest common denominator in this case IWorkbenchPart if all you need is the name.
oops, ignore last comment, I was trying to add a cc member and some random thoughts were committed.
You were correct though. Not all parts necessarily implement IWorkbenchPart2, so we should use IWorkbenchPart in the signature. Although WorkbenchPart does implement IWorkbenchPart2, the contract between the workbench and clients is in terms of the interfaces (IWorkbenchPart, IWorkbenchPart2, IEditorPart, IViewPart), not the convenience implementations (WorkbenchPart, EditorPart, ViewPart). The workbench implementation should never refer directly to the latter. The code should do: String getTitle(IWorkbenchPart part) { if (part instanceof IWorkbenchPart2) { return ((IWorkbenchPart2) part).getPartName(); } return part.getTitle(); } cc'ing Stefan to confirm.
mvm - given the discussion so far, do you still feel we should limit the use of this class to IEditorInput? I think the use of the part variable is fairly simple and it would be more straightforward to broaden the API to IWorkbenchPart, obtain the title as suggested below, and allow callers to supply views. Otherwise we have to disallow (by exception?) or no-op in the case of a non-editor, and this could cause surprises later on when we enabled it.
If all cases are tested then I agree with doing the right thing, but as you know, my fear is that just putting in things that "sound right" without really having a use case that tests it might not be the right idea.
Depends what you mean by test "all cases." The current implementation uses IEditorPart and is tested by the AbstractTextEditor framework. Opening up the API from IEditorPart to IWorkbenchPart basically introduces new use by IViewPart and any implementations of IWorkbenchPart that are not editors or views. This class is currently manually tested since it involves a user prompt. I intend to add tests to try it on a workbench view and a dummy part that answers null for the title and site. There's no practical way to test it against all possible views, so the question is whether you think this is enough testing. The reason I am fairly confident is that a programmer has to explicitly install this class on a view. In 3.1, it is not installed automatically by the workbench on behalf of some random view/editor, so there is no risk that an untested code path is introduced in the SDK itself.
Note that AbstractTextEditor sets its title to be that of its input, but this is not done in EditorPart. Other editors may provide some other value. However, the code above will still give the text that's shown on the editor or view tab.
mvm - still looking for your opinion on whether the testing proposed in comment #10 is enough.
sorry, yes it is, I'm ok with manual or automated for now. Just looking for a test that actually tries to call this API with any non - IEditorPart basically.
Created attachment 22568 [details] org.eclipse.ui.workbench patch proposed change + change to externalized strings
Proposed patch has been provided. Tested against the text editor hierarchy and against Package Explorer. Added additional bulletproofing for null titles. mvm - can you please approve the fact that two externalized messages also changed. The changes were: - put quotes around the part name when referred to. I added quotes because we can't assume the part name is a file name that makes sense in context. - changed "Local edits in {0}" to "Local changes in {0}" since the message applies to a part, not just an editor I will release this change on your approval.
approve additional messages
Released to HEAD, appearing >20050607.
Oops. Didn't notice this one until (it seems) too late. Nick's code in comment 7 is reasonable... but a much better solution would be to take an IWorkbenchPartReference. IWorkbenchPartReference.getName() will always give you the name from the tab without any instanceof checks... and it will even work correctly if the part hasn't been materialized yet.
Sorry: I meant IWorkbenchPartReference.getPartName()
Thanks, Stefan. In this case we were making a late-breaking change to an API that was already spec'ed as a part (IEditorPart) and generalizing it. The current SDK clients have the part already (in fact it's usually the part itself providing the parameter.) So we don't need an IWorkbenchPartReference in this case, but thanks for the info.
It may be better to ask the UI team to promote WorkbenchPage.getReference(IWorkbenchPart) to API, that way clients can get a reference from their part. It is always better to refer to a part reference than a concrete part.
I see what you mean, Stefan. However, this would mean two more API changes going into RC2, and platform text changing their code to pass the part reference for an editor. When I opened this bug, I could definitely imagine a client needing to instantiate the LinearUndoViolationUserApprover on a view vs. an editor and being restricted by the current API. This change would be reasonable if we believe there are common cases where a client has an IWorkbenchPartReference, but no part. I'm inclined to leave as is, adding an additional part reference signature later if needed. But if anyone disagrees (ie, we are trying to move platform UI API toward IWorkbenchPartReference over IWorkbenchPart), please reopen this.
"ie, we are trying to move platform UI API toward IWorkbenchPartReference over IWorkbenchPart" You got it exactly. We want clients to implement parts, and refer to part references. If we later add a new IWorkbenchPart3, then the instanceof checks in comment 7 would need to change, but IWorkbenchPartReference will continue to work for all time.
mvm/nick - is the generalization from IWorkbenchPart to IWorkbenchPartReference something we want to handle prior to 3.1 release?
It's not urgent. I just meant it as a recommendation.
I don't think it's necessary for 3.1, unless there's a chance that the Approver will outlive the part, in which case references hold onto less garbage.
Approvers are generally installed like listeners. Added when the part is created and removed as it is disposed. I think we are okay. Leaving this bug closed since it addresses the original problem. If there's a future problem requiring IWorkbenchPartReference, we'll open a bug for it. thanks everyone for the info.
verified in I20050609-1228 by inspecting the code in version v20050609-1200 and running the manual test scenario.
