Bugzilla – Full Text Bug Listing
|
Description
Angelo ZERR
New Gerrit change created: https://git.eclipse.org/r/118829 @Mickael, could your review my gerrit patch please. New Gerrit change created: https://git.eclipse.org/r/120005 @Mickael, I have redone a new gerrit patch at https://git.eclipse.org/r/120005 I have renamed the issue but I need it to fix https://github.com/angelozerr/jdt-codemining/issues/1 Gerrit change https://git.eclipse.org/r/120005 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=3743188767f8cef3a9854b23ff7569f335f53f70 Thanks, Angelo. This introduce an API addition ( isResolutionFailed ). Is this something still allowed directly in M7? Also, as it's an API, I think we should work on finding a better name as the proposed on sounds a bit awkward compared to usual conventions. We should also investigate whether we do actually need this method or whether some deeper fix can't be found (why would the codemining require to expose the state of resolution? Why making a difference between a failed resolution and an incomplete/wip resolution? Do we really need a public state for a failure?...) (In reply to Mickael Istria from comment #7) > This introduce an API addition ( isResolutionFailed ). Is this something > still allowed directly in M7? Also, as it's an API, I think we should work > on finding a better name as the proposed on sounds a bit awkward compared to > usual conventions. We should also investigate whether we do actually need > this method or whether some deeper fix can't be found (why would the > codemining require to expose the state of resolution? Why making a > difference between a failed resolution and an incomplete/wip resolution? Do > we really need a public state for a failure?...) I used API tooling, cleaned twice the WS and did not see any API error. Do you see an API error? In this case we need to revert and ask the PMC for permission. As you also have other concerns I revert the patch so that Angelo can work on your feedback. New Gerrit change created: https://git.eclipse.org/r/120014 Gerrit change https://git.eclipse.org/r/120014 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=6227d7fd9e564c12cfc7dc4b1069a1bcf734363f New Gerrit change created: https://git.eclipse.org/r/120015 https://git.eclipse.org/r/#/c/120015/ contains the original change from Angelo. (In reply to Lars Vogel from comment #8) > I used API tooling, cleaned twice the WS and did not see any API error. Are you using the API-frozen version (M6) as API baseline? I believe it's necessary to get some feedback from API Tools, but it can probably also lead to false-positive during API Freeze like this. I think this whole story relates to bug 532679 too: if we don't try to "prepare" the empty area, it may be enough to stick with the isResolved() method. Looking at the original description "In this case the mining label is null and a blank line is drawn" -> so why trying to redraw a line case label is null? Can't we just assume that a null mining is equivalent to not resolved? (In reply to Mickael Istria from comment #13) > Are you using the API-frozen version (M6) as API baseline? I'm using our latest 4.7 release. (In reply to Lars Vogel from comment #14) > I'm using our latest 4.7 release. Ok, it explains why API Tools don't complain: version was bumped since 4.7 so additions are permitted. During API Freeze, it may be better to have the last milestone as baseline (so it compares with what's supposed to be frozen), but it may also report some need to bump version for internal stuff which would be false-positive... So it's a bit hard to get an automated behavior that works well in the IDE. The "official" build has a magic comparator which can help here, but good luck if you want to use it locally... > I think this whole story relates to bug 532679 too: if we don't try to "prepare" the empty area, it may be enough to stick with the isResolved() method. This bug uses fResolvedMinings which should be filled only with resolved mining, but it's not true. Today fResolvedMinings can be filled with mining which can be cancelled, which provides the problem with "no command" text is displayed (case of mining is resolved and label is null).no It causes 2 problems: * flicking of codemining which display "no command" sometimes (case of mining which canceled) and the mining label. * no the possibility to avoid drawing the mining. A mining draw content with label, BUT it can draw other thing (like colorized square). In this case of colorized sqare, label is null. label is null before the execution of the CompletableFuture and after if CompletableFuture was canceled. So how to distinguish those 2 cases? My gerrit patch distinguish the case of mining was resolved (CompletableFuture was done with or without error) and resolved with failed (only when error). When resolved with failed was done, the mining must not be drawn and fResolvedMinings must not to be updated. The fResolvedMinings must keep the last mining which was resolved with none error. To reproduce the problem easily, you can add at https://github.com/angelozerr/jdt-codemining/blob/master/org.eclipse.jdt.codemining/src/org/eclipse/jdt/internal/ui/javaeditor/codemining/JavaReferenceCodeMining.java#L57 ---------------------------------------------------- if (true) throw new CancellationException(); ---------------------------------------------------- which emulates a cancel of mining. You will see "no command" which is diplayed. fResolvedMinings is updated with this label mining because mining is resolved (but have errore). So my gerrit patch fix those 2 problems and if it accepted, I could support https://github.com/angelozerr/jdt-codemining/issues/1 easily (just CancellationException when refCount == 0). Hope you will understand my explanation. @Mickael, I'm sorry to insist, but I think my gerrit patch provides a better support for mining: * in term of bug "no command" can be displayed if mining is canceled and drawn together. * in term of performance: we don't try to redraw a mining which was canceled * in term of functionnality: you can ignore the draw of mining by cancelling the mining in the resolve. I'm using this idea to not draw "0 references" https://github.com/angelozerr/jdt-codemining/issues/1 I hope you will understand my explanation. (In reply to Angelo ZERR from comment #16) > which provides the problem with "no command" text is > displayed I don't think this is a desired behavior for end user. If there is no command, for whatever reason, the proper thing to do is to not show anything. If there is a bug or something else that prevents the command from being resolved, it's better to just log it than to pollute user's editor. > A mining draw content with label, BUT it can draw other thing (like > colorized square). In this case of colorized sqare, label is null. label is > null before the execution of the CompletableFuture and after if > CompletableFuture was canceled. So how to distinguish those 2 cases? We could consider the "label is null" case as not resolved, and distinguish it from the "label is empty". To draw a square with color, the mining could return empty string or "\u00A0" as label when ready to draw to toggle enablement, and override the size as best for it. I really believe it's important for the maintainability of the API to not add a plethora of state methods, and to use existing one as much as possible. > You will see "no command" which is diplayed. As a end-user, I don't want to see this. As a plugin developer, I do not want to pollute the end-user editor with a bug of mine. As a plugin developer, I want the error from my mining to be logged in the regular log. As a plugin developer, I want the error on end-user IDE to be logged in the usual log so the error reporting features can be used by end-user to notify of the bug. New Gerrit change created: https://git.eclipse.org/r/120110 @Mickael, please see my new gerrit patch https://git.eclipse.org/r/120110 where I have tried to follow your idea with label (see ICodeMining#getLabel()) for explanation). Hope you will like it. Gerrit change https://git.eclipse.org/r/120110 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=695bac0ada85a8d646ca492b7d1910e9c98b265a Thanks a lot @Karsten for your review and merge my gerrit patch! @Mickael, hope it will be OK with this merge of my gerrit patch. @Lars, you can reinstall jdt codemining and use this merged gerit patch and retry https://github.com/angelozerr/jdt-codemining/issues/1 (if it works, please close it). Thanks! I reopen the bug because there is a little bug when you have several minings and label is empty: For instance if you have * mining1 with label ="" * mining2 with label="references 2" It displays: ------------------- | references 2 ------------------- instead of displaying ------------------- references 2 ------------------- (without pipe) New Gerrit change created: https://git.eclipse.org/r/120651 @Lars, please review my gerrit https://git.eclipse.org/r/120651 (which is basic). I have seen this problem with your screenshot with JUnit code mining (which starts with '|' because test method have 0 references). Thanks! Gerrit change https://git.eclipse.org/r/120651 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=0b66d53a6f7a1ca03c79ede469e7c0d54367d1b7 Thanks @Mickael! |