Community
Participate
Working Groups
Some sorts of fixes (like replace == with ===) could be fixed for an entire file instead of one at a time. This would be a real help when getting code that is full of warnings and quickly fixing it to be warning free.
Eric, what do you think of this one?
Another workflow would be to enhance the problems list view to apply quickfixes to groups of problems at once. Either by sorting by problem type and apply the same quickfix or by selecting a subset of problems and applying the first quick fix available (or prompt user).
Agree, however, many people don't even know the problems view exists and might want to fix it a file at a time. The "quick fix apply all" should be in both places. This is a small item that could add a lot of value (like Eric's check box on the search). Does it always make sense to have the check box there for every quick fix? My thinking is "yes" but some fixes will require changing every place in the file ("===") and other ("ESLint") will require only one change. ASIDE: Why do I have to click in order to get the Quick fix? When would I ever just want the hover? (I can enter a bug for this if it makes sense)
(In reply to Steve Northover from comment #3) > ASIDE: Why do I have to click in order to get the Quick fix? When would I > ever just want the hover? (I can enter a bug for this if it makes sense) Are you talking about hovering in the ruler? The issue brought up in a UX call when quickfixes were first added was that the hovers for ruler annotations were too cluttered if you had multiple annotations. In addition to validation annotations, you can have search results, bookmarks, occurrences, matching bracket, and others in the ruler. That being said I made some improvement to which of those annotations contributed to hover. If this just boils down to "If you have only one annotation with hover on the line, show quick fixes", then yes, file the bug and assign it to me.
I think this makes sense because people might no know to click ("click for quick fix"). Let's explore this idea in another bug report (see bug 475943).
Curtis, I've assigned this to you and I think it would be great if we could get this done. Do you have time or is Eric or someone else a better choice?
(In reply to Steve Northover from comment #6) > Curtis, I've assigned this to you and I think it would be great if we could > get this done. Do you have time or is Eric or someone else a better choice? Leave it with me. We'll see how things shake out with the existing priority items.
There was some question on the call this week as to whether the 'fix all' needs a preview or not. My take is 'no' because the presumption is that all quickfixes are safe (i.e. we'd get shot if we start offering quick fixes that corrupt the code). YMMV....the question was raised that (for example) fixing all NON-NLS$ strings may cause a translatable string to be incorrectly declared as not needing translation...
(In reply to Eric Moffatt from comment #8) > There was some question on the call this week as to whether the 'fix all' > needs a preview or not. My take is 'no' because the presumption is that all > quickfixes are safe (i.e. we'd get shot if we start offering quick fixes > that corrupt the code). I agree it should not need a preview. Perhaps the choice if a fix 'applies to all' can be left to the provider of the fix, that way, for example, in the JS tools I could say the fix for '==' can be applied to all, but the fix to non-nls is not.
As described to Eric, I am looking for a single check box with the text 'Apply to all' or 'Fix all'. For errors/warnings that are fixed by a directive, then already fix all when the directive is applied. Not sure we want a special case for these or not (ie. you can check the box or not, all will be fixed when the directive is added)
This one is really important to me because it is just so obvious (ie. "please fix all == to be === and don't bug me again").
I have a hacked together example of this working. It renders the quickfix command twice, the second applying the fix to all annotations with the same id. I only did the implementation of no-extra-semi fix as it can be applied repeatedly. Here is a list of the biggest TODOs: 1) A better way to get all annotations The tooltip knows what annotation it is anchored to. To get all the annotations in the file I use _annotationModel. I imagine there is a safer way for us to get all the annotations. 2) Check if a quickfix can apply to all The quickfix commands need a way to say 'yes I can fix multiple annotations'. Further down the line we might have quickfixes that only apply to groups of annotations, not singles. 3) Rendering the fix all command For now I'm just rendering the command twice. Once (2) is done we can know when to render it. We could do a second button, or a link that only shows up on hover. 4) Implement quickfixes The JS quick fixes assume they apply to only one annotation so they will have to be rewritten to run on single or multiple. 5) Merge multiple text edits into single step Even if the quick fix applies iteratively on each annotation, we want the text edits to be done in a single action that can be undone in on step.
(In reply to Curtis Windatt from comment #12) > 1) A better way to get all annotations I now optionally pass the editor into tooltip so we can lookup the annotation model. > 2) Check if a quickfix can apply to all Contributors must add a second command in the scope orion.edit.quickfixAll to have the action rendered. This doubles the number of commands, but we can reuse the JS quickfixComputer and I don't see another way to alter the title of the command. > 3) Rendering the fix all command Separate commands can have their own names, I also added support to replace ${0} with the annotation count. > 4) Implement quickfixes TODO > 5) Merge multiple text edits into single step Done, by providing arrays of selections to setText(). Unfortunately there are issues a) You must set the selection afterwards which is difficult because the annotation offsets will be quite different. b) Any modified line will be marked as a current line (this helps the user see what changed though).
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=d1f6911fd53b20befac318f134fcc9755c853c0e Applied the first set of changes to master
Is there anything I can try? I don't seem to see the option in the UI anywhere.
(In reply to Steve Northover from comment #16) > Is there anything I can try? I don't seem to see the option in the UI > anywhere. If you have a file with multiple extra semicolons: var a = 3;; var b = 4;; It should show up as an option. That is the only quickfix with fix all implemented.
Sorry, I was trying "== to ===". That is another one that you might just want to fix (although you should really look at the code).
Is it easy enough to enable that one too or is it a bunch of work? We should have a list of quick fixes that should have multiple options. Where can I find the list of quick fixes (in code maybe?)
(In reply to Steve Northover from comment #19) > Is it easy enough to enable that one too or is it a bunch of work? We > should have a list of quick fixes that should have multiple options. Where > can I find the list of quick fixes (in code maybe?) You can look at javascriptPlugin.js and look for commands in the scope 'orion.edit.quickfixAll'. Currently there is only the one. Any fix that can be applied iteratively and independently are easy to do. I just don't want to spend too much time writing fixes until we are pretty sure this is the architecture we want to go with. Plus I still have to figure out how to track the final selection offset, meaning the quickfixes may have to track that information themselves.
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=35de8d0a5e1b1b9d6af8a8f2cb51b53b0e52cecf Added quick fix all for == and != (eqeqeq)
Thanks. Can you dark launch what you have while we figure out next steps? It works and is cool but it looks a bit strange in the UI. Alternately, I know I am check box obsessed but if you could turn it into the old text with a check box (if this was easy). Might be better to spend the time making quick fixes support multiple annotations as discussed with SSQ today. SSQ?
Remember that by default, everything we do is pushed to Bluemix PROD twice a week.
I think adding another scopeId for quick fix all commands is over kill. We should just add a property to the command that tells whether the command can fix all instances of a problem. Something like (i.e. fixAllEnabled): provider.registerServiceProvider("orion.edit.command", quickFixComputer, { name: javascriptMessages["removeExtraSemiFixName"], scopeId: "orion.edit.quickfix", id : "rm.extra.semi.fix", contentType: ['application/javascript', 'text/html'], fixAllEnabled: true, validationProperties: [ {source: "annotation:id", match: "^(?:no-extra-semi)$"} ] } ); And command registry renders that quick fix command accordingly. The simplest way is to render a checkbox to the right of the command which says "Fix all 10 issues". Or something similar.
Maybe fixAllEnabled should really be "allowMultipleFixes" for when we add UI to let the user selectively choose the problems to fix.
I'm not seeing a reason to hide this functionality under dark launch. It works and the UI will only improve over time. The functionality won't disappear. Silenio, do you have time to look at the setText with a selection/caret location issue? I can put something together there, but I'm hesitant to make changes in text view without you around to review them. I'll work on modifying the command registry to support single commands with a checkbox or similar generic UI.
> I'm not seeing a reason to hide this functionality under dark launch. It "looks ugly" and customers are running Bluemix DevOps in the field?
As discussed on the phone, there is no burning requirement to dark launch this feature. Although this is what I would prefer, the feature is working etc. so I am ok if we leave it with the current UI for a short time period.
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=9397c045ffade71c9936f2eab3b947543a0b37a8 As Silenio won't have time to fix the selection issue in TextView, I created a workaround from his suggestions. Also fixed layout issues and a potential ordering problem.
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=dbb039658ace937dbbe699163efe395f235e098c Implements the single command structure with a checkbox to enable quickfix all. TODO: - Code cleanup, JSDoc, NLS (are we happy with 'Fix all in file') - I reverted to getting the model out of the annotation as it was easier, but we can pass the full annotation list to extension commands if we want to avoid using internals. Either way will require some code cleanup. - Figure out the best implementation for JS quickfixes (currently fix and fixAll are two separate functions). - Add more quickfix all and tests.
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=154df8d5a8fb12958b51daa90de1a295a0c9ffa1 Allows JS quick fixes to easily apply to all problems of that type in the file.
> Code cleanup, JSDoc, NLS (are we happy with 'Fix all in file') How about right aligning the check box and changing the text to 'All' or 'Fix all'? (x) Fix all
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=a279b891504d9fc6417538c1fe597a934b88ec1a Changes text to 'Fix all' NLS the text Fix vertical alignment (In reply to Steve Northover from comment #32) > > Code cleanup, JSDoc, NLS (are we happy with 'Fix all in file') > > How about right aligning the check box and changing the text to 'All' or > 'Fix all'? > > (x) Fix all Right aligning the checkbox looks awful if you have a long error message. It also slows down the mousing as you have to move the mouse to the far right side, then back to click on the button. I'm closing this as FIXED. We can open new bugs for ongoing issues.
