Community
Participate
Working Groups
At moment, JDT provides the engine. a) it would nice, if I have also a spelling engine, if I don't use Java/have JDT. b) perhaps, instead of 'default spelling engines' from each programming language plug-in (like JDT), it would make more sense, the language specific spelling engines extend/wrap the platforms default.
We would have liked to do this for 3.3 but there wasn't enough time. Re b: the Java spelling engine is already content type aware.
Will this make it into 3.3.1?
No way. This is feature work and is quite complicated. It's not even sure we spend time during 3.4 for this. Sorry. Of course patches are welcome.
If no default engine is planned it would be good to at least change the preference page UI to give a more informative message. Currently it just says "The selected spelling engine does not exist". There is no selected spelling engine. I think it should just tell you there is no spell checking support installed in the platform, or not even show the preference page at all if there are no spell checking engines.
That's already done for 3.3.1.
Okeedokee.
The problem that I'm seeing with the spellingEngine extension is that contributions are global to the platform. The JDT implementation is trying to handle other content types and in my opinion this is incorrect. It should just stick to handling JAVA relevant files. What ends up happening is that any unrecognized content type is treated as text and then you end up with a bunch of spelling errors on files for other languages. Here is a snippet of the JDT's implementation. ISpellingEngine engine= getEngine(context.getContentType()); if (engine == null) engine= getEngine(TEXT_CONTENT_TYPE); if (engine != null) engine.check(document, regions, context, collector, monitor); It seems the extension point should have an entry for content type id and the SpellingService should provide the correct engine based on type.
*** Bug 204370 has been marked as a duplicate of this bug. ***
My product is built on top of Eclipse 3.4 and I want to make use of the spell checking capability. I want to know if Eclipse Spell Checking support will be part of the Eclipse 3.4 Runtime.
Sorry, we do not have enough resources to work on this. A patch would of course be reviewed and accpeted if it's of good quality.
Perhaps, this can be given a slightly higher priority? Enabling spell checking in text fields would be a huge value-add in many RCP applications. Thank you.
Sorry, situation is still as outlined in comment 10.
*** Bug 297019 has been marked as a duplicate of this bug. ***
As a first step towards this bug, I suggest to move the spell checker out of JDT and into the core. Right now, I prefer to move it to "org.eclipse.core.spelling". Any reasons to put it elsewhere?
The spelling API is tightly coupled to Platform Text APIs (org.eclipse.ui.workbench.texteditor) and hence the engine would have to be put there.
Tagging for M3 for a detailed investigation (feasibility, impact for clients, time estimates, etc.).
Created attachment 185201 [details] v0.01 I think we should be able to do the following. (The patch tries to accomplish some of it) Use existing extension point - Add an optional attribute 'contentType' to the existing extension point - If contentType is missing then take that engine as an 'AllContentType' engine. This should take care of spell engines contributed by others e.g. CDT, another RCP app. (See SpellingEngineRegistry.getDescriptor(IContentType) ) - Maintain a map of engine and content types in SpellingService (currently in DefaultSpellingEngine), and if a AllContentType engine is found then it is used. - else the engine for the given content type is found and used => So JDT will contribute 2 engines - JavaSpellingEngine, PropertiesFileSpellingEngine And Platform will contribute 1 engine - TextSpellingEngine Preferences - Some common preferences and some engine specific preferences - User should be able to select an engine from a drop down list and set preferences for that engine (there is already some code for this, see org.eclipse.ui.internal.editors.text.SpellingConfigurationBlock) - The engine selector should show the label attribute of the engine (content type can also be shown in addition to the label if it makes sense) - Options could be provided to user to turn on spell checking only for some content types. - I don't think we need options to allow a user to choose an engine for a content type. (Preferences are mostly broken with the attached patch) Dictionaries Each engine will have its own dictionary (or dictionaries) - The current English dictionaries go with TextSpellingEngine - Spell checking of plain text should be delegated to TextSpellingEngine (As with Preferences this also does not work yet) Spelling QuickFixes With the patch the quickfixes mostly work in the text editor (but not is Java Editor, though that can be easily fixed). Contribution questionaire for the English Dictionary (https://dev.eclipse.org/ipzilla/show_bug.cgi?id=1416) was filed only for JDT Note: There is some hardcoding done in the patch, to get it to work you will have to provide the correct dictionary path in SpellCheckEngine.resetUserDictionary() I think it will take another 2 weeks to complete work on this, as a lot of code is touched and a number of loose ends have to be tied up.
By looking at the old implementation I saw some drawbacks and I'd like to know if these are being addressed right now. 1) The code massively referns to static instances. This makes reuse of exisiting interfaces a hazard. There was no easy way to replace the default spell checking engine with my own implementation nor was I able provide my own dictionaries. Why, you may ask because the interfaces are already there which gets me to 2) The plugins depend on several eclipse UI plugins. I can't see how the process of spell checking is UI dependent. Of course at the end I want to provide feedback to the user but how about contributing a dictionary from a non-UI Plugin to the spelling engine. I end up adding UI dependencies where previously were none. If you could please provide some feedback how the upcoming implementation addresses these issues.
(In reply to comment #18) > By looking at the old implementation I saw some drawbacks and I'd like to know > if these are being addressed right now. > > 1) The code massively referns to static instances. This makes reuse of > exisiting interfaces a hazard. There was no easy way to replace the default > spell checking engine with my own implementation nor was I able provide my own > dictionaries. > > Why, you may ask because the interfaces are already there which gets me to > > 2) The plugins depend on several eclipse UI plugins. I can't see how the > process of spell checking is UI dependent. Of course at the end I want to > provide feedback to the user but how about contributing a dictionary from a > non-UI Plugin to the spelling engine. I end up adding UI dependencies where > previously were none. > > If you could please provide some feedback how the upcoming implementation > addresses these issues. This bug is only about moving the spelling engine away from JDT. There are no plans to address the issues you mention with this bug (e.g. moving spelling to the non-UI layer).
Deepak, I didn't have time yet to look at your patch. Can you please try the following: 1. run Platform + your patch + JDT + CDT and verify that spell checking works in the CDT editor (this is already working in 3.6). 2. run Platform + your patch + CDT and verify that spell checking works in the CDT editor (this does not work in 3.6 since the engine was in JDT).
(In reply to comment #20) > Deepak, I didn't have time yet to look at your patch. Can you please try the > following: > 1. run Platform + your patch + JDT + CDT and verify that spell checking works > in the CDT editor (this is already working in 3.6). > 2. run Platform + your patch + CDT and verify that spell checking works > in the CDT editor (this does not work in 3.6 since the engine was in JDT). These tests don't seem to make a lot of sense since CDT has its own spelling engine. See org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/spelling/CSpellingEngine.java and CSpellingService.java.
> These tests don't seem to make a lot of sense since CDT has its own spelling > engine. See > org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/spelling/CSpellingEngine.java > and CSpellingService.java. Well, I assume they are using the current infrastructure/framework from Platform Text. Are you saying that it's OK to break them and we should not care about that?
Besides the already mentioned tests you need to test that: 3. run Platform + your patch + JDT + CDT and verify that the default and the CDT spelling engines appear and work (especially the CDT one). 4. run Platform + your patch + CDT and verify that the default and the CDT spelling engines appear and work (especially the CDT one).
(In reply to comment #21) > These tests don't seem to make a lot of sense since CDT has its own spelling > engine. See > org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/spelling/CSpellingEngine.java > and CSpellingService.java. Try to get in contact with the guy who wrote this; maybe he's interested in this as well. That way, we could get one step closer to having a single spelling core for all plug-ins.
> to having a single spelling core for all plug-ins. That's the plan. There is already a common core - just the engines (and complete dictionaries) are specific at this point.
(In reply to comment #24) > Try to get in contact with the guy who wrote this; maybe he's interested in > this as well. That way, we could get one step closer to having a single > spelling core for all plug-ins. That guy is me. Breaking CDT is definitely not an option. All I was saying that is unlikely to break. A more involved test should include removal of org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/spelling/CSpellingService.java
(In reply to comment #26) > (In reply to comment #24) > > Try to get in contact with the guy who wrote this; maybe he's interested in > > this as well. That way, we could get one step closer to having a single > > spelling core for all plug-ins. > > That guy is me. ;-) > A more involved test should include removal of > org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/spelling/CSpellingService.java Deepak, that's test 5 then for you ;-)
Created attachment 187093 [details] v0.1 (In reply to comment #17) > Use existing extension point > - Add an optional attribute 'contentType' to the existing extension point > - If contentType is missing then take that engine as an 'AllContentType' > engine. This should take care of spell engines contributed by others e.g. CDT, > another RCP app. (See SpellingEngineRegistry.getDescriptor(IContentType) ) > - Maintain a map of engine and content types in SpellingService (currently in > DefaultSpellingEngine), and if a AllContentType engine is found then it is > used. > - else the engine for the given content type is found and used No change here > Preferences > (Preferences are mostly broken with the attached patch) These work now. Test results (yes: spell check works, no: spell check does not work) Platform + patch + JDT -Text editor: yes -Properties file editor: yes -Java editor: yes Platform + patch + CDT -CDT editor: yes -Text editor: no Platform + patch + JDT + CDT -CDT editor: yes -Text editor: no -Properties file editor: no -Java editor: no If CDT is present I was trying to use the CDT spelling engine (AllContentType case as mentioned above), which does not work and is probably not right as well. (The patch is zipped because bugzilla does not allow files greater than 2000KB - the patch includes dictionaries)
Created attachment 187095 [details] v0.2 (In reply to comment #28) > If CDT is present I was trying to use the CDT spelling engine (AllContentType > case as mentioned above), which does not work and is probably not right as > well. This is fixed in this patch (see SpellingEngineRegistry.getDescriptor(IContentType)). Test results (yes: spell check works, no: spell check does not work) Platform + patch + JDT -Text editor: yes -Properties file editor: yes -Java editor: yes Platform + patch + CDT -CDT editor: yes -Text editor: yes Platform + patch + JDT + CDT -CDT editor: yes (checked by CSpellingEngine) -Text editor: yes (checked by TextSpellingEngine) -Properties file editor: yes (checked by JavaSpellingEngine) -Java editor: yes (checked by PropertiesFileSpellingEngine) So most of the things work and CDT does not break with this patch! Will someone else break ? I don't know that yet. (In reply to comment #26) > Breaking CDT is definitely not an option. All I was saying that is unlikely to > break. A more involved test should include removal of > org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/spelling/CSpellingService.java I have not tried this yet.
The change is quite big, and while most things work the code is bit of a mess. It will not make it in 3.7.
(In reply to comment #29) > -Properties file editor: yes (checked by JavaSpellingEngine) > -Java editor: yes (checked by PropertiesFileSpellingEngine) That's a typo: the engines are swapped. (In reply to comment #30) > The change is quite big, and while most things work the code is bit of a mess. > It will not make it in 3.7. Agree to attack in 3.8 M1.
Just a status(In reply to comment #31) > > The change is quite big, and while most things work the code is bit of a mess. > > It will not make it in 3.7. > Agree to attack in 3.8 M1. Any update on this? Can you please also add bug 92348 as dependent on this one? Thanks!
(In reply to comment #32) > Just a status(In reply to comment #31) > > > The change is quite big, and while most things work the code is bit of a mess. > > > It will not make it in 3.7. > > Agree to attack in 3.8 M1. > > Any update on this? Can you please also add bug 92348 as dependent on this one? > Thanks! Other work became more important. It's unlikely that this will make it into 3.8.
Is there a target version on this? (I'd like to know because depending on the time-frame I may end up adding yet another spelling engine to PyDev or not) Thanks, Fabio
(In reply to comment #34) > Is there a target version on this? There are no plans to work on this.
It's a really good initiative. It's a pity this item is not planned for a next release :'(
So we have an initial patch proposal attached, but Dani and Deepak say that "the change is quite big" and "the code is a mess" (in comment 30). I'm wondering what's the minimum amount of change needed for this to make any sense ? Even if no work is scheduled for this right now, what do the experts think where this should be going - is the attached patch the right approach ?
(In reply to comment #37) > So we have an initial patch proposal attached, but Dani and Deepak say that > "the change is quite big" and "the code is a mess" (in comment 30). The change is > 200k lines. > I'm wondering what's the minimum amount of change needed for this to make > any sense ? Even if no work is scheduled for this right now, what do the > experts think where this should be going - As outlined in this bug, the code should go from JDT to Platform Text. But at least for Kepler and Luna it is not going anywhere, since there are no resources to do this. > is the attached patch the right > approach ? I trust Deepak, that it goes into the right direction, but given he says "the code is a bit of a mess", I'd qualify the patch as POC only.
If you cared to look at the patch, you'd see that the size is due to the fact that a couple of big files are moved around in the source tree and that generates a lot of +- lines where nothing important changes. For example, the file `dictionaries/en_GB.dictionary` is moved which has 49824 lines, so that file alone is half of the patch. The rest is moving classes to new packages. It's hard to see what code was actually changed. The diff format is clearly unsuited to give an easy way to see what is going on. I can't say much for "the code is a mess" since I couldn't figure out what has been changed (except for the absolute necessary changes like fixing the package names). My gut feeling is that this applies to the original code as well. The question is how to proceed. Quite a few people are interested in this but not enough to contribute. How dangerous would it to be to apply the patch? Or could we keep the old plugin as is and just create a 1:1 copy as a first step. So the first patch would be just renaming of files. That way, we could add tests and improve the API while everyone still used the original plugin. Things would still work *and* Deepak could make this work without much pressure. When the quality of the new plugin is good enough, he could post patches to switch the editors to the new plugin.
Forgot to mention: The file en_US.dictionary is the other half of the patch with 49759 lines. If you ignore those dictionaries, about 18'000 lines are left. As I said, most of those are just "rename Java package" patches which are also big but "harmless".
So if there is enough public interest, what about creating a clone on github, applying the patch, and continuing experiments there ? Access permissions can be added fine granular on github I think, so it should be easy to allow multiple people to collaborate. It might be a bit more complex if the JDT and/or CDT code also need changes, but if I understood the patch right then the initial version should work with JDT and CDT unmodified. Pydev might be a good candidate to try out the changes, since that's available public on github as well and doesn't support spell checking today if I'm not mistaken.
PS of course if the eclipse.org repository owners are willing to create a branch as well and serve pull requests, that would easy the IP side of it. Accepting a whole branch from github where multiple people contributed might be hard, therefore it would be good to get the initial patch into a branch on Eclipse.org and clone from there, given that Deepak is a committer.
I'm not sure if JDT has already the Gerrit review system setup but if, the patch could be staged their and people could create an Eclipse IDE based on CBI with that patch.
(In reply to comment #43) I'd like Gerrit because it would make the IP flow clear, but in my experience dealing with very large patches gets problematic in Gerrit. Since the "special Gerrit Branches" are not directly accessible. I think that even if Gerrit is used for tracking individual contributions, it would make sense to have a regular branch in the repository for the entire feature. Then Gerrit can track contributions against that branch.
The first step is to take the patch and make sure it applies to master and attach it here. That patch does not need to contain the dictionaries, as they can easily be moved 1:1 as a separate step. It would not need any IP work, assuming, it just adjusts the current patch. When that patch is available, I can put that code into a branch for volunteers to work/test with. First step would be to test whether CDT and others (see all the scenarios listed in this bug) still work, given that JDT will no longer provide the spelling engine. Side note: all translated dictionaries will have to be adjusted, since they are currently in fragments for JDT UI. They will have to be in fragments for Platform Text.
Thanks Dani, Your approach absolutely makes sense. Just for completeness, while the attached "Eclipse Workspace Patch" comes in a single file, multiple git repositories are affected; the JDT part (removing Spelling) and the Platform part (adding Spelling). While it makes sense to rebase the patch onto master entirely, and create the respective branches entirely, I assume that any follow-up work would mostly be on the Platform side; and I assume it would be an acceptance criterion that the existing JDT and CDT spelling engines continue to work even if the new Platform engine exists. So, change of existing translated fragments for JDT is perhaps less of an issue when the JDT engine continues to exist in parallel for a while. Projects currently affected (from the #P of the attached patch): Line 2: #P org.eclipse.jdt.text.tests Line 33: #P org.eclipse.jdt.ui Line 107161: #P org.eclipse.jdt.ui.tests Line 107185: #P org.eclipse.ui.editors Line 110740: #P org.eclipse.ui.workbench.texteditor The first three are from jdt/eclipse.jdt.ui.git The latter two are from platform/eclipse.platform.text.git
(In reply to comment #46) > So, change of existing translated > fragments for JDT is perhaps less of an issue when the JDT engine continues > to exist in parallel for a while. If we move it down to Platform Text, then we will remove it in JDT as test/show case that it actually works. If it turns out, that this breaks clients, and we would end up having to maintain both code bases, then this would be a no go.
Sure. My point was, since you had mentioned the JDT team had no time for this in Luna, the JDT contribution would remain in place for Kepler and Luna; but my hope was that the Text contribution could still be added in Luna (or a fork of the Platform which adopters install by feature patch). Since there might be other spelling engines than CDT and JDT, which might not be able to migrate to the new base, the new base must not break existing engines. That's IMO the more important must-have criterion. In other words, whatever requirements the JDT team has, they can't be blockers for the new capability in Platform/Text. Since the plain text capability is what RCP's etc need. I see your point about not duplicating code bases; but if the JDT team has not time or other reasons why they can't get rid of their implementation, it would not be fair to block the Platform/Text addition because of that.
(In reply to comment #48) > Sure. > > My point was, since you had mentioned the JDT team had no time for this in > Luna, the JDT contribution would remain in place for Kepler and Luna; but my > hope was that the Text contribution could still be added in Luna (or a fork > of the Platform which adopters install by feature patch). > > Since there might be other spelling engines than CDT and JDT, which might > not be able to migrate to the new base, the new base must not break existing > engines. That's IMO the more important must-have criterion. > > In other words, whatever requirements the JDT team has, they can't be > blockers for the new capability in Platform/Text. Since the plain text > capability is what RCP's etc need. > > I see your point about not duplicating code bases; but if the JDT team has > not time or other reasons why they can't get rid of their implementation, it > would not be fair to block the Platform/Text addition because of that. There is no Platform Text committer outside the JDT team, so you are not buying yourself anything here ;-). I would never start to review a solution that ends up with two code bases that *we* have to maintain plus no use case in the Eclipse SDK where the Platform spelling engine is used/extended by a client (like e.g. JDT).
Just to note, if someone wants to use a spelling engine, I've extracted the relevant code from JDT some time ago and created a project in github (which I use in LiClipse): The repo with the code is: https://github.com/fabioz/eclipse.spellchecker (there's currently no update site for it -- it has to be compiled to be used and I don't plan to create an update site because the proper solution would be having this properly integrated in eclipse itself). Note that I don't have plans to extend it nor further enhance it -- as I don't want to create a fork (so, it's bound to become outdated), but for those that want the capability without relying on JDT it's possible to use the plugins on that repo until a committer from the platform decides this is important enough given its priorities. i.e.: those that want to have that as a part of the core eclipse should add a vote on this bug to raise its priority.
(In reply to Fabio Zadrozny from comment #50) > the plugins on that repo until a committer from the platform decides this is > important enough given its priorities. I would be interested in bringing it into platform, but I think it should go into platform.text for which I'm not (yet) a committer. But maybe you can I can work on it and created Gerrit reviews for it?
(In reply to Lars Vogel from comment #51) > I would be interested in bringing it into platform, but I think it should go > into platform.text for which I'm not (yet) a committer. Yes, not one doubts it needs to go into Platform Text. There are already some close patches. > But maybe you can I > can work on it and created Gerrit reviews for it? Not sure how that translates into German ;-)
(In reply to Dani Megert from comment #47) > If we move it down to Platform Text, then we will remove it in JDT as > test/show case that it actually works. If it turns out, that this breaks > clients, and we would end up having to maintain both code bases, then this > would be a no go. If that happens we could still move the code (and dictionary) and deprecate the old functionality.
(In reply to Dani Megert from comment #52) > Yes, not one doubts it needs to go into Platform Text. There are already > some close patches. Sounds good, I try to convert them (hopefully with the help of Fabio) to Gerrit reviews for Mars.
Hi Lars, Since Dani gave us a green light I think it'll be worth porting it to gerrit then :) I think the best timing for this is right after 4.4 is released right? -- so we'll have enough time to test until a final release :)
(In reply to Fabio Zadrozny from comment #55) > I think the best timing for this is right after 4.4 is released right? -- so > we'll have enough time to test until a final release :) Yes, lets try to work on this beginning of Mars.
*** Bug 450144 has been marked as a duplicate of this bug. ***
Just to be sure, This will not be available for Eclipse Mars ? Maybe for eclipse 4.6 ? :)
(In reply to Simon Bernard from comment #58) > Just to be sure, This will not be available for Eclipse Mars ? > Maybe for eclipse 4.6 ? :) Fabio?
I definitely won't have time to get this for Mars.
*** Bug 521658 has been marked as a duplicate of this bug. ***
If I am not mistaken, Oxygen has seen some changes in the way text editors work. Would now be a good time to get the spell checker infrastructure to catch up? It's quite frustrating from an end user point of view to have partial support for spelling, depending on what editor you happen to be in.
I confirm Peter point of view. In my case spell check works only inside C editor, but it would very usefull to have it working in mylyn task editor in primis and other txt and source (php and javascript) too.
This has been resolved, now mylyn, php and javascript and txt file have the spell checker that work by means of pressing ctrl+1. so form me this bug is fixed
(In reply to Alessandro Fardin from comment #64) > This has been resolved, now mylyn, php and javascript and txt file have the > spell checker that work by means of pressing ctrl+1. > so form me this bug is fixed I think they copied JST engine, we should still move it.
s/JST/JDT
On eclipse oxygen at some point the spell check starts working also in task editos, php editors, etc. (I'm using eclipse cdt with some oter plugin installed). Now I've upgraded to photon and splell check works only on C editors.
(In reply to Alessandro Fardin from comment #67) > On eclipse oxygen at some point the spell check starts working also in task > editos, php editors, etc. (I'm using eclipse cdt with some oter plugin > installed). > Now I've upgraded to photon and splell check works only on C editors. This is completely unrelated to this bug report. Please file a separate one.
Hi Dani, Sorry if I post in the wrong place, but, last year I was opened the bug 521658 that was closed as dupicated of this one. And the current behaviour is the same I was descibed in the bug 521658. I thought this was a regression respect oxygen
(In reply to Alessandro Fardin from comment #69) > Hi Dani, > Sorry if I post in the wrong place, but, last year I was opened the bug > 521658 that was closed as dupicated of this one. > And the current behaviour is the same I was descibed in the bug 521658. > I thought this was a regression respect oxygen No regression. If it worked before and no longer works, then file a bug report against the specific editor.
Hi Dani Ive got the preferences/general/editors/text editors/spelling enabled. The problem is that only C editor have the spell checking working. all other editors have no spell check (and pressing ctrl+1 provide "no suggestions available"): 1) generic editor --> NOK 2) javascript editor --> NOK 3) php editor --> NOK 3) sh editor --> NOK 4) task editor --> NOK it sounds strange to me that is aproblem of each editors. TO make a test I've just installed the eclipse PDT package from scratch: in this eclipse instance the preferences/general/editors/text editors/spelling say: "The spelling service is not installed" I think I'm missing something, I can't uderstand why in eclipse oxygen spelling work right, with photon only C editor works. Reading the mylyn, php ecc official guide it seem that it would work as it is. Please explain me what is the current (expected correct) behaviour in term of spell check
(In reply to Alessandro Fardin from comment #71) > Please explain me what is the current (expected correct) behaviour in term > of spell check As this bug says, the spelling engine is currently in JDT. So, if you install something that does not have JDT, it won't work unless the editor(s) provide their own implementation. I don't think anything changed since Oxygen. You probably just have a different install.
After some check i've understand that the plugin angular2 eclipse (I've installed it in eclipse oxygen) from angelo zerr installs some jdt plugin that make spell check works in all editor. The CDT has it's own spell checker. I've installed angular2 eclipse also in photon and now spell check works also in photon.
Added CDT committers, which AFAIK also keep a copy of the spelling engine. Maybe they want to help.
