Bug 488798

Comment 1 Curtis Windatt 2016-03-03 17:26:54 EST

Created attachment 260074 [details]
Fix

We have the annotation model when rendering the quickfix so this fix iterates through the annotations until it finds one with a matching id.  I haven't see any performance issues with this fix (file with ~300 annotations + worst case scenario of no duplicate annotation).  If you have a file with way more annotations the performance will get worse, but usually the rendering of the annotations themselves is already causing perf problems in the editor.

Comment 2 Curtis Windatt 2016-03-03 17:30:39 EST

Steve, what do you think about this?  Without this change, there is no indication that you are modifying mulitple things in the file or not.  Alternatively we could disable the checkbox rather than hide it to indicate that quickfix all exists but doesn't make sense.

The deluxe solution would be to get the complete list of matching annotations so we have a count "Fix all 7 problems".  We would want to store the list in the command so we don't redo the work when the command is executed.

Comment 3 Steve Northover 2016-03-03 17:38:41 EST

I guess I am not understanding the problem.  I followed the steps and selected 'Fix all' and the ";;" disappeared.  Is it that we are showing the same fix twice on the same line both with a fix all?

Comment 4 Curtis Windatt 2016-03-07 09:56:33 EST

(In reply to Steve Northover from comment #3)
> I guess I am not understanding the problem.  I followed the steps and
> selected 'Fix all' and the ";;" disappeared.  Is it that we are showing the
> same fix twice on the same line both with a fix all?

If you only have one instance of a problem in your entire file, we still offer you the option of 'fix all'.  It gives the incorrect impression that you have multiple problems in the file.

In my first implementation I had this check (in fact I listed how many problems existed in the file).  So not having it now was bugging me.  However, I suspect other users may not care about this information and are fine with running fix all and having it fix an unknown number of issues.

Comment 5 Steve Northover 2016-03-07 13:44:21 EST

Personally, I wouldn't bother with this.  I am fine with an 'All' button when there is only one thing to be fixed.  Can we close this?  The risk is that we put in fancy code and get it wrong and break the feature versus just leaving the current behavior that is fine.

Comment 6 Curtis Windatt 2016-03-07 14:50:17 EST

Realized that Mike and Olivier weren't cc'd on this.  I don't think we should close this without some discussion of what we think is the best user experience.  This change is not creating any real risk to the stability of quick fixes.  There is a small performance penalty (iterating through the file's annotations).

1) Always show quick fix all checkbox
2) Only show quick fix all checkbox if there are multiple problems in the file
3) Show the number of problems that would be fixed by running the quick fix.

When first demo'ing quick fix all, there was definitely some interest in (3).

Comment 7 Steve Northover 2016-03-07 14:54:43 EST

Agree.  Let's see what they think.

3) is very cool.  It give you some idea about how many places will be changed and how far the code is out of whack.  It seems somewhat unrelated to this bug.

I am not violently opposed to implementing this feature, it's just that it seems a bit of over kill and there are more fish to fry.

Comment 8 Michael Rennie 2016-03-07 16:57:07 EST

(In reply to Curtis Windatt from comment #6)
> Realized that Mike and Olivier weren't cc'd on this.  I don't think we
> should close this without some discussion of what we think is the best user
> experience.  This change is not creating any real risk to the stability of
> quick fixes.  
> 
> 1) Always show quick fix all checkbox
> 2) Only show quick fix all checkbox if there are multiple problems in the
> file

> 
> When first demo'ing quick fix all, there was definitely some interest in (3).

I personally am a huge fan of "only show as much UI as absolutely needed", so I am definitely on board for only showing 'fix all' if there is an 'all' to fix. For what its worth, in JDT we only show the option to fix multi-problems where there are multi-problems.

> There is a small performance penalty (iterating through the
> file's annotations).

What kind of performance hit are we talking about here? Does it cause a noticeable delay showing the hover / tooltip? what happens if you have a file with a tonne of problems in it (I'm guessing the perf hit grows as the annotation count goes up)?

> 3) Show the number of problems that would be fixed by running the quick fix.

I agree this is cool.

Comment 9 Curtis Windatt 2016-03-07 17:04:52 EST

(In reply to Michael Rennie from comment #8)
> What kind of performance hit are we talking about here? Does it cause a
> noticeable delay showing the hover / tooltip? what happens if you have a
> file with a tonne of problems in it (I'm guessing the perf hit grows as the
> annotation count goes up)?

There is no delay.  The performance hit consists of iterating through all the annotations and comparing their ID which is going to be blazing fast.  Of course it scales with the size of the annotation list.  Any time I've purposely built a file with thousands of annotations the editor was virtually unusuble due to the auto-save + lint cycle.  Even then I suspect this wouldn't delay the hover opening but I can give it a test.

Comment 10 Steve Northover 2016-03-07 21:39:57 EST

> I personally am a huge fan of "only show as much UI as absolutely needed", 

The flip side of this is the "where they hell did my menu item go" problem.  In any case, not a biggie either way ... but I will haunt you if I find a bug in it!  <g>

Comment 11 Curtis Windatt 2016-04-13 13:56:00 EDT

Closing as WONTFIX.  After having this in my workspace a few times I agree that it is easy to wonder where your fix all option went.  This could be mitigated by listing the number of problems found, but I want to keep the UI as simple as possible.