Opened 15 years ago
Closed 15 years ago
#5145 closed Task (fixed)
SCAYT fixes
Reported by: | WebSpellChecker.net | Owned by: | |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 3.3 |
Component: | UI : Spell Checker | Version: | SVN (CKEditor) - OLD |
Keywords: | Review+ | Cc: |
Attachments (8)
Change History (28)
Changed 15 years ago by
Attachment: | 5101.patch added |
---|
comment:1 Changed 15 years ago by
Changed 15 years ago by
Attachment: | 5101_2.patch added |
---|
comment:3 Changed 15 years ago by
Keywords: | Review? added; review+ removed |
---|
comment:4 follow-up: 7 Changed 15 years ago by
Keywords: | Review- added; Review? removed |
---|---|
Summary: | all spellchekcer.net fixes → all spellchecker.net fixes |
The changes from CKEDITOR._scaytParams to CKEDITOR.config._scaytParams mean that it won't work the same way for people that try to upgrade without reviewing their configuration.
This can be solved by using something like this:
var configParams = CKEDITOR.config._scaytParams || CKEDITOR._scaytParams ; if ( configParams ) { for ( var k in configParams ) { oParams[ k ] = configParams[ k ]; } }
The configuration options don't need to use an or, just set the default value:
CKEDITOR.config.scayt_autoStartup = false;
The initialization at
oParams.customerid = editor.config.scayt_customerid || "1:WvF0D4-UtPqN1-43nkD4-NKvUm2-daQqk3-LmNiI-z7Ysb4-mwry24-T8YrS3-Q2tpq2"; oParams.customDictionaryIds = editor.config.scayt_customDictionaryIds; oParams.userDictionaryName = editor.config.scayt_userDictionaryName; oParams.sLang = editor.config.scayt_sLang || "en_US";
should be done in the default configuration values (put there the scayt_customerid and remove the ORs in this block)
There are several spelling mistakes in the documentation of the configuration options, example: "Languages tab will ont rendered ", and the sentences should start with a capital letter.
The example in the last two configuration items repeat the " set up German language as prior for spellchecking "
Changed 15 years ago by
Attachment: | 5145_3.patch added |
---|
comment:5 Changed 15 years ago by
I'm attaching a new patch with the following changes:
- Removed the element path fix. There is no need to have an all in one patch. In this way things don't get blocked because of other issues, and the review is simpler. I've uploaded a new dedicated patch at #3570.
- Code cleanup.
- Better documentation.
- Transformed all settings in "documentation only" comments. We don't need to waste file size by setting default values.
I didn't change the code intent in general. I was not able to check the effectiveness of it because SCAYT stopped working after the patch. In fact, the new scayt22.js file is returning 404.
I would ask SpellChecker.net to review the changes and additionally, based on my patch, solve these issues:
- Make SCAYT work with the patch.
- Previously,
CKEDITOR._scaytParams
was being used for the SCAYT hidden stuff, which is a bad choice. NowCKEDITOR.config._scaytParams
is being used, which is even worst. Please change it toCKEDITOR._.scaytParams
, which is the right way to hide internal stuff.
comment:6 Changed 15 years ago by
Summary: | all spellchecker.net fixes → SCAYT fixes |
---|
comment:7 Changed 15 years ago by
Replying to alfonsoml:
The changes from CKEDITOR._scaytParams to CKEDITOR.config._scaytParams mean that it won't work the same way for people that try to upgrade without reviewing their configuration.
The fact is that CKEDITOR.config._scaytParams is not really a public setting (if it is, it must not have the underscore). I've addressed it in my previous comment.
The configuration options don't need to use an or, just set the default value.
The initialization at ... should be done in the default configuration values (put there the scayt_customerid and remove the ORs in this block)
Actually, the opposite is correct. We should avoid having code to simply initialize default settings. I've addressed it with my patch also.
Your other comments have been addressed also.
comment:8 Changed 15 years ago by
Milestone: | CKEditor 3.2 → CKEditor 3.x |
---|
Changed 15 years ago by
Attachment: | 5145_4.patch added |
---|
comment:9 Changed 15 years ago by
5145_4 summary: CKEDITOR._scaytParams and CKEDITOR.config._scaytParams is really hidden stuff so its changed to CKEDITOR._.scaytParams
The SCAYT url corrected so SCAYT is started to work with plugin
Better documentation (added some addition params)
Changed 15 years ago by
Attachment: | 5145_5.patch added |
---|
comment:10 Changed 15 years ago by
5145_5 patch summary:
#4799 ticket is addressed with it
preserved scayt settings(language, state, options)
removed conflicts of scayt refresh method and editor's code related to selection
comment:11 Changed 15 years ago by
A lot of time has been spent to provide the 5145_3.patch patch. I've explicitly requested to use that patch as the base for new patches. So please provide a new patch based on it. 5145_5.patch will not be accepted, and it looks like it doesn't even contain the enhancements provided wit 5145_4.patch.
comment:12 Changed 15 years ago by
Sorry for our mistake while preparing 5145_5 patch. Wee have taken into account all changes in 5145_6 patch.
Changed 15 years ago by
Attachment: | 5145_6.patch added |
---|
comment:13 Changed 15 years ago by
Please divide the patch which targeting different ticket, e.g. #4688 and #4125, instead of batching them to speed up our problem solving.
Beside, by only restore dirty state on 'setData' at L144 of 5145_6.patch doesn't work because the incorrect word marking could just come later (than 'afterSetData' event), it should perform right before SCAYT's making any DOM changes.
comment:14 Changed 15 years ago by
Beside, I found CKEDITOR._.scaytParams stays an empty object after the patch, is this object really necessary? And what it been used for?
Changed 15 years ago by
Attachment: | 5145_7.patch added |
---|
comment:15 Changed 15 years ago by
comment:16 Changed 15 years ago by
Please find, that CKEDITOR._.scaytParams was deprecated, and now we will use global config variable scayt_custom_params.
I see no changes to the previous patch on this.
comment:17 Changed 15 years ago by
Changed 15 years ago by
Attachment: | 5145_8.patch added |
---|
comment:18 Changed 15 years ago by
Keywords: | Review? added; Review- removed |
---|
Align with [5234] and necessary lint and coding style fixings.
comment:19 Changed 15 years ago by
Keywords: | Review+ added; Review? removed |
---|---|
Milestone: | CKEditor 3.x → CKEditor 3.3 |
#4125 not resolved in 5101.patch