Community
Participate
Working Groups
When using the run on server wizard to move Ear1 to the server, I can also move additional items from the left column to the right. After adding ear2 to the right, and selecting it, it cannot be 'removed' to the left. The enablement for "remove all" remains true, but for the singular 'remove' is incorrectly false. Patch to be attached.
I'd like to see this small patch in wtp 3.2.3
Created attachment 184029 [details] Adds an else to set enablement to true The patch above fixes this issue. The issue is only replicatable during the run on server wizard and not during the add / remove modules wizard, because the run on server wizard sets requiredModules to some value, but the add / remove wizard does not. What's basically happening here is an additional check is being done if requiredModules is not null, it then checks if its equal to a required module. If it is equal to the required module, the enablement must be false. However, if required modules has some value but it is NOT equal, it is not being set to true as it should be.
- Does remove all removes all the modules? - Does your changes affect the remove action for the modules that is being published(aka. the selected project)?
Remove-all's behaviour has not changed. It continues, as before, to remove all modules except hte one selected to "run on server". Basically, my patch only fixes the issue and does not break any other behaviour.
ping on the bug ;)
Ran out of time due to other commitments. I wanted to run through some testing of this one as we have had problems in this area in the past, re-targeting
(In reply to comment #4) Rob I wanted to drop this change today, but I might not have time to test and address the question from comment #3 > - Does your changes affect the remove action for the modules that is being published(aka. the selected project) If you can answer the question, I might still be able to drop it tonight
Sorry for the delay Angel: After the patch, if you "Run on server" the project "DynamicWeb2", it will open the wizard, and "DynamicWeb2" will be on the right side. Even after the patch, "DynamicWeb2" cannot be "removed". That use-case is unaffected. The use-case that *is* affected is if you have two projects, DynamicWeb1 and DynamicWeb2, and you "Run on Server" the project "DynamicWeb1". At this point, DynamicWeb1 is on the right side of the wizard and cannot be removed. If you THEN move DynamicWeb2 to the right side, and try to "remove" it back to the left, pre-patch, remove is disabled; post-patch remove is enabled. But, as I said, the behaviour of the module associated with the original project selected to "Run on Server" is *UNCHANGED*.
I would just like to note it seems absolutely ridiculous to me that a 3-line patch, which I have assured you was tested, is not only no longer targeted to the maintenance release, but *ALSO* is not targeted to the 3.3 stream! Targeting this to "future" is kinda hard for me to tactfully accept, and we all know I've never been one for tact. This is a 100% simple UI fix consisting of an else and a variable set. It is more an overlooked else than anything. The bug has sat with a patch on it for five months. If you genuinely didn't have the time to properly test it, you could / should have asked us to have another community member verify the patch worked before we add it into the build, or any number of other possible solutions to the issue. Ignoring a 3 line patch for 5 months is pretty nuts.
Rob, I hear your concern. There is a problem here for bugs with patch attached that is not specific to you. As a component lead, I very inquisitive about patches. Perhaps I am being too rigorous and that is why I want to create a page/process that will contain a template of what I expect to see when a patch is contributed (patch format, explanation of change, testing coverage) I do not want to be the bottle neck (which I currently am) for patches but I also have to ensure that the patches do not regress functions thus I expect a good testing coverage. As you know, sometimes a simple false to true of a variable is a simple one line change, but it may have significant impact on the code. Server tools is the component that sits in the middle of WTP thus it is delicate when something breaks or change in behaviour. As you have seen in the bugs where you have submitted patches I tend to always ask for what kind of testing has been done. I ask for this information in a bullet form so that is to the point and easy to read. This helps assure that aside from the unit testing that you might have done, there was also a deep analysis on the change, and how the code change affects its callers. As a rule of thumb I tend to always do a reference on the method where the change is occurring and see which scenarios could be affected. In addition to look through the code, I also use my experience in how could the change affect the usage and test those scenarios as well. In terms of target, Server tools didn't use specific milestone targets for 3.3 this time around, we only have a 3.3 target. So for lack of a target I moved this defect to future, but I am still considering it for 3.3. I just don't want to commit to it yet until I am advice of the time that I have to spend on WTP. Since you added your response I didn't have time to sit down and read the response in detailed, and understand if the testing that you did was sufficient.
With all due respect, I do think you're being overly careful, or perhaps you are just too busy (not your fault) to designate the time to accepting and reviewing patches. The fact is this is a very small block of code with predominately locally-scoped variables. The two lines in question are inside a 30-line "else" block. 6 of these lines are comments. Four are closing brackets. 5 are either boolean-setting or break. There simply isn't a lot of logic here at all. The "enabled" boolean has a scope only relevant to those 30 lines inside an else statement. Outside of those 30 lines, the boolean doesn't even exist. The intent of those 30 lines, clearly outlined in comments, is to discover if the "remove" button should be enabled. That is the only functionality of the 30 lines. There is no other functions mixed in. Analysing the 30 lines shows clearly that it is iterating through a selection of modules (usually one, but users may click several). If it finds *ANY* case where the selection cannot be removed, it sets the enabled boolean to false, and breaks the iteration loop (effectively setting the "remove" button to disabled.) Since the loop is setting the flag to false any time it finds an unacceptable situation, the 30 lines could be just as easily fixed by changing the boolean's initial value to true and deleting the 'else' on line 795 and 796. Instead, my initial patch adds an 'else' where there is an unremovable-module but the selection is NOT that module, to force enablement to true. The real issue here is that you don't seem to have enough time to analyze even simple patches in very isolated blocks of code. In fact, and this is of course not meant in any way against you, it would seem to me that you never even opened the class to see HOW isolated the code around the patch was. If you had opened it, with the amount of comments already in the code, it would have been almost immediately obvious what was occurring. It was to me, at least. And since you're maintainer of the code, I would expect your familiarity with the code to at least rival mine, and most likely surpass by a huge margin. The first problem is that you're requiring exhaustive documentation for extremely isolated code, but the second is that it seems you're not even really analysing the situation to see what the potential for harm is. Obviously with community involvement there will, on occasion, be bugs. This is 100% obvious. If you are going for a zero-bug strategy, you can lock down the codebase 100% right now and have no contributors. You asked me questions, specifically about the behavior of the "remove all" button, but even just a GLANCE at the code in the surrounding block would have shown you that the "removeAll" button is not mentioned ONCE anywhere in the block. It was a completely irrelevant question, and the only way someone would ask that is if they never even bothered to open the class to look at the code block. If you had instead given the code a quick look, and had me promise I scoured the patch for all other possible situations, or required another user to attest that they have tried the patch and it worked and nothing else was broken, the patch could have made it into a build 5 months ago, and could have achieved much wider testing from many many more people. I can't blame you personally as we all have deadlines and work to do, and we all try to do what we think is right for the codebase we are protecting, but it is extremely unfortunate that now, this patch will require even more rigorous testing because it is being added in so late in the release cycle. As I've already given huge analysis (exhaustive, really) to what is 30 lines of very uncomplicated enablement logic, I'm really not sure what more I can do. I'm quite sure there's nothing I *can* do, until you open the class and look at the code.
Angel: I guess I can sum up my previous semi-rant by saying that if you do not want to be the bottleneck, then you need to start trusting your contributors more. You are currently the bottleneck as you're one of the only people with commit rights to the project. We physically cannot commit anything, and so we must push our patches to you. You are the bottleneck but it's your by your own design. If you don't want to be, you need to loosen up your grasp a bit and let patches flow through you, based on the good-faith assurances of your users and contributors.
Created attachment 195034 [details] Initialize removed enablement to false if selection is empty, true otherwise This is a second patch that achieves the exact same thing. I leave it to you to decide which of the two patches you prefer for coding style.
Comment on attachment 195034 [details] Initialize removed enablement to false if selection is empty, true otherwise I will take the first patch.
Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. This defect has been requested to be fixed by an adopter. Is there a work-around? If so, why do you believe the work-around is insufficient? The user can use remove all, but it would be better to have the buttons correctly enabled How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? Rob tested the patch locally Give a brief technical overview. Who has reviewed this fix? Angel What is the risk associated with this fix? low
changes committed to HEAD(3.3)
changes released to HEAD(3.3)
fixed, if a new streams open for 3.2M we should dup this one for the 3.2M stream
New Gerrit change created: https://git.eclipse.org/r/109046
