Community
Participate
Working Groups
The DeleteAction contains quite an impressive amount of logic around deletion of tasks and containers. Would be great to have this in the core so other extensions to the task list could leverage that functionality without depending on the UI.
Agreed. This should be extracted and available as API on repository model.
Looks like we'll need an extension point so that the UI can register a pre- and post- listener for deleting tasks.
7905: 349955: [api] Move Delete (task/container) logic to core [Ia6ec6829] https://git.eclipse.org/r/#change,7905 Because things like TaskDataManager are only accessible from UI, the actual deletion still happens in the UI (in a listener), which means the core doesn't do anything except call the listeners. Would it make sense to create a TasksCoreUtil class that provides accessors for the TaskDataManager, RepositoryManager, etc.?
Since we already have ITaskListChangeListener, it doesn't make sense to me to add another listener. Could we add another Kind to TaskContainerDelta for an event that would be fired before the deletion happens? Most of the actual deletion should be moved to the core, but we do need some kind of listener to close the open editor, and currently this is done before the deletion happens.
(In reply to comment #3) > Would it make sense to create a > TasksCoreUtil class that provides accessors for the TaskDataManager, > RepositoryManager, etc.? I would like to keep tasks.core as a stateless library. We can create an additional bundle to provide a service for accessing the default RepositoryModel and TaskList instance but we should look into how we far we get in terms of resolving this bug without going down that path. The proposed change goes into the right direction but it seems that there is a still a lot of logic in the UI listener class which is what we need to get rid of and wire up the manager classes through listeners. (In reply to comment #4) > Since we already have ITaskListChangeListener, it doesn't make sense to me to > add another listener. Could we add another Kind to TaskContainerDelta for an > event that would be fired before the deletion happens? Most of the actual > deletion should be moved to the core, but we do need some kind of listener to > close the open editor, and currently this is done before the deletion happens. Do these actions need to happen pre-deletion? Just wondering if we could get away without that.
(In reply to comment #5) > (In reply to comment #3) > > Would it make sense to create a > > TasksCoreUtil class that provides accessors for the TaskDataManager, > > RepositoryManager, etc.? > > I would like to keep tasks.core as a stateless library. We can create an > additional bundle to provide a service for accessing the default RepositoryModel > and TaskList instance If we allow the core to depend on the bundle then I'm not sure what we gain by having a separate bundle, and if we don't allow the dependency, then I don't know how it helps, unless we put the deletion logic in this (or another) bundle. But then it might as well stay in the UI. > but we should look into how we far we get in terms of > resolving this bug without going down that path. I don't see how we can get much further without having access to TaskDataManager and other things that are only in the UI. > The proposed change goes into the right direction but it seems that there is a > still a lot of logic in the UI listener class which is what we need to get rid > of and wire up the manager classes through listeners. I agree, although this approach does at least allow the core to have an entry point to do the deletion. But as long as TaskDataManager and other things are not available in the core itself (i.e. not in a special bundle), the deletion will always depend on the UI. > > (In reply to comment #4) > > Since we already have ITaskListChangeListener, it doesn't make sense to me to > > add another listener. Could we add another Kind to TaskContainerDelta for an > > event that would be fired before the deletion happens? Most of the actual > > deletion should be moved to the core, but we do need some kind of listener to > > close the open editor, and currently this is done before the deletion happens. > > Do these actions need to happen pre-deletion? Just wondering if we could get > away without that. We could try, but if we're going to move the recursion into the core, then we may still need a separate deletion event, as recursively calling ITaskListChangeListener could break things. I wonder if it would make sense for the TaskDataManger, activity manager, task list, etc. to each listen for deletions and be responsible for cleaning up their own state? I don't know whether they need to clean up in a particular order.
(In reply to comment #6) > > I would like to keep tasks.core as a stateless library. We can create an > > additional bundle to provide a service for accessing the default > RepositoryModel > > and TaskList instance > > If we allow the core to depend on the bundle then I'm not sure what we gain by > having a separate bundle, and if we don't allow the dependency, then I don't > know how it helps, unless we put the deletion logic in this (or another) bundle. > But then it might as well stay in the UI. The core wouldn't depend on it but it would allow other core bundles (not connectors) to consume the common instance of the data model without coupling to the UI. > > The proposed change goes into the right direction but it seems that there is a > > still a lot of logic in the UI listener class which is what we need to get rid > > of and wire up the manager classes through listeners. > > I agree, although this approach does at least allow the core to have an entry > point to do the deletion. But as long as TaskDataManager and other things are > not available in the core itself (i.e. not in a special bundle), the deletion > will always depend on the UI. Yes and no. There needs to be an entity that knows the various manager instances but these don't necessarily need to know about each other. We could couple them loosely through listeners. These listeners could live in a class that manages the model as a whole which I would propose should be in the core model bundle (but can be in the UI for now). > I wonder if it would make sense for the TaskDataManger, activity manager, task > list, etc. to each listen for deletions and be responsible for cleaning up their > own state? I don't know whether they need to clean up in a particular order. Yes, that sounds right to me.
I think this makes sense. I will take another pass at this.
Mylyn has been restructured, and our issue tracking has moved to GitHub [1]. We are closing ~14K Bugzilla issues to give the new team a fresh start. If you feel that this issue is still relevant, please create a new one on GitHub. [1] https://github.com/orgs/eclipse-mylyn
