Add recursive deletion for components with children #89

Closed
jariba opened this Issue Mar 22, 2013 · 8 comments

Comments

Projects
None yet
4 participants
@jariba

jariba commented Mar 22, 2013

User should be able to delete subcomponents from the top level without having to delete each child individually, which very cumbersome.

@DanBerrios

This comment has been minimized.

Show comment
Hide comment
@DanBerrios

DanBerrios Mar 26, 2013

Contributor

In the queue of feature requests. Given the potential for accidental deletion of large numbers of components, this requires sufficient UI warnings before implementation.

Contributor

DanBerrios commented Mar 26, 2013

In the queue of feature requests. Given the potential for accidental deletion of large numbers of components, this requires sufficient UI warnings before implementation.

@jariba

This comment has been minimized.

Show comment
Hide comment
@jariba

jariba May 7, 2013

We care primarily about having an API available, UI is nice to have.

jariba commented May 7, 2013

We care primarily about having an API available, UI is nice to have.

@iorbitearth

This comment has been minimized.

Show comment
Hide comment
@iorbitearth

iorbitearth Jun 18, 2013

Not a priority

Not a priority

@VWoeltjen

This comment has been minimized.

Show comment
Hide comment
@VWoeltjen

VWoeltjen Aug 22, 2013

Contributor

One option would be to have delete function recursively, and pop up a verification dialog with lists of components which will be deleted entirely and components which will still exist somewhere in the system (there is a similar dialog that comes up when removing multiple manifestations, IIRC)

That approach doesn't deal with the case where another user owns some child object somewhere in the deletion graph. This should be okay if that object exists somewhere else (effectively making it a remove manifestation). Maybe disallow the action in the case where there is some undeletable descendant that is the last reference to that object in existence? (I believe this case was the motivation for the current delete rules.)

Contributor

VWoeltjen commented Aug 22, 2013

One option would be to have delete function recursively, and pop up a verification dialog with lists of components which will be deleted entirely and components which will still exist somewhere in the system (there is a similar dialog that comes up when removing multiple manifestations, IIRC)

That approach doesn't deal with the case where another user owns some child object somewhere in the deletion graph. This should be okay if that object exists somewhere else (effectively making it a remove manifestation). Maybe disallow the action in the case where there is some undeletable descendant that is the last reference to that object in existence? (I believe this case was the motivation for the current delete rules.)

VWoeltjen added a commit to VWoeltjen/mct that referenced this issue Oct 15, 2013

[Delete] Copy Delete action to Delete All
Create a copy of the Delete action to serve
as the basis for Delete All, for nasa/mct#89

VWoeltjen added a commit to VWoeltjen/mct that referenced this issue Oct 15, 2013

[Delete] Specify name of 'Delete All' action
Delete All is specified as the name for the recursive
delete action requested by nasa/mct#89.

This name is chosen for symmetry with the 'Save All'
action introduced in
770df79

VWoeltjen added a commit to VWoeltjen/mct that referenced this issue Oct 16, 2013

[Delete] Use Window Manager for Delete All dialog
Use the Window Manager (as opposed to static methods
of OptionBox) to show the warning dialogs related
to the Delete All action introduced for nasa/mct#89.

This slightly reduces the explicit Swing
dependencies associated with the action and
facilitates automated testing of interactions
(the WindowManager can be mocked, but the static
methods of OptionBox cannot). These are ongoing
concerns of nasa/mct#155.
@VWoeltjen

This comment has been minimized.

Show comment
Hide comment
@VWoeltjen

VWoeltjen Oct 16, 2013

Contributor

Beginning work on this issue. Using "Delete All" as name for menu item, for symmetry with Save All.

Note that there are two possible interpretations for dealing with owned descendants in a recursive Delete All:

  • Delete the owned descendant, even if it appears somewhere else.
  • Delete the owned descendant if it is the last manifestation of the object; otherwise, leave it be. (Effectively this is like a Remove Manifestation that results in deletion, with the user implicitly confirming it)

Using the former definition as that is most consistent with the use cases others have expressed.

Contributor

VWoeltjen commented Oct 16, 2013

Beginning work on this issue. Using "Delete All" as name for menu item, for symmetry with Save All.

Note that there are two possible interpretations for dealing with owned descendants in a recursive Delete All:

  • Delete the owned descendant, even if it appears somewhere else.
  • Delete the owned descendant if it is the last manifestation of the object; otherwise, leave it be. (Effectively this is like a Remove Manifestation that results in deletion, with the user implicitly confirming it)

Using the former definition as that is most consistent with the use cases others have expressed.

@VWoeltjen

This comment has been minimized.

Show comment
Hide comment
@VWoeltjen

VWoeltjen Oct 16, 2013

Contributor

Added user-facing Delete All action in #216.

Comments and related issues mention both UI and API for this. The provided solution meets the UI need, but is probably not very useful from an API perspective (although it is at least a model for how to achieve this result programmatically).

One problem for API is that it's not clear what the rules should be for object deletion at this level (is policy enforced? what should failure behavior be?). In the presence of the delete() method of PersistenceProvider, one can say that the API that is missing is a method (short of manual graph traversal) for assembling a list of components which exhibit a certain property - "descendants that can be deleted", for example. Added a follow-up issue #217 for this cross-cutting concern. In the presence of a good graph querying API, recursive deletion should be simple - persistenceProvider.delete(graphProvider.query(...this component and its descendants...))

Contributor

VWoeltjen commented Oct 16, 2013

Added user-facing Delete All action in #216.

Comments and related issues mention both UI and API for this. The provided solution meets the UI need, but is probably not very useful from an API perspective (although it is at least a model for how to achieve this result programmatically).

One problem for API is that it's not clear what the rules should be for object deletion at this level (is policy enforced? what should failure behavior be?). In the presence of the delete() method of PersistenceProvider, one can say that the API that is missing is a method (short of manual graph traversal) for assembling a list of components which exhibit a certain property - "descendants that can be deleted", for example. Added a follow-up issue #217 for this cross-cutting concern. In the presence of a good graph querying API, recursive deletion should be simple - persistenceProvider.delete(graphProvider.query(...this component and its descendants...))

@VWoeltjen VWoeltjen closed this Oct 16, 2013

@VWoeltjen

This comment has been minimized.

Show comment
Hide comment
@VWoeltjen

VWoeltjen Oct 18, 2013

Contributor

Self-verified on Mac OS X 10.8.5 and Windows 7 10/18/2013

Contributor

VWoeltjen commented Oct 18, 2013

Self-verified on Mac OS X 10.8.5 and Windows 7 10/18/2013

@VWoeltjen

This comment has been minimized.

Show comment
Hide comment
@VWoeltjen

VWoeltjen Jan 2, 2014

Contributor

Verified on Windows 7, hackathon 1/2/2014, Scenario build 177

Contributor

VWoeltjen commented Jan 2, 2014

Verified on Windows 7, hackathon 1/2/2014, Scenario build 177

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment