Community
Participate
Working Groups
CVS has an action set which provides the CVS main menu and the New Repository Location toolbar item. We should convert these to use the new declarative menu support.
Created attachment 59777 [details] Patch file Changes introduced by the patch: * org.eclipse.ui.menus extension point used in plugin.xml * uncommented section with handler in plugin.xml * mnemonics for Actions' labels * from now on TeamAction extends AbstractHandler and implements IActionDelegate2 (inter alia) * consequences of the above in subclasses (when needed) * mechanism for Action's for acting both as handlers and action delegates was taken (pulled up) from CVSAction Still to do: * handling exeptions thrown in TeamAction * UI messages for TeamAction * positioning of 'Checkout from CVS' button on the main toolbar * conversion of popup menus - objectContribution
The patch looks good so far. The only comments I have are: 1) You need to push up the error handling (which you already have on your to-do list). 2) I think we want to leave the actionSet defined but empty so we can use it in the visibility rules of the menus.
Created attachment 59859 [details] Patch file The patch file extends previous one with fixes for: * Handling exceptions thrown in TeamAction -- exception handling was pushed back to CVSAction where it had been taken from. This made action execution in TeamAction much simpler (less try/catches blocks) * UI messages for TeamAction -- introduced keys in TeamUIMessages and values in messages.properties * Positioning of 'Checkout from CVS' button on the main toolbar TODO: * Conversion of pop-up menus (objectContribution tag). We are going to open a new bug for it, aren't we? * Items in main menu are enabled by default. After you click on a selection for which action should be disabled a warning dialog shows up. The conclusion is that programmatically enablement works fine, the problem is with declarative enablement, BIMBW. Should we open new bug for it too? * The "Checkout from CVS" button on the toolbar looks odd (see attachment) * Javadocs
One more thing I have noticed is that after switching TeamAction to extend AbstractHandler and implement IActionDelegate2 instead of extending ActionDelegate the method selectionChanged(IAction. ISelection) isn't called as it was before. Before my changes to TeamAction, this method was called after any selection change, now it's called just before an action is executed. This causes inadequate selection values while checking enablement of an item both toolbar and main menu. Any tips? I'm a rookie but I belive for you Michael answer is obvious.
Looks good so far. Here are some comments: 1) TeamAction is not API. Therefore, I think we should make isEnabled abstract. 2) We don't want sublcasses to make the mistake of overriding run(IAction) so we should make that method final. I think runWithEvent should also be final. 3) In regards to your question on selection change events, in the old cvs action set, the actions implemented IWorkbenchWindowActionDelegate and in the init(IWorkbenchWindow) method, TeamAction added itself as a selection change listener on the window. For the new menu story, we need some way for the handler to listen to selection changes on the active page. I couldn't find this described on the menu wiki page so you'll have to ask Paul how to do this. 4) I've entered bug 175693 for the objectContributions. 5) Regarding the menu items being initially enabled. I'll like to get the solution to 3 working to see how that affects the situation. Once we have that, we can determine whether the behavior is acceptable or if a bug is required. 6) The "Checkout from CVS" button looks fine to me. Could you attach the screen shot of how it looks for you?
Created attachment 59874 [details] Screenshot The screenshot of my "CVS checkout" button. I'm using 3.3M5. PS.Sorry, forgot to attach it after submiting last comment.
What OS are you on? Is in Windows XP or WIndows 2000? Also, I would like to know what it looks like on the latest integration build as it is possible that the issue has already been addressed.
I'm on Windows XP. I've just tried Eclipse version: 3.3.0, build id: I20070222-0951 and it looks the same. I20070227-0800 is not available for download.
(In reply to comment #5) > Looks good so far. Here are some comments: > 1) TeamAction is not API. Therefore, I think we should make isEnabled abstract. No problem, I will simply remove, since it's already made abstract in AbstractHandler. > 2) We don't want sublcasses to make the mistake of overriding run(IAction) so we > should make that method final. I think runWithEvent should also be final. Yesterday we agreed to enable subclasses to override run(IAction). At this moment the only class which actually does it is CVSAction which adds some exception handling. However, I see you point so my suggestion is as follows: * Now (with latest patch applied): TeamAction public void run(IAction action) { try { execute(action); } catch (InvocationTargetException e) { // Handle the exception and any accumulated errors handle(e); } catch (InterruptedException e) { // Show any problems that have occurred so far handle(null); } } CVSAction overrides run(IAction) . *After my suggestion: final public void run(IAction action) { run2(action); } protected void run2(IAction action) { try { execute(action); } catch (InvocationTargetException e) { // Handle the exception and any accumulated errors handle(e); } catch (InterruptedException e) { // Show any problems that have occurred so far handle(null); } } This way CVSAction can override run2(Action) and still add some specific exception handling. I will make runWithEvent(Action,Event) final as you wish.
Actually, I forgot about the CVSAction overriding run(IAction). Just leave it like it is in the patch (i.e. don't introduce a run2 method). I've talked to Paul and I think we are going to need to create a property tester to do enablement for our menu items. To keep things as simple as possible, I think we should create a property tester that instantiates a second instance of each handler and runs the enablement of that handler. The xml for a handler would looks something like this: <handler class="org.eclipse.team.internal.ccvs.ui.actions.CommitAction" commandId="org.eclipse.team.cvs.ui.commit"> <activeWhen> <adapt type="org.eclipse.core.resources.mapping.ResourceMapping"> <test property="org.eclipse.core.resources.projectPersistentProperty" args="org.eclipse.team.core.repository,org.eclipse.team.cvs.core.cvsnature" /> </adapt> </activeWhen> <enabledWhen> <adapt type="org.eclipse.core.resources.mapping.ResourceMapping"> <test property="org.eclipse.team.cvs.ui.handlerEnablementTester" args="org.eclipse.team.internal.ccvs.ui.actions.CommitAction" /> </adapt> </enabledWhen> </handler> The activeWhen clause is using the same expression as the CVS objectContributions and indicates that the hanlder only applies to CVS projects. The enabledWhen clause is using a property tester that we need to write that would simply instantiate the handler class from the args list and run the enablement. The selection is provided to the tester as the default variable.
Created attachment 60323 [details] Patch file v03 This is next version of a patch for this bug. It addresses your last comment Michael. However, I'm still having some problems with making the new menu support working in 100%. 1) Appearance of CVS action set is customizable on the "Customize Perspective" window. What is interesting is that the option doesn't need to be ticked to enable CVS actions set, we just need to click it (tick it or untick it). 2) After adding handlers with enabled/activeWhen clauses to the plugin.xml, my runtime Eclipse started to spit out with warning (see exported log file in the next attachment). 3) Even though isEnabled method of a given Action class, and in consequence the propertyTester is returning false, the menu item is still enabled.
Created attachment 60324 [details] Exported log mentioned in comment 11
OK, here are my observations: 1) Paul's example: The propertyTester's test method is invoked only when the plug-in is activated/loaded. This happens when I use (click) one of the menu items. After that SelectionPropertyTester is constructed and it's methods are called. 2) Paul's example: Let's say I have the second menu item showed up (the one dependent on our SelectionPropertyTester), when I switch to another window and go back to Eclipse it disappears. To make it show up again I need to start clicking around Eclipse (change selection). 3) My patch: All menu items are visible and enabled. It doesn't matter what does SelectionPropertyTester return. 4) My patch: The "CVS check out" button on the main toolbar isn't visible at the start (as mentioned in comment 11). However, after any of CVS menu items is clicked, the button shows up (it's disabled), which means that button appear after the CVS propertyTester is constructed. The question is "why?". 5) My patch: After clicking on a menu item which should be disabled a proper message dialog shows up ("The chosen operation is not enabled"). At the beginning I took this for a good sign. The menu item is being disabled, that's fine. Unfortunately, it's now disabled forever. To be honest with you guys, I didn't expect this bug to be so hard. I'm especially concerned with it's unpredictable nature. I managed to make selectionProperty work as I would like to (it turned menu items on and off) but I simply don't remember a combination of "clicks" I'd made before. This is very embarrassing :/ Maybe I should checkout some extra projects from Eclipse repository? I've already got core.expressions, jface and ui.workbench...
Bullet 2) from comment 13 looks to be fixed in I20070306-1200.
Created attachment 60383 [details] Patch file v03.1 Updated version of the patch
(In reply to comment #13) > OK, here are my observations: > 1) Paul's example: The propertyTester's test method is invoked only when the > plug-in is activated/loaded. This happens when I use (click) one of the menu > items. After that SelectionPropertyTester is constructed and it's methods are > called. Right, that's the nature of the property tester. The behaviour that we really want to ensure is that if the test returns NOT_LOADED we interpret that as TRUE. > 2) Paul's example: Let's say I have the second menu item showed up (the one > dependent on our SelectionPropertyTester), when I switch to another window and > go back to Eclipse it disappears. To make it show up again I need to start > clicking around Eclipse (change selection). This could indicate a timing issue between when visibility is updated and when the window switching is fired. This will have to be investigated by us. > 3) My patch: All menu items are visible and enabled. It doesn't matter what > does SelectionPropertyTester return. It sounds like your property test method is being called. I'd be interested to know what the stack trace is like when that happens (you can attach a trace in a file if you want). > 4) My patch: The "CVS check out" button on the main toolbar isn't visible at > the start (as mentioned in comment 11). However, after any of CVS menu items is > clicked, the button shows up (it's disabled), which means that button appear > after the CVS propertyTester is constructed. The question is "why?". your property tester isn't valid until the plugin is loaded. But we really need to have it default to TRUE instead of FALSE. That's one possible explanation. > 5) My patch: After clicking on a menu item which should be disabled a proper > message dialog shows up ("The chosen operation is not enabled"). At the > beginning I took this for a good sign. The menu item is being disabled, that's > fine. Unfortunately, it's now disabled forever. What change in eclipse should re-enable it? i.e. a selection change? The active part change? Does the IHandler itself return isEnabled()==false after that. Does it ever change to true? > To be honest with you guys, I didn't expect this bug to be so hard. I'm > especially concerned with it's unpredictable nature. I managed to make Everything in eclipse is hard :-) You are doing great (and your work is invaluable). It's the converting the framework from the action framework to the command framework that is causing your instability, and I'm afraid that some of it won't go away until RC1. It wouldn't hurt for you to check out the platform-ui module (at the bottom of the expanded HEAD from cvs repository exploring). That would ensure you are working against the same code I am. PW
Created attachment 60422 [details] Stack trace Stack trace while calling test method from SelectionPropertyTester
(In reply to comment #16) > your property tester isn't valid until the plugin is loaded. But we really need to have it default to TRUE instead of FALSE. > That's one possible explanation. So there is nothing I can do about it? This is the way it should work, isn't it? > > 5) My patch: After clicking on a menu item which should be disabled a proper > > message dialog shows up ("The chosen operation is not enabled"). At the > > beginning I took this for a good sign. The menu item is being disabled, that's > > fine. Unfortunately, it's now disabled forever. > What change in eclipse should re-enable it? i.e. a selection change? The > active part change? Does the IHandler itself return isEnabled()==false after > that. Does it ever change to true? IMO the same event which calls test method from the SelectionPropertyTester should re-enabled the disabled item. It doesn't matter what kind of event it is. Anserwing your question about IHandler: (I'm not sure If this is what you were asking about) The isEnabled method doesn't seem to be called to determine if the dialog should be displayed. However, after I've clicked "OK" on the dialog the method is called , but this probably happens due to selection change. After that, after menu item has been disabled, isEnabled method returns either true or false depending on the current selection. So, it does change to true, if this is what you meant, but it doesn't affect menu item in anyway. It's still disabled.
(In reply to comment #18) > So there is nothing I can do about it? This is the way it should work, isn't > it? That is the way it should work, in that your property tester should not be called until after the plugin is loaded. However, things should default to visible and/or enabled if their property tester isn't loaded. If that's not the behaviour you are seeing, please open a bug with a summary start of [Contributions]. > IMO the same event which calls test method from the SelectionPropertyTester > should re-enabled the disabled item. > It doesn't matter what kind of event it is. That wasn't what I meant ... there are 2 enablement points, the enabledWhen expression in the XML and the isEnabled() method on the IHandler. I mean do you have 1 or both of these defined? What specific usecase do you have. Please open a bug and from a fresh workspace, create a small example plugin that has your menu contribution, command, and handler. If you attach the small example plugin (use File>Export...>Archive file to export it to zip) and write down your steps and your expected behaviour, I'll have a look at it. Although if it is changes to programmatic enablement that you expect to update the items, see my note below. > > Anserwing your question about IHandler: (I'm not sure If this is what you were > asking about) The isEnabled method doesn't seem to be called to determine if > the dialog should be displayed. However, after I've clicked "OK" on the dialog > the method is called , but this probably happens due to selection change. After > that, after menu item has been disabled, isEnabled method returns either true > or false depending on the current selection. So, it does change to true, if > this is what you meant, but it doesn't affect menu item in anyway. It's still > disabled. And when the value of the IHandler isEnabled() method changes from false to true, in the patch I didn't see it fire a HandlerChangeEvent. Without that, programmatic changes in enabled state won't notify anybody. PW
I've had a look at the patch and here is how I would like to proceed: 1) I would like you to extract out the conversion of TeamAction to an IHandler into a separate patch. We can then commit this to the repository since class and its subclasses should still work as both action delegates and handlers. Doing this now will simplify the process of inspecting future patches. I think that the code is already in good shape for this but I would like you to remove some of the code you've commented out. Also, in the TeamAction#run method you have a call the handle(null). I think the interrupt exception should just be silently ignore (i.e. we don;t want to prompt the user if they canceled the operation). 2) I think it would be a good idea to create some test cases for the SelectionPropertyTester just to make sure that the enablement it produces matches the enablement for the wrapped class. We can start with a few test cases (i.e. test enabled and disabled). The test setup is described on the wiki: http://wiki.eclipse.org/index.php/CVS_Development
Created attachment 62111 [details] Patch w/ TeamAction extending AbstractHandler
Created attachment 62112 [details] Patch w/ plugin.xml changes There is also SelectionPropertyTester class included in the patch.
Created attachment 62114 [details] Patch w/ tests for the SelectionPropertyTester
Provided patches don't consist any major changes comparing to attachment 60383 [details]. It's rather the new way of submitting fixes for this bug, as Michael suggested. However, while starting to work with the bug again two questions came up: 1) In reply to comment 19, after firing HandlerStateEvent I noticed that there is no listener waiting for that event. 2) Is the MenuEnablementTest.java class good place to put my tests for SelectionPropertyTester? And what is even more interesting for me are tests from this class valid and reliable? They don't seem to pass when launch against current version of Eclipse.
I have released the changes to TeamAction to Head. I would put the tests in a new test class (e.g. SelectionPropertyTesterTest). In regards to your HandlerStateEvent (or Paul's HandlerChangeEvent), I don't see this anywhere in the code. Is this something you tried but didn't include in the patch?
Created attachment 62138 [details] Minimized patch I've released the plugin.xml stuff with the menu support commented out. This will hopefully minimize the size of the future patches. I couldn't release the plugin.properites as there were many test changes so I've create a patch to the plugin.xml and plugin.properties that will enable the new work. I also logged a couple of bugs against UI for things that are currently blocking this one.
Created attachment 62239 [details] Patch w/ tests v2 I put the tests in a new test class. I also added some new testXXX methods. So you can tell if this is proper way of testing the SelectionPropertyTester. While extracting test methods from MenuEnablementTest I realized that I will need to duplicate some code. To avoid that I created new class EnablementTest which is extended by both MenuEnablementTest and SelectionPropertyTesterTest. I could have created an utility class, but I thought this is not the best solution. Second option wast to extend MenuEnablementTest in my SelectionPropertyTesterTest. This way I would have access to the methods I need. However, launching SelectionPropertyTesterTest, would also run tests methods from MenuEnablementTest, which in my opinion is inappropriate.
(In reply to comment #25) > (...) In regards to your > HandlerStateEvent (or Paul's HandlerChangeEvent), I don't see this anywhere in > the code. Is this something you tried but didn't include in the patch? You're tight, I'd removed it before I wrote that comment. The issue I was referring to is easy to reproduce. What I did is add fireHandlerChanged(new HandlerEvent(this, true, true)); as the first line inside org.eclipse.team.internal.ccvs.ui.actions.WorkspaceAction#isEnabled. (If you wish, I can submit a patch for that.) I know that it's not the best place to put it in, but this way I can easily check if there are any listeners register for a handler. At least that's what I think I'm doing. The next step is to lauch Eclipse application in debug mode. Here is what I noticed for CommitAction: getListeners() from line 68 of AbstractHandler return an empty array and this is my concern. If I'm supposed to call fireHandlerChanged() I belive there should be a listener waiting for that event. The other problem is that, when I'm using new instance of an action in SelectionPropertyTester (line 66) it has no listeners registered either. Stack from debug: Thread [main] (Suspended) CommitAction(AbstractHandler).fireHandlerChanged(HandlerEvent) line: 69 CommitAction(WorkspaceAction).isEnabled() line: 291 CommitAction(WorkspaceAction).beginExecution(IAction) line: 65 CommitAction(CVSAction).run(IAction) line: 111 CommitAction(TeamAction).runWithEvent(IAction, Event) line: 511 ActionDelegateHandlerProxy.execute(ExecutionEvent) line: 276 Command.executeWithChecks(ExecutionEvent) line: 471 ParameterizedCommand.executeWithChecks(Object, Object) line: 424 HandlerService.executeCommand(ParameterizedCommand, Event) line: 164 SlaveHandlerService.executeCommand(ParameterizedCommand, Event) line: 247 CommandContributionItem.handleWidgetSelection(Event) line: 514 CommandContributionItem.access$8(CommandContributionItem, Event) line: 508 CommandContributionItem$3.handleEvent(Event) line: 498 EventTable.sendEvent(Event) line: 66 MenuItem(Widget).sendEvent(Event) line: 938 Display.runDeferredEvents() line: 3673 Display.readAndDispatch() line: 3284 Workbench.runEventLoop(Window$IExceptionHandler, Display) line: 2361 Workbench.runUI() line: 2325 Workbench.access$4(Workbench) line: 2200 Workbench$4.run() line: 466 Realm.runWithDefault(Realm, Runnable) line: 289 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 461 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149 IDEApplication.start(IApplicationContext) line: 101 EclipseAppHandle.run(Object) line: 152 EclipseAppLauncher.runApplication(Object) line: 106 EclipseAppLauncher.start(Object) line: 76 EclipseStarter.run(Object) line: 359 EclipseStarter.run(String[], Runnable) line: 174 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: not available DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: not available Method.invoke(Object, Object...) line: not available Main.invokeFramework(String[], URL[]) line: 476 Main.basicRun(String[]) line: 416 Main.run(String[]) line: 1141 Main.main(String[]) line: 1116
(In reply to comment #28) > You're tight, I'd removed it before I wrote that comment. (...) I meant "You're right". Sorry for that typo ;)
(In reply to comment #28) > referring to is easy to reproduce. What I did is add fireHandlerChanged(new > HandlerEvent(this, true, true)); as the first line inside As an aside, if you change your enabled state just mark that as true. Usually handler proxies fire the handlerChanged event. > org.eclipse.team.internal.ccvs.ui.actions.WorkspaceAction#isEnabled. (If you > wish, I can submit a patch for that.) I know that it's not the best place to > put it in, but this way I can easily check if there are any listeners register > for a handler. At least that's what I think I'm doing. The next step is to > lauch Eclipse application in debug mode. Here is what I noticed for > CommitAction: I haven't had a chance to review your stuff, but just a few quick comments. Your handler won't have a listener until it's the active handler. Also, the action in ActionDelegateHandlerProxy is currently disconnected. There's a bug open about this. Later, PW
This is not containable in the 3.3 release so we're postponing it until 3.4.
Mass update - removing 3.4 target. This was one of the bugs marked for investigation (and potential fixing) in 3.4 but we ran out of time. Please ping on the bug if fixing it would be really important for 3.4, and does not require API changes or feature work.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
