Ticket #5145 (closed Task: fixed)

Opened 5 years ago

Last modified 4 years ago

SCAYT fixes

Reported by: SpellChecker.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

5101.patch (12.6 KB) - added by SpellChecker.net 5 years ago.
5101_2.patch (12.7 KB) - added by SpellChecker.net 5 years ago.
5145_3.patch (8.9 KB) - added by fredck 5 years ago.
5145_4.patch (21.5 KB) - added by SpellChecker.net 5 years ago.
5145_5.patch (10.6 KB) - added by SpellChecker.net 5 years ago.
5145_6.patch (12.2 KB) - added by SpellChecker.net 5 years ago.
5145_7.patch (11.6 KB) - added by SpellChecker.net 5 years ago.
5145_8.patch (11.4 KB) - added by garry.yao 5 years ago.

Change History

Changed 5 years ago by SpellChecker.net

comment:1 Changed 5 years ago by SpellChecker.net

#4125 not resolved in 5101.patch

Changed 5 years ago by SpellChecker.net

comment:2 Changed 5 years ago by SpellChecker.net

5101_2.patch has fixed #4125

comment:3 Changed 5 years ago by fredck

  • Keywords Review? added; review+ removed

comment:4 follow-up: ↓ 7 Changed 5 years ago by alfonsoml

  • Keywords Review- added; Review? removed
  • Summary changed from all spellchekcer.net fixes to 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 5 years ago by fredck

comment:5 Changed 5 years ago by fredck

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 5 years ago by fredck

  • Summary changed from all spellchecker.net fixes to SCAYT fixes

comment:7 in reply to: ↑ 4 Changed 5 years ago by fredck

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 5 years ago by fredck

  • Milestone changed from CKEditor 3.2 to CKEditor 3.x

Changed 5 years ago by SpellChecker.net

comment:9 Changed 5 years ago by SpellChecker.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 5 years ago by SpellChecker.net

comment:10 Changed 5 years ago by SpellChecker.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 5 years ago by fredck

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 5 years ago by SpellChecker.net

Sorry for our mistake while preparing 5145_5 patch. Wee have taken into account all changes in 5145_6 patch.

Changed 5 years ago by SpellChecker.net

comment:13 Changed 5 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 5 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 5 years ago by SpellChecker.net

comment:15 Changed 5 years ago by SpellChecker.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 5 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 5 years ago by SpellChecker.net

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

Changed 5 years ago by garry.yao

comment:18 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

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

comment:19 Changed 5 years ago by alfonsoml

  • Keywords Review+ added; Review? removed
  • Milestone changed from CKEditor 3.x to CKEditor 3.3

comment:20 Changed 4 years ago by garry.yao

  • Status changed from new to closed
  • Resolution set to fixed

Fixed with [5293].

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy