Bug 276214 - [target] unable to remove things from target platform
Summary: [target] unable to remove things from target platform
Status: VERIFIED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.5   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 3.6 RC1   Edit
Assignee: Curtis Windatt CLA
QA Contact:
URL:
Whiteboard:
Keywords: polish
Depends on: 287619
Blocks:
  Show dependency tree
 
Reported: 2009-05-13 21:32 EDT by Jeff McAffer CLA
Modified: 2010-05-04 16:00 EDT (History)
3 users (show)

See Also:
darin.eclipse: review+


Attachments
Work in progress (9.19 KB, patch)
2009-07-08 16:44 EDT, Curtis Windatt CLA
no flags Details | Diff
mylyn/context/zip (62.19 KB, application/octet-stream)
2009-07-08 16:44 EDT, Curtis Windatt CLA
no flags Details
Fix (10.11 KB, patch)
2010-05-04 12:34 EDT, Curtis Windatt CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff McAffer CLA 2009-05-13 21:32:12 EDT
using the target platform provisioning with a software site I added some features from a repository.  There does not seem to be a way of then removing the features from the target.  You can deselect them on the contents page or remove the entire site but cannot uninstall the particular features.  I do wonder then how the previsouly installed features can be updated?
Comment 1 Darin Wright CLA 2009-05-13 22:38:09 EDT
(In reply to comment #0)
>  I do
> wonder then how the previsouly installed features can be updated?

Pascal has requested a "check for updates" enhancement for targets - is this what you mean? See bug 274212.

> There does not seem to be a way of then removing
> the features from the target.

Currently you can only add/remove locations to/from targets. Once a location has been added, you can deselect bundles from that location (on the content tab). The target provisioner is not yet an "Admin UI". Each time a location is resolved, it collects and installs required IU's. Uninstalling bits from a provisioned target is not part of the current design (since re-resolving would add them back).
Comment 2 Curtis Windatt CLA 2009-05-14 09:07:41 EDT
For 3.5 to remove the top level IU, you would edit the container and uncheck the IU.  There are known issues with this, as sometimes the correct things are not checked and checked items are not always selected and revealed.  Going forward we may need a more detailed edit dialog allowing you to see what is installed.

In addition, it wouldn't be difficult on the UI side to allow removal of a root IU as requested.  We would just have to recreate the container so things were resolved correctly.

Marking as 3.6 as one way or another we will need better mangement of what is included in an IUBundleContainer.
Comment 3 Jeff McAffer CLA 2009-05-14 10:55:27 EDT
(In reply to comment #2)
> For 3.5 to remove the top level IU, you would edit the container and uncheck
> the IU.  

In the Add workflow the user selects a feature.  in the Locations page there is a list of features but no ability to remove (grayed out).  On the Content page there is a list of bundles with checkboxes.  What does the user uncheck from there that corresponds to the thing that they added via the other workflow?

> There are known issues with this, as sometimes the correct things are
> not checked and checked items are not always selected and revealed.  Going
> forward we may need a more detailed edit dialog allowing you to see what is
> installed.

If something is installed to the profile via add, we should be able to uninstall it from the profile using remove.  Can you rely on p2 to do the heavy lifting?

> In addition, it wouldn't be difficult on the UI side to allow removal of a root
> IU as requested.  We would just have to recreate the container so things were
> resolved correctly.
> 
> Marking as 3.6 as one way or another we will need better mangement of what is
> included in an IUBundleContainer.

for 3.5 we need to clearly set expectations for users around what they can add/remove and how.  In the past people had to manage things manually either by keeping separate locations or groveling in the dirt.  The new support is an order of magnitude better/more powerful for discovery and adding.  Using the site stuff the user shifts to the higher level of features, repos etc and forgets about locations and bundles.  There is a disconnect however in removal.  

Note sure how to smooth this.

Comment 4 Jeff McAffer CLA 2009-05-14 11:02:49 EDT
(In reply to comment #1)
> Pascal has requested a "check for updates" enhancement for targets - is this
> what you mean? See bug 274212.
Indeed it is.  added myself there.  thanks.

> > There does not seem to be a way of then removing
> > the features from the target.
> 
> Currently you can only add/remove locations to/from targets. Once a location
> has been added, you can deselect bundles from that location (on the content
> tab). The target provisioner is not yet an "Admin UI". Each time a location is
> resolved, it collects and installs required IU's. Uninstalling bits from a
> provisioned target is not part of the current design (since re-resolving would
> add them back).
I'm talking here about the top level IUs shown under the locations on the Location page.  These are the things that the user selected and installed.  If you uninstall them from the profile then there should be nothing that will add them back.

In theory the Remove button is already there and the action would be the compliment of the install operation.  I'm not thinking fancy here.

Comment 5 Darin Wright CLA 2009-05-14 11:10:15 EDT
(In reply to comment #4)

> I'm talking here about the top level IUs shown under the locations on the
> Location page.  These are the things that the user selected and installed.  If
> you uninstall them from the profile then there should be nothing that will add
> them back.
> In theory the Remove button is already there and the action would be the
> compliment of the install operation.  I'm not thinking fancy here.

OK, now I understand. Yes, basically we need to improve the UI here. Currently, you have to "Edit..." the location and deselect what you want to remove.
Comment 6 Jeff McAffer CLA 2009-05-14 11:22:30 EDT
cool, I had no idea you could/should do that.  The presence of the Remove button led me to think that that is the way things are removed so did not look for an alternative.  

Note however that you cannot remove all the elements from a location as the Edit dialog says that at least one thing must be selected.  (BTW, why is that?)  Forcing users to remove the location to get rid of the last element is painful as people are often tweaking etc and don't really want to lose the location, just temporarily remove the contents.
Comment 7 Curtis Windatt CLA 2009-06-17 15:55:27 EDT
(In reply to comment #6)
> cool, I had no idea you could/should do that.  The presence of the Remove
> button led me to think that that is the way things are removed so did not look
> for an alternative.  
> 

It is straight forward enough to hook up the remove button to remove the feature.  Even if the UI changes some more in 3.6 it's easy enough to hook this up for M1.

> Note however that you cannot remove all the elements from a location as the
> Edit dialog says that at least one thing must be selected.  (BTW, why is that?)
>  Forcing users to remove the location to get rid of the last element is painful
> as people are often tweaking etc and don't really want to lose the location,
> just temporarily remove the contents.
> 

The dialog prevents you from creating a location with nothing in it.  I don't know whether that is really a problem or not.  Will try it out and see what the UI feels like.

Comment 8 Curtis Windatt CLA 2009-07-08 16:44:23 EDT
Created attachment 141136 [details]
Work in progress

This is more work than I expected.  Getting the dialog to handle cases where nothing is selected is easy enough, but hooking up the remove button is a big hassle.  We need to figure out the container an IU belongs to, then that container must be replaced with a new one, as the iu containers can't handle having their IUs switched.
Comment 9 Curtis Windatt CLA 2009-07-08 16:44:48 EDT
Created attachment 141137 [details]
mylyn/context/zip
Comment 10 Curtis Windatt CLA 2009-08-04 15:05:21 EDT
Not going to change this for the early milestones.  Since the simple fix turned out to not be very simple, I am going to wait to see what other changes we want to make to the UI.  We may end up redoing the wizard to make it easier to view and edit the container (currently the wizard is primarily set up to add IUs).
Comment 11 Curtis Windatt CLA 2010-01-28 13:55:04 EST
Removing milestone as the new profile based targets is not going in for 3.6, which would have fixed this issue.
Comment 12 Curtis Windatt CLA 2010-04-14 13:52:42 EDT
Marking for investigation in RC1.  While the majority of changes from the branch cannot be merged into HEAD, it's possible that we can take some of the work on improve the UI flow.  If we were able to fix this bug, there would be no need to pre-select items in the available IU group, solving Jeff's problems on bug 235654 and bug 272725.
Comment 13 Jeff McAffer CLA 2010-04-14 16:09:41 EDT
I'm not quite sure how changing the preselected set in the Edit Content dialog for a software site would address bug 235654. If you don't preselect all of the current content from the chosen site then the Edit... button is not an Edit any more.  How would I remove something? or how would you distinguish between things I wanted to add, what is already there and stuff that I wanted to remove.

To be clear, my argument is not with the edit workflow but with the data loss that is happening. if you change to not preselect the current content then you have to have an add and remove rather than an edit. Then, in the Remove flow you have to ensure that the filter field does not deselect everything.  The problem is still there.
Comment 14 Curtis Windatt CLA 2010-04-14 16:30:39 EDT
Changing the workflow is a significant amount of work, more work than I keep expecting, so I'm not sure if it would be better to look for a hack to fix 235654 (for PDE usecases anyways).

Changing the workflow fixes bug 235654 by avoiding using an edit wizard.  The available IU group was never intended as a way of managing installed IUs, rather it is a way to install new things.  So if we changed the workflow so that you can add/remove individual IUs, we wouldn't have to prepopulate the available IU group.
Comment 15 Jeff McAffer CLA 2010-04-14 17:02:35 EDT
OK, I understand what you are saying. I was NOT proposing changing the workflow at this point. Honestly I'm not sure what add/remove would look like and it does feel like quite a bit of work. I would be cautious there.  This bug is relatively minor compared to bug 309136 which results in data loss. It would be OK IMHO to just fix the other bug with a hack of some sort and leave the workflow alone at this point.  This can be revisited in 3.7.
Comment 16 Curtis Windatt CLA 2010-05-04 12:34:00 EDT
Created attachment 166985 [details]
Fix

Allows for removing of the individual IUs from the locations tab using the remove button or the delete key.  You can also double click on the IUs to open the edit wizard of the parent container.

If you remove all the IUs from a container, the container itself remains.  I'm not sure if this is the right behaviour.  A user might want to add a different IU in replacement, or they might be trying to remove it completely.  The user always has the option of selecting the container directly and removing it.
Comment 17 Curtis Windatt CLA 2010-05-04 12:44:17 EDT
Darin, please review and apply the patch.
Comment 18 Darin Wright CLA 2010-05-04 15:47:00 EDT
Looks good. One small issue - the count of total bundles in the IU container is not updated when I remove an IU from the container.
Comment 19 Curtis Windatt CLA 2010-05-04 15:54:05 EDT
(In reply to comment #18)
> Looks good. One small issue - the count of total bundles in the IU container is
> not updated when I remove an IU from the container.

In the handleRemoveMethod, line 366, the tree refresh should use true as its argument to refresh the labels.  Normally during a remove the entire container is removed so there was no need to refresh the labels.
Comment 20 Darin Wright CLA 2010-05-04 15:59:29 EDT
Yup, that does it. Fixed/Applied.
Comment 21 Darin Wright CLA 2010-05-04 16:00:13 EDT
Verified