Bugzilla – Full Text Bug Listing
| Summary: | Why does GlobalDeleteAction specifically ignore removal attempts on multiple selections? | ||
|---|---|---|---|
| Product: | [WebTools] WTP ServerTools | Reporter: | Rob Stryker <stryker> |
| Component: | wst.server | Assignee: | Elson Yuen <eyuen7> |
| Status: | RESOLVED FIXED | QA Contact: | Elson Yuen <eyuen7> |
| Severity: | normal | ||
| Priority: | P3 | CC: | eyuen7, manderse |
| Version: | 3.3 | ||
| Target Milestone: | Future | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| See Also: | https://git.eclipse.org/r/109078 | ||
| Whiteboard: | |||
| Attachments: | |||
Created attachment 195036 [details]
Allows multiple modules under the same server to be removed in one delete action
The attached patch does the following:
1) Adds a new String to the message class and properties file to pluralize "resource"
2) maintains unchanged the usecase behaviour of a single server selection (delete the server)
3) maintains unchanged the usecase of multiple server selection (do nothing)
4) maintains unchanged the usecase of mixed server / module selection (do nothing)
5) maintains unchanged the usecase of child modules being deleted (do nothing)
6) maintains unchanged the usecase of modules under several different servers being deleted (do nothing)
7) CHANGES the usecase of multiple module selection under ONE server object in the following ways:
a) adds a new RemoveModuleAction constructor to handle multiple modules (old constructor is unchanged and backwards compatible)
b) It will still open one dialog before deleting resources, though the message may be singular or plural depending on whether module count is 1 or greater.
c) GlobalDeleteAction has updated logic to ensure the new usecase is handled and previous usecases are not mishandled.
This was tested using exhaustive tracing.
The previous logic was as follows:
1) Get first selected object. If it's a server, set the server variable. If it's a ModuleServer, set both module and server variables
2) If the selection is of size > 1, set the vars to null and do nothing
3) If the server is set and the module is not, delete the server
4) else if the module is not null, delete the module
The logic has been updated as follows:
1) if the selection is size 1 and is an IServer, delete the server, then return
2) Otherwise, get a list of removable modules. If the list is null, do nothing. Otherwise, remove the list of modules
2a) Return empty list in case of any non-ModuleServer selection
2b) Return empty list in case modules under different servers are in the selection
2c) Return empty list if any selected item is a child module
2d) If module fills all criteria, add it to the list.
2e) return the list after iteration and adding.
Hopefully the logic is clear enough that a simple read-through shows it covers all the same use-cases as previously. The number of possible combinations in which this action is called is very small and I believe strongly I've covered all previous use-cases adequately.
Created attachment 195875 [details]
Allows multiple modules under the same server to be removed in one delete action
This patch also modifies the menu so that the action is visible there as well.
Created attachment 195877 [details]
Screenshot of menu vs action
Comment on attachment 195875 [details] Allows multiple modules under the same server to be removed in one delete action >+ if( nextMod == null || nextMod.length > 1 || nextMod[0] == null) >+ // If the module is a child module (ejb / war underneath an ear) ignore this request >+ return null; >+ >+ // Add the item to the list of removable modules >+ moduleList.add(((ModuleServer)next).getModule()[0]); This part of the code seems a suspicious, wouldn't the third evaluation fail with a ArrayOutofBoundException in the case where of an array zero length? Hmm... good call on a coding level, though since this is a UI action, I'm not sure a module of length 0 can ever exist in the UI... but yes, you're right. I'll clean that up for the next patch (In reply to comment #5) > Hmm... good call on a coding level, though since this is a UI action, I'm not > sure a module of length 0 can ever exist in the UI... but yes, you're right. > I'll clean that up for the next patch Not sure it counts in this specific case but in general its possible to call actions/commands via UI (Ctrl+3) which won't do the check first and then you could be called with 0 elements. Just saying ;) Created attachment 197070 [details]
Fixed patch
Fixed hte patch to change > 1 to != 1, so that in the case of an empty collection, it also short-circuits and returns null.
Angel I fixed the patch according to your requests but no further action was taken. Can this be targeted to the maintenance release? Thanks. Any update to someone reviewing this patch? Been another 6 weeks already, 3 months total since patch was attached. And this was whiteboarded for 3.3.1 Why is this draft plan for 3.3.1 and now marked to future and being completely ignored? There's a patch. If there's something wrong with the patch, it'd be nice for someone to tell me. If the current assignee is not responsible, it'd be nice if he re-assigned it to someone willing to review the patch. thanks. Created attachment 207694 [details]
v1.0
Changes looks good. Minor update on original patch with some minor code clean up and copyright update.
Code released to 33M and HEAD Elson if you get a chance, can you just target this bug appropriately? You finished the work but it's still targeted to "Future". New Gerrit change created: https://git.eclipse.org/r/109078 |
Our users have been requesting the ability to select 3 modules to remove from a server, press the delete key, and have them removed. The code in GlobalDeleteAction specifically ignores this in the following block: // avoid no selection or multiple selection if (iterator.hasNext()) { server = null; moduleArray = null; } Why?