Community
Participate
Working Groups
As discussed at https://github.com/angelozerr/jdt-codemining/issues/57
working on it at https://github.com/enikao/eclipse.platform.text/blob/Bug_540443/org.eclipse.ui.genericeditor/src/org/eclipse/ui/internal/genericeditor/ProblemMarkerCodeMiningProvider.java
Thanks Niko, please provide a Gerrit.
Created attachment 276386 [details] Current state (icons instead of unicode, overlapping annotations in same file) I replaced the unicode symbols by icons (as proposed by @vogella): Outstanding issues: * I currently re-use the icons from org.eclipse.ui.internal.ide.IDEInternalWorkbenchImages. What's the right thing to do here? Ignore the warnings? Copy the icons? * If there's more than one marker at one line, they appear at the same line (IMHO, that's a general code mining issue). However, the text of the first entry is cut off by the number of pixels I reserved for the icon. * Do I need to translate the exception messages in e.g. https://github.com/enikao/eclipse.platform.text/blob/Bug_540443/org.eclipse.ui.genericeditor/src/org/eclipse/ui/internal/genericeditor/ProblemMarkerCodeMining.java#L93 ? * How to handle monitor.isCancelled() in streams? This looks rather strange: https://github.com/enikao/eclipse.platform.text/blob/Bug_540443/org.eclipse.ui.genericeditor/src/org/eclipse/ui/internal/genericeditor/ProblemMarkerCodeMiningProvider.java#L84
Please provide a Gerrit so that we can start the review process. As suggested on Github, please also put the code minings in the same line as the error markers.
If you need help with Gerrit, let me know via email, happy to help here.
Are you sure? I like the minings in their own line, because the whole idea is to be able to read the error message without any interaction. If we show the mining behind the line, we might be able to read the first error message if it's a short one, but not a long error message or multiple ones.
(In reply to Niko Stotz from comment #6) > Are you sure? I like the minings in their own line, because the whole idea > is to be able to read the error message without any interaction. If we show > the mining behind the line, we might be able to read the first error message > if it's a short one, but not a long error message or multiple ones. Ok, lets leave it for now as header and see what other people says.
(In reply to Niko Stotz from comment #6) > Are you sure? I like the minings in their own line, because the whole idea > is to be able to read the error message without any interaction. If we show > the mining behind the line, we might be able to read the first error message > if it's a short one, but not a long error message or multiple ones. It depends on the personal taste. When I first saw this feature, I felt like it's better to have the error message at the end of the line. For me, it seemed that it's less disruption when reading the code. On the other hand, your use-cases make sense. Having long and/or multiple messages might be harder to read. With that in mind, now I'd vote for having the message above the line.
New Gerrit change created: https://git.eclipse.org/r/131754
The Eclipse code contribution process feels like "designed by IBM" ... The gerrit patch contains links to the first of 4 commits, it makes no sense to look at them individually (especially "simplified code; replaced unicode symbols by icons" changes a lot of the code).
(In reply to Niko Stotz from comment #10) > The Eclipse code contribution process feels like "designed by IBM" ... > > The gerrit patch contains links to the first of 4 commits, it makes no sense > to look at them individually (especially "simplified code; replaced unicode > symbols by icons" changes a lot of the code). Please squasch them into one commit/ Gerrit.
New Gerrit change created: https://git.eclipse.org/r/131794
sorry, somewhere between the fights with gerrit, git, and rebase, I lost my way. The code in gerrit is NOT what I have in my workspace. Please disregard this gerrit, I'll try again.
(In reply to Niko Stotz from comment #13) > sorry, somewhere between the fights with gerrit, git, and rebase, I lost my > way. The code in gerrit is NOT what I have in my workspace. Please disregard > this gerrit, I'll try again. Now it looks better, please have a look at the gerrit.
Outstanding issue: If there's more than one marker at one line, they appear at the same line (IMHO, that's a general code mining issue). However, the text of the first entry is cut off by the number of pixels I reserved for the icon (also visible in the screenshot attached above).
Thanks Niko, this is a very interesting use-case! Technically, the code mining provider should also be a resource listener to react to changes on marker to refresh the list of code-minings, so things would change immediately instead of waiting for one more edit/save. Also, we probably need a preference to control the severity of the shown problems markers (do we show info, warnings, errors...)?
Created attachment 278495 [details] Preferences dialog I picked up this issue again. Now, there's a preferences dialog (see attachment). The name might not be perfect yet, it's too long. Any suggestions? I implemented a resource change listener as per comment #16.
Created attachment 278496 [details] Two Markers in same line The overlapping issue of comment #15 is fixed.
(In reply to Niko Stotz from comment #18) > Created attachment 278496 [details] > Two Markers in same line > > The overlapping issue of comment #15 is fixed. Why are they shown one *after* each other, not *below*? I would assume code mining can render as many lines as necessary? Some errors / warnings will be unreadable this way.
(In reply to Andrey Loskutov from comment #19) > (In reply to Niko Stotz from comment #18) > > Created attachment 278496 [details] > > Two Markers in same line > > > > The overlapping issue of comment #15 is fixed. > > Why are they shown one *after* each other, not *below*? I would assume code > mining can render as many lines as necessary? Some errors / warnings will be > unreadable this way. I agree, below would be nicer. I think rendering them in the same line is (intended?) behavior of LineHeaderCodeMining. Does this make sense in other cases?
If you want separate actions and separate images (non-emojis) for the code minings, then you need distinct ones and they will currently be rendered on the same line. That is an initial design choice mimicked from VSCode, but not something we couldn't accept a change for. If you want separate lines, (I think) you can have labels just using line breaks and they would be rendered on different lines. It can be a 1 code-mining <-> serverl lines mapping. Using emojis instead of images allows to have one image on each line. The drawbacks are 1. You can embed actual images beyond emojis (although as I already advocated with you at EclipseCon, emojis > custom images) and 2. you cannot easily have specific different actions for each line, although it's still possible to implement it by looking at details of the MouseEvent, but it seems more like a workaround.
Adding a newline doesn't work, as AbstractCodeMining.draw() will not care for it: public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) { String title= getLabel() != null ? getLabel() : "no command"; //$NON-NLS-1$ gc.drawString(title, x, y, true); return gc.stringExtent(title); }
Regarding images vs. emojis: I agree emojis scale better and are easier to implement. However, I think the usual icons for markers are immediately recognized by the user and thus convey the point of the code mining better.
(In reply to Niko Stotz from comment #23) > Regarding images vs. emojis: > I agree emojis scale better and are easier to implement. However, I think > the usual icons for markers are immediately recognized by the user and thus > convey the point of the code mining better. Sure, but I would even advocate to replace the usual error markers in Eclipse IDE by emojis ;), but that's way beyond the current topic. (In reply to Niko Stotz from comment #22) > Adding a newline doesn't work, as AbstractCodeMining.draw() will not care > for it: I don't get what shows here the .draw() method doesn't care, if the label contains is "a\nb" for instance.
Created attachment 278499 [details] No newlines rendered I changed the implementation to Object message = marker.getAttribute(IMarker.MESSAGE); if (message != null) { return message.toString() + "\n" + "asdf"; } Without success (see screenshot). As far as I can tell, there's no trim() or similar in the call chain until gc.drawString(). So I presume drawString() doesn't care about newlines.
(In reply to Niko Stotz from comment #25) > So I presume drawString() doesn't care about newlines. That could be it. It's not really intended that said, there is room for improvement in code mining rendering itself. Would you mind opening a specific ticket for handling for multi-line strings in general?
(In reply to Mickael Istria from comment #26) > (In reply to Niko Stotz from comment #25) > > So I presume drawString() doesn't care about newlines. > > That could be it. It's not really intended that said It's not intended *from code-mining perspective*, I'm not advocating to change the implementation of drawString(), but more to change the implementation of AbstractCodeMining.draw()
(In reply to Mickael Istria from comment #26) > (In reply to Niko Stotz from comment #25) > > So I presume drawString() doesn't care about newlines. > > That could be it. It's not really intended that said, there is room for > improvement in code mining rendering itself. Would you mind opening a > specific ticket for handling for multi-line strings in general? Done: Bug 546995
I had a round of thoughts about it, as the current patch is IMO of good quality for an upcoming merge. I'm wondering whether the code mining provider and the preference page should move to a more generic bundle (like org.eclipse.ui.workbench). Indeed, the code that's in here would for example work as well for JDT and CDT and others (they only need to declare a reconcilier). I suggest the preference and extension be moved upstream, and the preference be something like "Show errors and warnings as code-minings for editors that support it". With this, the same preference would also control JDT editor for instance.
(In reply to Mickael Istria from comment #29) > I had a round of thoughts about it, as the current patch is IMO of good > quality for an upcoming merge. > I'm wondering whether the code mining provider and the preference page > should move to a more generic bundle (like org.eclipse.ui.workbench). > Indeed, the code that's in here would for example work as well for JDT and > CDT and others (they only need to declare a reconcilier). I suggest the > preference and extension be moved upstream, and the preference be something > like "Show errors and warnings as code-minings for editors that support it". > With this, the same preference would also control JDT editor for instance. +1
I agree with Mickael proposal. +1
The code minings actually already do show up in Java editors in its current stage. However, they show a problem: markers represent the _persisted_ state of a file, not the _current_ (possibly dirty) state. So if there's an error in a Java file and the user opens the file, the code mining shows up (as we have a marker). Now the user fixes the error, thus the red squiggly and the red X in the ruler are gone. However, as the user didn't save yet, the marker + code mining is still there. This is really bad user experience. Would it be possible to create the code minings based on the ruler entries instead of markers? We should have ruler entries for all relevant markers anyway, and editors should already take care to keep the ruler entries in sync with the current editor state.
(In reply to Niko Stotz from comment #32) > The code minings actually already do show up in Java editors in its current > stage. Ok cool. > Would it be possible to create the code minings based on the ruler entries > instead of markers? We should have ruler entries for all relevant markers > anyway, and editors should already take care to keep the ruler entries in > sync with the current editor state. That could be worth trying, but I doubt it's as "easy" as implementing code minings based on markers. I've always found JDT annoying on the matter of reporting errors without error markers. It's just using and reimplementing an alien internal thing that doesn't integrate well in the IDE. Markers instead can be made non-persistent for such cases of "work in progress". I would suggest that maybe JDT should adopt markers for such reports as well. That would really make a lot of things (like hooking a quickfix, maintaining JDT-LS...) being able to rely on upstream and more reusable Platform APIs.
(In reply to Mickael Istria from comment #33) > (In reply to Niko Stotz from comment #32) > > The code minings actually already do show up in Java editors in its current > > stage. > > Ok cool. > > > Would it be possible to create the code minings based on the ruler entries > > instead of markers? We should have ruler entries for all relevant markers > > anyway, and editors should already take care to keep the ruler entries in > > sync with the current editor state. A workaround would be to hide code minings on dirty editor. > That could be worth trying, but I doubt it's as "easy" as implementing code > minings based on markers. There is annotation model in text editors. That is what ruler shows. > I've always found JDT annoying on the matter of reporting errors without > error markers. I found it very helpful - an immediate feedback without saving the file. > It's just using and reimplementing an alien internal thing > that doesn't integrate well in the IDE. It integrates well, from the user perspective.
(In reply to Andrey Loskutov from comment #34) > There is annotation model in text editors. That is what ruler shows. Right, this is probably what should be looked for, instead of the ruler UI, to show such additional error annotations. However, it might be hard to infer which annotations are error reports in a generic way since IIRC JDT error annotations use internal types. I'm afraid this has to be a JDT (and then another CDT and another whateverDT) specific development. > > I've always found JDT annoying on the matter of reporting errors without > > error markers. > > I found it very helpful - an immediate feedback without saving the file. That's not the point of saving or not. If you use LSP4E for C#, Rust, JS, TS... you'll also get feedback as you type. But this feedback is stored as error markers so it shows up in Project Explorer, in Problems view, can benefit from the quickFix computers and so on. > > It's just using and reimplementing an alien internal thing > > that doesn't integrate well in the IDE. > > It integrates well, from the user perspective. Except it doesn't show problems in Problems view or Project Explorer. So if you have a lot of editors open, one of them as a "WIP" error annotation and it's hidden in the myriad of editors, one cannot see the error on the Project Explorer to be remembered of it.
I re-implemented this idea, based on Annotations (the structure behind the markers in the ruler) in bug 547665. I think Annotations are more suited, as they show the current state of the editor, vs. Markers mostly show the persisted state.
Should we close this one in favor of bug 547665 ?
Replaced with bug 547665 which is more generic (works on all text editors, and for both annotations and markers) *** This bug has been marked as a duplicate of bug 547665 ***
