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:

Description

the following tickets are addressed with this patch: #3570 #3935 #4125 #4411 #4688

Attachments (8)

5101.patch (12.6 KB) - added by WebSpellChecker.net 15 years ago.
5101_2.patch (12.7 KB) - added by WebSpellChecker.net 15 years ago.
5145_3.patch (8.9 KB) - added by Frederico Caldeira Knabben 15 years ago.
5145_4.patch (21.5 KB) - added by WebSpellChecker.net 15 years ago.
5145_5.patch (10.6 KB) - added by WebSpellChecker.net 15 years ago.
5145_6.patch (12.2 KB) - added by WebSpellChecker.net 15 years ago.
5145_7.patch (11.6 KB) - added by WebSpellChecker.net 15 years ago.
5145_8.patch (11.4 KB) - added by Garry Yao 15 years ago.

Download all attachments as: .zip

Change History (28)

Changed 15 years ago by WebSpellChecker.net

Attachment: 5101.patch added

comment:1 Changed 15 years ago by WebSpellChecker.net

#4125 not resolved in 5101.patch

Changed 15 years ago by WebSpellChecker.net

Attachment: 5101_2.patch added

comment:2 Changed 15 years ago by WebSpellChecker.net

5101_2.patch has fixed #4125

comment:3 Changed 15 years ago by Frederico Caldeira Knabben

Keywords: Review? added; review+ removed

comment:4 Changed 15 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed
Summary: all spellchekcer.net fixesall 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 Frederico Caldeira Knabben

Attachment: 5145_3.patch added

comment:5 Changed 15 years ago by Frederico Caldeira Knabben

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. Now CKEDITOR.config._scaytParams is being used, which is even worst. Please change it to CKEDITOR._.scaytParams, which is the right way to hide internal stuff.

comment:6 Changed 15 years ago by Frederico Caldeira Knabben

Summary: all spellchecker.net fixesSCAYT fixes

comment:7 in reply to:  4 Changed 15 years ago by Frederico Caldeira Knabben

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 Frederico Caldeira Knabben

Milestone: CKEditor 3.2CKEditor 3.x

Changed 15 years ago by WebSpellChecker.net

Attachment: 5145_4.patch added

comment:9 Changed 15 years ago by WebSpellChecker.net

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 WebSpellChecker.net

Attachment: 5145_5.patch added

comment:10 Changed 15 years ago by WebSpellChecker.net

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 Frederico Caldeira Knabben

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 WebSpellChecker.net

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 WebSpellChecker.net

Attachment: 5145_6.patch added

comment:13 Changed 15 years ago by Garry Yao

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 Garry Yao

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 WebSpellChecker.net

Attachment: 5145_7.patch added

comment:15 Changed 15 years ago by WebSpellChecker.net

Separated patches for #4688 (addressed in #5288 ticket) and #4125 are submitted. Please find, that CKEDITOR._.scaytParams was deprecated, and now we will use global config variable scayt_custom_params instead. ResetDirty will process every manipulation with markup.

comment:16 Changed 15 years ago by Garry Yao

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 WebSpellChecker.net

Those changes, related to scaytParams are describeD in #4411 bug and addressed with #5287 ticket

Changed 15 years ago by Garry Yao

Attachment: 5145_8.patch added

comment:18 Changed 15 years ago by Garry Yao

Keywords: Review? added; Review- removed

Align with [5234] and necessary lint and coding style fixings.

comment:19 Changed 15 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review+ added; Review? removed
Milestone: CKEditor 3.xCKEditor 3.3

comment:20 Changed 15 years ago by Garry Yao

Resolution: fixed
Status: newclosed

Fixed with [5293].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy