Opened 7 years ago

Closed 7 years ago

#5599 closed Bug (fixed)

Labels for special characters need to be resourced.

Reported by: Damian Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.5
Component: General Version:
Keywords: IBM Cc: Satya Minnekanti, joek, jamcunni@…

Description

The labels in the special characters dialog are hard coded with English values. These labels should be resourced to enable translation.

Attachments (9)

5599_1.patch (12.1 KB) - added by Tobiasz Cudnik 7 years ago.
5599_2.patch (13.3 KB) - added by Tobiasz Cudnik 7 years ago.
5599_3.patch (11.7 KB) - added by Tobiasz Cudnik 7 years ago.
5599_4.patch (11.6 KB) - added by Tobiasz Cudnik 7 years ago.
Coding standard fixes.
5599_5.patch (13.1 KB) - added by Tobiasz Cudnik 7 years ago.
5599_6.patch (14.2 KB) - added by Tobiasz Cudnik 7 years ago.
5599_7.patch (13.6 KB) - added by Tobiasz Cudnik 7 years ago.
5599_8.patch (13.4 KB) - added by Tobiasz Cudnik 7 years ago.
5599_9.patch (13.6 KB) - added by Tobiasz Cudnik 7 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 7 years ago by Garry Yao

Milestone: CKEditor 3.3CKEditor 3.4

comment:2 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5599_1.patch added

comment:3 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added
Status: newassigned

comment:4 Changed 7 years ago by Garry Yao

Keywords: Review- added; Review? removed

It's great, but how about using directly Numeric/Character entities as lang entry?

comment:5 Changed 7 years ago by Tobiasz Cudnik

I don't think localization files are the correct place for keeping array of characters shown by Special Chars dialog. I'm not saying this won't have benefits for localization, although for such things config file seems quite more logical.

We can place chars in config and take labels from lang files if they exists, but in the same time give ability to define custom labels in the config, for people who needs custom special chars.

Saying this in the code, it would looks like this (config.js):

CKEDITOR.config.specialChars = [
  '”', '–', /* ... */,
  [ '&custom;', 'Custom Char Label' ]
];

We can also provide a way of adding new special chars without overriding ones defined by default using the same way as plugins list does today.

comment:6 Changed 7 years ago by Garry Yao

I'm pretty agree with the configuration approach, the problem is the bytes of keeping translation in the config, plugin specific lang files could be used for it.

comment:7 Changed 7 years ago by Frederico Caldeira Knabben

I liked the proposed patch approach, partially. Let's try to make things logic, smaller and flexible.

  • Localization stuff should go into language files.
  • Let's take the opportunity to reduce the loading size of the editor, having a plugin language file, loading it on demand when opening the dialog for the first time, similarly to the a11y plugin.
  • It's still nice to have the option to set the label text into the configurations directly. The patch approach is ok for it.
  • To reduce further the size, and to make the configuration easier, there is no need to have ["€", lang.euro] in the settings. We can have some simple label resolve rules for it:
    • If the entry is an Array, take the second element of it (if available) as the label.
    • Otherwise, check whether there is a language entry for the "exact" special character entry. If the entry is an entity, strip the "&" and ";".
    • Finally, if nothing is found, use the character itself as the label.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5599_2.patch added

comment:8 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

Second patch implementes Fred's suggestions, with exception of pre-pending "sign_" prefix for numerical-only chars, as object keys can't be numeric in JS without quoting, which may be missing in out langtool.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5599_3.patch added

comment:9 Changed 7 years ago by Tobiasz Cudnik

Previous patch was missing external language file for labels. Third one fixes that.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5599_4.patch added

Coding standard fixes.

comment:10 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

Instead of

chars[ chars.length ] = extraChars[ i ]

You can use

chars.push = extraChars[ i ]

But in this case I think that it should be possible to use Array.concat (untested):

chars = editor.config.specialChars.concat( editor.config.extraSpecialChars );

The plugins shouldn't be available in source mode:

 modes : { wysiwyg:1, source:1 },

comment:11 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

Ive fixed those 2 things in last patch.

comment:12 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

It looks like the last patch has been created differently. I'm not able to apply it. Can you please recreate it?

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5599_5.patch added

comment:13 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

comment:14 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4CKEditor 3.5

comment:15 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4.1CKEditor 3.5

comment:16 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

There's entity missed between '‛' and 'ŷ'

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5599_6.patch added

comment:17 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

I've added the missing sbquo entity, fixed labels for uppercased entities and abandoned "sign_" prefix, as it should not be a problem for our releaser.

comment:18 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

In such way how do we distinguish the lang entry between entities in decimal and hexadecimal?

comment:19 Changed 7 years ago by Garry Yao

Ok, forgive my last comment, besides, I think the 'extraSpecialChars ' could be removed and just get merged into the config.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5599_7.patch added

comment:20 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

Now we're using only one config entry, i've also added an example how to extend it.

comment:21 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:22 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_passedreview_failed

Please move the configuration entry from the core files to the plugin file.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5599_8.patch added

comment:23 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

comment:24 Changed 7 years ago by James

Cc: jamcunni@… added

comment:25 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

Pls change config initialization to our convention.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5599_9.patch added

comment:26 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

I've modified the config in a way the Autogrow plugin have, although it's not KISS at all.

comment:27 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

Last patch fails the following case:

config.specialChars = config.specialChars.concat( [ '"', [ '’', 'Custom label' ] ] );

So let's back to 5599_8.patch which is kept the same as "smiley" plugin.

comment:28 Changed 7 years ago by Tobiasz Cudnik

Resolution: fixed
Status: review_passedclosed

Fixed with [6088], using 5599_8.patch.

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