Bugzilla – Full Text Bug Listing
| Summary: | creating command handlers to manipulate tasks list elements | ||
|---|---|---|---|
| Product: | z_Archived | Reporter: | Izzet Safer <isafer> |
| Component: | Mylyn | Assignee: | Mylyn Inbox <mylyn-inbox> |
| Status: | RESOLVED WONTFIX | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P4 | CC: | murphy, wmitsuda |
| Version: | unspecified | Keywords: | helpwanted |
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | Windows 2000 | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Izzet Safer
This sounds good Izzet. In the future please try to focus your bug report descriptions on a single problem or enhancement, and submit multiple reports if there are multiple. The first thing to do here is submit a patch for one or two of the Task List commands that you would like to see, and then we can iterate. Created attachment 44854 [details]
Commands to add tasks and subtasks, and activate/deactivate tasks
These commands can be used (in cheat sheets) as the following:
<command returns = "TaskHandle"
serialization = "org.eclipse.mylar.tasklist.commands.addNewLocalTask(taskName=School Schedule Application,taskHandle=MyApp-10,categoryName=Cheat Sheets)"/>
<command returns = "TaskHandle2"
serialization = "org.eclipse.mylar.tasklist.commands.addNewLocalSubTask(taskName=Create UI,parentHandle=MyApp-10,activateTask=false)"/>
<command returns = "retVal"
serialization = "org.eclipse.mylar.tasklist.commands.activateTask(taskHandle=MyApp-10,activate=false)"/>
Please inspect the commands extension point for other parameters of the commands
Created attachment 44873 [details]
Commands to add, activate, mark complete tasks and subtasks
added command to mark tasks complete/incomplete
fixed a small bug in add task (if an already existing task is added and its context is wanted to be reset, the task's subtask will be cleared too)
I looked over the patches Izzet and have a couple of questions before applying: - Should we be replacing the current actions (e.g. NewLocalTaskAction) with these handlers? If not we will have duplicate code and that's a very bad idea because it is unlikely to get updated consistently. If we do not merge the actions and commands somehow then I think that you should rewrite the commands to delegate to the actions, and you could give the actions a method that takes additional parameters. - Is there a better way of doing the parameter names than strings? We refactor the code frequently and these won't get changed with the refactorings. Is there a convention for this? If there is no better way then I think you will neeed to add some unit test coverage here to ensure you are getting the right field values. I can say that in essence they are doing the same job and we can replace Actions with Commands. A presentation I found from eclipse con proposes that the commands will be used instead of actions in the future. (http://www.magma.ca/~pollockd/despumate/eclipseCon2006.pdf) But for now, as a quick fix, if we don't want to keep two different structures doing the same job, I can delegate the commands to actions, as you proposed. On the other hand, although it's not well modularization, I need some commands to do multiple jobs. For example, according to a given parameter, when I add a task, I may want it activated right away. The reason I don't want to do this with two separate commands is that, within cheat sheets the users have to click to a link to trigger the commands and each click can run only one command. I don't want the users click tens of times for a series of operations. If I'm delegating commands to actions, I will be then delegating a command to multiple actions. Mik, for the parameter names, are you talking about the ones in the handler code or in the command element to call the handlers? If you're talking about the handlers, I think we can get the parameter names from the command definition in the extension point. I'll look more into that. If you're talking about calling the commands, since the commands are serialized/deserialized, I don't think we have any other option. I took a look to parameter names, as far as I understand there is a convention about the parameters when the values are from a given set (e.g. a command to show a perspective can only accept valid perspective ids) but this is not our case. I think using constants for parameter names will be a good and feasible action OK, go ahead and do it with constants, and make sure that both handler and extension point reader use those constants. For now please make your patch delegate to the actions, and down the road we can refactor everything into commands. Regarding needing to do more in your commands, could you make the more sophisticated commands a part of your plug-in (rather than being part of mylar.tasklist)? I should tell that I have to do lots of refactoring in NewLocalTaskAction. I have to add another constructor, extract the contents of the run method to make the code reusable and modify its content to work with parameters. You are ok with that right? I'm not creating a separate plug-in for my cheat sheet work at all. I have to do some extensions to mylar.monitor too, if you want I can combine all in a separate plug-in but it will be awkward since one part will fully belong to the tasklist, and the other to the monitor. Created attachment 44963 [details]
Commands to add, activate, mark complete tasks and subtasks delegated to existing actions
Mik, could you take a look if the patch seems good? It still may need some polishing.
I also had to create NewLocalSubTaskAction since I am using subtasks. (I haven't added anything to do it visually though) When are you going to be done with the subtask support? I also found a bug in subtask activation, should I go ahead and report it or wait for the completion of the feature?
The refactoring should be fine, but try to keep it minimal. I will review your patch tomorrow morning. I'm surprised that you aren't creating a seperate plug-in because this is new functionality. Even if it were to go into Mylar directly it should go into either a new plug-in that goes into the sandbox or into the sandbox/org.eclipse.mylar.sandbox plug-in (probably in a cheatsheats package). Your plug-in can extend both the monitor and the tasklist. Please post some notes on your architecture so that I can advise. Regarding sub-tasks, that has been deferred to 0.7. I suggest keeping the sub task action and related behavior in your plug-in for now, and we can move it into the core once we have made progress on but 137543. The reason I haven't created a separate plug-in yet is because I've created commands to create/manipulate taks (which I believe semantically belong to the tasklist package) and doesn't have anything to do with cheat sheets. These commands could be used by any program who understands 'commands' and cheat sheets are just one of them. For the monitor extensions, I didn't finish it yet but I'll create a separate plug-in. Also, I haven't clearly understood what that sandbox in mylar does. Could you give me a brief info? Yes, as long as the contributions are all about task list actions that's fine. This is a big patch, but I expect to be able to review it tomorrow. For future references it would have been better to do a single command, get my feedback on that patch, have it integrated, and do the others. But I took a quick look and it looks like the right idea now. The sandbox is just a place for experimental plug-ins that don't ship with the release. I'll post some docs online about this. Izzet, unfortunately with the 0.6/Callisto stabilization I can't apply this big a change until next week. For now I assume that you are able to continue using these changes from your locally modified workspace. No problem, I'll continue locally. Furthermore I discovered a bug in my add new subtask command and fixed it, I can post it later. Izzet, due to the refactoring we'll need this patch re-cut for me to apply. You should be able to do that by moving your classes into the new packages, organizing imports, and changing any necessary plug-in IDs. I think that compiler should complain about all of those problems, and the implementation of the classes has not changed. Thanks Mik. I am aware of the changes, I'll post a new patch soon. Izzet: do you have a patch coming for this? It would be good to maek this change, and ensure that we're supporting your cheat sheet extensions. Created attachment 47037 [details]
latest patch
Commands to add, activate, deactivate, mark complete and incomplete tasks and subtasks. Commands are delegated to existing actions. The patch also includes a findTask method added to TaskList, used by the commands to find tasks and subtasks according to given handles
I just reviewed this again and do this right will require additional of refactoring on our end (e.g. some of those actions should not be referring to the Task List view). But we definitely do want to integrate this handler contribution, so I will look into this again in about 2 weeks (after I return from vacation). Also, the TaskList.findTask(..) code that you added to TaskList is not quite right. A fundamental part of the design of the Task List is that tasks have unique handle identifiers. So TaskList.getTask(handleIdentifier) should always return the task without requiring a search. The logical structure of the TaskList (e.g. queries, containers, and subtasks), is maintained seperately. Note that the subtasks part of that structure won't be right until we complete bug 151655. Also note that it's not a good idea to use an a field for storing a temporary computation value that way (i.e. your search). Could you give some examples of my actions which should not refer to Task List view? If I didn't skip anything by mistake, while modifying, I sticked to actions' last implementation. If you said that because you saw my NewLocalTaskAction was based on version 1.1, I based it on 1.3 properly. It's just Eclipse who wasn't able to merge the files successfully when updating because I made lots of changes. The only reason findTask is not O(1) is just because it looks for subtasks and subtasks are kept in a set of tasks. As you said, unless the subtask support is finalized, I guess my solution is the only viable one. (For constant time lookup we can change 'Set<ITask> children' in Task.java to 'Map<String,ITask> children' and use the handle as key and the task as the value) I apologise for nonelegance of my implementation, I had to come up with a quick solution. I'll do a better one for the actual patch. Only time for quick reply on 1st paragraph: the coupling to the view is not a problem on your patches end. We need to improve the notification/listener mechanism for task changes so that this isn't needed. Also need Alt+Enter to open edit query dialog. (In reply to comment #22) > Also need Alt+Enter to open edit query dialog. > This can be handled creating a retargetable "properties" action (similar to the Synchronize - F5) and the Alt+Enter would come for free. The problem with Alt+Enter is that the "obvious" behavior would be to open a "query properties" dialog. Not sure if this applicable or appropriate here. The action revamp for the Task List needed to make this happen will have to wait post 1.0. FYI: link to the command framework on wiki. http://wiki.eclipse.org/index.php/Platform_Command_Framework Tentatively scheduling for 3.0. We will likely do this incrementally as we improve the way that actions are handled. We currently don't have a driver to replace existing actions with command handlers. We can reconsider if someone was willing to provide a contribution. |