Bugzilla – Full Text Bug Listing
| Summary: | [Markers] Trying to implement WorkbenchMarkerResolution | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Benno Baumgartner <benno.baumgartner> | ||||
| Component: | UI | Assignee: | Tod Creasey <Tod_Creasey> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P2 | CC: | martinae, Tod_Creasey | ||||
| Version: | 3.2 | ||||||
| Target Milestone: | 3.2 M5 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Benno Baumgartner
The UI already exists. Select Quick Fix and you will see the wizard. The idea is that we will group all of the marker resolutions that satisfy canBeGroupedWith together on the same page so that the user can select one and apply it to many of them. Do you need the marker again? I can be flexible about what we do but wouldn't you check marker.exists() before you try to update? I don't think it would save you anything. We thought about doing multiple problems in the problems view but it turns out that the use case people we most after was fixing all of the same type of problem. Hunting and pecking through the view was not that useful to them. We also wanted to avoid multiple pages because it is much more complicated for both sides (us providing the UI and you for the resolutions). If we want to do more than a simple "fix all of the same type" we are looking at doing a proper refactoring which way outside of what we are trying to solve. Imagine this case You have 3 pages for 3 types of resolution. You select them all on all 3 and hit done. I apply the first page but then all of the ones on the second page are now obsolete so I have to ask again. It is really wierd for a wizard to fail constantly so we dropped the wizard idea altogether. The Back case is even worse. You have applied them, failed on the second set and now go back to the first which also are now obsolete. My simple test case looks like this:
class E {
String s1= "";
String s2= "";
}
With warning enabled for non externalized strings.
This results in two markers and 3 solutions for each marker:
- Add non nls tag
- Open externalize string wizard
- Add @SuppressWarnings nls to E
Add non nls tag can be applied in batch and will not generate any conflict.
The wizard can't be applied in batch mode at all.
@SuppressWarnings can be applied in batch but after the first problem at s1 is solved the problem at s2 is obsolete.
So I expect when I open your wizard and then select add matching problems that the two problems are shown, with two possible solutions to select from:
- Add tag
- add warning
But in fact there are no other matching problems. Is this a bug in the wizard?
I understand it like this: The wizard should group the same problems. This problems may have multiple possible solutions, some of these solutions can be applied in batch mode, some not. The wizard should only show the solutions which can be applied in batch. The user then selects one of this solutions to resolve the problems.
Let us assume both problems whould be shown in the wizard and I select add warning as resolution. The first problem would be fixed making the second obsolete. I can then check if there is still a problem at the marker position when I get a call to getUpdatedResolution, but a check on marker.exists() whould be faster IMHO.
What has to be noted is that if I choose 'add non-nls tag' and the first one is applied this triggers a save and a rebuild resulting in all markers in the file to be removed and new markers created. That means that marker that the second proposal knew doesn't exist anymore. No - a limitation. The dialog checks that all of the resolutions can be grouped - if any of them can't it doesn't get added to the markers of the dialog. If you could attach a patch with your code here that would be very helpful just so I can make sure it isn't a bug you are seeing. Don't forget they get applied one by one - there is no "batch mode" per se as they need to be updated after each individual update. Your use case below is the reason why I do it one by one. I assume some problems will be obsolete which is why I call getUpdatedResolution each time. > If you could attach a patch with your code here that would be > very helpful just so I can make sure it isn't a bug you are seeing. See Bug 116848 for an implementation of WorkbenchMarkerResolution (CorrectionMarkerResolutionGenerator) > No - a limitation. The dialog checks that all of the resolutions can > be grouped - if any of them can't it doesn't get added to the markers > of the dialog. I see, why is that? I would show all the resolutions which can be grouped. > What has to be noted is that if I choose 'add non-nls tag' and the first > one is applied this triggers a save and a rebuild resulting in all markers > in the file to be removed and new markers created. That means that > marker that the second proposal knew doesn't exist anymore. Mmmm, ok, but then I need the new marker when getUpdatedResolution is called. Or if the marker does not exist anymore getUpdatedResolution should not be called at all (or with marker== null or marker.exist()== false). When I get a call to getUpdatedResolution I use the old marker to calculate the new resolution. This only works (and only sometimes) because it takes a while until the old marker is not valid anymore. If I don't get the new marker I don't know where the problem is located and if there is a problem at all. OK that makes sense. Calling marker.exists() beforehand is certainly a good way to reduce issues. The main issue I see is that JDT core throws away markers in the same file and recreates them on a build. A build will occur if your marker saves the file. >I would show all the resolutions which can be grouped. I still want the user to be able to keep thier selection even if it can't be. I suppose I could make a special case for those that cannot be grouped. >Don't forget that these are applied one by one - are they really not able to be >grouped in your use case? i.e. should you close the dialog, resave everything >and reopen to apply the second one? >When I get a >call to getUpdatedResolution I use the old marker to calculate the new >resolution. This only works (and only sometimes) because it takes a while until >the old marker is not valid anymore. If I don't get the new marker I don't know >where the problem is located and if there is a problem at all. Currently you don't do this so we won't lose the marker but the line number will be wrong (if it is in the same file). One of my goals in this is to prevent an issue where the user can never finish because thier selected markers keep disappearing and reappearing. I would just as soon throw out a resolution where marker.exists() returns false and let them do it again rather than trapping them in a dialog. The easy answer for you is to check if the resolution I am sending has a valid line number any more and return null if not. This would mean that you couldn't fix two problems in the same file however. Beno could you give me a scenario (a little test plugin would be awesome) where I could get a set of resolutions that can group? Grouping is possible in following scenario:
package test1;
public class E1 {
public String s1; //$NON-NLS-1$
public String s2; //$NON-NLS-1$
public String s3; //$NON-NLS-1$
}
With warning for non nls enabled. This works sometimes and sometimes it does not work for s3 (on my machine): s1 is fixed, save, s2 is fixed, save, reconcile, marker for s3 is not longer valid, s3 is fixed-> can't be fixed.
This entry summarizes discussion we had per mail:
A fundamental problem is that fixing a marker will invalidate second marker as e.g. a rebuild removes and creates all markers in the same file.
A simplified approach would be to say that we only fix multiple markers if we can do this with a single fix. That means a marker resoultion processor would get all markers selected and will only provide resolutions that can fix all of them.
This will avoid that resolutions holding old information have to run after changes have been made.
A suggestion for such an API would be:
IMultiMarkerResolutionGenerator
IMultiMarkerResolution[] getResolutions(IMarker[] markers)
IMultiMarkerResolution
public void run(IMarker[] markers);
...
To avoid that the user has to make a complicated multi-element-selection, Tod siggestion is to add functionality to ask for more markers to be multi fixed:
IMultiMarkerResolutionGenerator
getSimilarMarkers(IMarker[]) : IMarker[]
This API would be called by the UI to get more markers that could be fixed with the current fix.
We make this change early M5. I have added WorkbenchMarkerResolution# findOtherMarkers(IMarker[] markers) and have reworked the MarkerResolutionDialog to support this new API. The old API is deprecated and will be deleted before the end of M5 (I'll wait for the go ahead from JDT as you are currently the only implementors). Hi Tod I have implemented WorkbenchMarkerResolution# findOtherMarkers(IMarker[] markers) This works fine. But you can't call run(IMarker) on the resolution for every marker returned by findOtherMarkers because this will not solve the problem we've discussed. Instead, call run(IMarker[]) with a subset of IMarkers returned by findOtherMarkers. I will add a patch to show you what I mean. I've allready implemented run(IMarker[]). You can try it when you apply the patch, it's quite nice. All problems which can be solved by the clean up action can also be solved by Multi Quick Fix (i.e. add missing NON-NLS tags). Have fun. Created attachment 32990 [details]
proposed api
The API is fine. I'll move my implementation in the dialog to WorkbenchMarkerResolution and you guys can override. BTW your patch has a ClassCastException for IMarkerResolutions that are not WorkbenchMarkerResolutions so I fixed that too. Updated patch released for build >20060113 Hi Tod, I was thinking (yes, I know, I couldn't help myself:-) You could also let the user multi quick fix problems when he multi selects markers in the problem view and this markers are all element of findOtherMarkers(markers). Then you could enable 'Quick Fix' in the context menu, and when selected by the user resolve the markers. Even without showing the dialog if only one fix is possible. What do you think? You could also disable the button 'Add Matchin Problems' when you do not have a WorkbencheMarkerResolution. >You could also disable the button 'Add Matchin Problems' when you do not have a
>WorkbencheMarkerResolution.
This is already how it works.
Doing it from multi select is not a bad idea - feel free to open an enhancement if you like.
See Bug 124614 Verified in 20060113-1200 |