Bugzilla – Full Text Bug Listing
| Summary: | [ui] Enable site when user attempt to add a disabled site | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Ryan <rbrueske> | ||||
| Component: | p2 | Assignee: | Susan McCourt <susan> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | enhancement | ||||||
| Priority: | P3 | CC: | david_williams, eclipse-bugs, georg.sendt, jakub.jurkiewicz, john.arthorne, matt, mik.kersten, Mike_Wilson, mlists, pascal, philippe_mulet, preuss, shawn.minto, steffen.pingel, susan | ||||
| Version: | 3.4 | Flags: | susan:
pmc_approved?
(philippe_mulet) susan: review? (john.arthorne) |
||||
| Target Milestone: | 3.4.2 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Ryan
You might already have that site listed but it is disabled. Check the Manage Sites dialog and see if it is there but disabled (no checkmark). Adding this site works for me in RC3. Thanks, I didn't know that the site was already in there. It would have been helpful to get a message stating that the site was not added as it exists and can be enabled by clicking on Manage Sites. Reopening as an enhancement. I agree. A reasonable expectation is that the site become enabled at this point, or else it shouldn't let the add proceed because the site is already known. *** Bug 238619 has been marked as a duplicate of this bug. *** *** Bug 239863 has been marked as a duplicate of this bug. *** I think the right place to fix this in 3.5 is for the repository manager addRepository(...) method to enable a repository if someone calls add, and there is already a disabled repository known to the manager. This has the invariant that a repository is always present and enabled after returning from the addRepository call. This will fix the UI case, but also programmatic clients who add a repository and then proceed with the assumption that it is present and enabled. To make this work, we would need for a RepositoryEvent of type CHANGED to be fired so that the UI knows to make that repository visibible in its views. Having said that, I think this is a bit complex for a 3.4.1 fix. I think it's still worth doing something in 3.4.1 though, because this is a common problem for users and leads to very unintuitive behaviour. Susan, can you recommend a place in the UI code for a strategic 3.4.1 fix? I.e., so that when the user goes to add a repository, we would ensure it is enabled at the same time. Perhaps in AddColocatedRepositoryOperation? Moving to Susan for a 3.4.1 fix at the UI level. I have opened bug 241307 to address this more broadly in 3.5 stream. *** Bug 241226 has been marked as a duplicate of this bug. *** yes, I was expecting to fix in the 3.4.1 stream on the UI side. A similar problem occurs when an site links associate sites that already exist but are disabled: The linked sites are not enabled and dependencies from those sites are not resolved. Should I open a separate bug or will this be considered as part of fixing this bug? >A similar problem occurs when an site links associate sites that already exist >but are disabled: The linked sites are not enabled and dependencies from those >sites are not resolved. It can be considered part of this bug for the case where a site defines an enabled associate site and it does not get enabled because it has already been added as a disabled site. The more general issue that associate sites are being added as disabled sites in the first place and that users may not know they are there is tracked in bug #234213 Fixed in R3_4 maintenance stream >20080731. The fix is not ideal, but is surgical and will result in a better user experience. I agree with John that this should get cleaned up at the repository manager level. Changes to AddColocatedRepositoryOperation - retrieve a list of the disabled artifact and disabled metadata repos - before adding a repo, check to see if it is disabled - if it is disabled, set it to be enabled, otherwise add it as done previously - note (HACK) - I had to publish a repository add event when I enable the site so that the UI can update in the same way it does as when the user adds a site. There is no repository change event sent by the repo managers when a repo is enabled. Since I would have had to generate an event anyway from the UI, I chose to generate the add event rather than the change event. This causes the UI to behave the same way as if the user added the site and requires no new CHANGE handling in the UI code. I checked listeners of repository ADD events throughout p2 to ensure this didn't break any assumptions. The correct approach in the future would be to publish a CHANGE event as John mentions in comment #7. From comment #10 >A similar problem occurs when an site links associate sites that already exist >but are disabled: The linked sites are not enabled and dependencies from those >sites are not resolved. The fix here does not solve this problem because I only enable the site in the scenario where the user has added the site (via a dialog or drag/drop). The case where the site is added by reference from another site must be fixed at the repo manager level. That will be fixed when bug #241307 is fixed. Reopening. It appears that one of the ui bundles was not tagged/released when the last batch of M-stream ui fixes were completed on 7/31. That means this fix is in the maintenance branch but is not actually in the M-builds. This fix is quite important for R3.4.1 This bug was believed to be fixed for 3.4.1 long ago and was not tagged properly to get into the builds. I just discovered today while re-checking some bugs and after observing some odd behavior. I propose we fix this for 3.4.1, as this was one of the more confusing bugs hitting users when 3.4 was released. IMPACT: Note the number of duplicate bugs reported on this issue. It is a very annoying and confusing problem when the user tries to add a site and it does not appear to work. No error is reported and nothing appears in the log to help diagnose the problem. Because the presence of the disabled sites is not necessarily known to the user, it is not clear how to solve the problem. RISK: the risk is low. The code has been in the M-stream since 7/31 and I have used it in self-hosting environments since then. The code affects one class and the fix is surgical. Rather than automatically add the specified metadata repository and artifact repository locations, the new code checks to see if the locations are known as disabled locations, and if they are, then the repository is enabled rather than added. We use the same event (ADD) to signal the UI that the repository has been added, so the UI update code path is the same once the repository has been enabled. Adding John for review of the change (see AddColocatedRepositoryOperation) and Philippe for approval of the fix. The fix is already in the maintenance branch. If approved, this fix needs to be tagged and added to the map file. I should add that the workaround for this problem is: - open the "manage sites..." dialog - locate the repository you were trying to add and enable it Note that most users don't know the site has already been added (because often it was done so programatically by the the p2 update site metadata generator). So this workaround is not obvious at all to the end user. +1 to release now I'm not convinced we should release this at this point. This bug is an annoyance, but I wouldn't call it a show-stopper bug, which is all we're supposed to be fixing for RC3. The other reason against it in my books is that we didn't release this fix in the 3.5 stream, so we don't have much validation that this doesn't cause other regressions. Firing an ADDED event in this case isn't technically correct, but I suppose this is needed to make the UI react to the enablement? Given concerns, why not rescheduling it for 3.4.2 ? RC3 feels a bit late to try things. Agreed for 3.4.2. In the waiting of a more appropriate fix for 3.5, I think it would be good to release this one and revise later, this way we get at least some testing in for potential inclusion in 3.4.2 Post 3.4.1, why not. But you need to be ready to back it out later on, if needed... As a member of the community and someone who answers lots of questions for users, I have to disagree strongly about postponing the release of this. Let me be very clear about this: p2 already enjoys a dubious reputation among the user community. I have been reading and participating in a number of discussions where people are asking, "when will p2 be replaced" and questions like that. My point is that this particular bug looks, to the user, like a very blatant problem in p2. We should not assume that because the "insiders" know a simple work-around that it means this is not a major blemish on the tool. Susan has already pointed out that the fix is quite surgical and only touches one class in the UI; her assessment of the risk is LOW. We have an opportunity to correct something that many people encounter and in the process help to improve the (undeserved but real) reputation of p2. (In reply to comment #21) > ... p2 already enjoys a dubious reputation among the user community... Eric, I understand the point you are making. However, we all need to realize that the odds of this particular fix being the one that changes the minds of those who don't like p2 is correspondingly LOW. There are *plenty* of people who see the promise of p2, understand the path we are on, and realize that compromising good software engineering principles is not the way to make progress. Unless we believe that this is actually the fix (or at least has the shape that we believe the real fix should have), it does not make sense to release it now. Let's not add the potential for people to build patterns that depends on this fix, which might then be broken later, to the already volatile mix. Even if we do believe this is the right direction, John's comment #17 effectively means that we *must* wait for 3.4.2. There is simply no way that we could test it sufficiently for 3.4.1 now. changing milestone to 3.4.2. Eric, I understand your frustration. I was hoping to get it in because it was probably the biggest usability hit of all my 3.4.1 bugs. I'm pretty bummed/embarrassed that a tag error on my part prevented this from getting exercised much earlier and that I just now caught it. As far as 3.4.2 goes, this is the correct fix in my opinion in the absence of a more general fix in core. >Firing an ADDED event in this case isn't technically correct, but I suppose >this is needed to make the UI react to the enablement? see comment #12. I guess the decision has been made, but I still wanted to reply to Comment 22 because I think it misunderstand both the intended nature of the fix and my objection to not shipping it in 3.4.1. The fix that Susan did, if I understand correctly, is a UI workaround to a limitation in the p2 core. Again, from what I gather by reading this discussion, it does not seem like something that anyone would "build patterns that depend on" (I'm not really sure what that means, so perhaps I'm wrong on that...). As to this fix not by itself changing people's minds about p2, well nobody suggested that this was THE big downfall of p2. This is just one usability problem that a lot of users bump into. My argument is that because we have a very small fix that can eliminate it, it should be addressed sooner rather than later. Believe me, I completely understand the principles of thorough testing and confidence. However, nothing is black and white and there are always trade-offs to be made. In this case, it seemed to me that the policy of "only showstopper bugs in RCs" needed some re-consideration in this particular case because of the high visibility of this problem and the fact that the change was "surgical." I just wanted to balance the discussion with the user's point of view. Created attachment 112121 [details]
patch to AddColocatedRepositoryOperation
Backed out the change for this bug from the maintenance stream so that I could release other approved changes in org.eclipse.equinox.p2.ui
This patch will reapply the fix and will be applied in the maintenance stream after 3.4.1
fixed >20080929. released the fix back into the maintenance branch and tagged the project/updated map files. I will verify this bug on the next M-build and again when the 3.4.2 test pass starts. verified that this fix is in Build id: M20081127-1656 *** Bug 260471 has been marked as a duplicate of this bug. *** I just discovered another scenario that is impacted by the underlying p2 issue and would like to know if this fix will address this new scenario. Let's say an update site is using an "associate sites" file, and that one of the sites listed in the associates is already in the users list of sites but is currently un-checked. It appears that p2 does not find that site. If the user manually goes into Manage Sites and enables the site, then p2 can resolve its features OK. So it appears that disabled sites that are referenced by associate files are also silently ignored. Because of this and the fact that the p2 dependency error messages are next to useless, update site developers face a daunting task trying to debug why their features won't install. Will this fix for 3.4.2 cover this scenario? Unfortunately, the case you mention is not covered by the UI fix in the 3.4.2 stream. (this is one of the programmatic cases John refers to in comment #7). The core-level fix appears in the 3.5 stream per bug 241307 verified in Build id: M20090121-1700 |