Opened 7 years ago

Closed 6 years ago

#5599 closed Bug (fixed)

Labels for special characters need to be resourced.

Reported by: damo Owned by: tobiasz.cudnik
Priority: Normal Milestone: CKEditor 3.5
Component: General Version:
Keywords: IBM Cc: satya, 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 6 years ago.
5599_2.patch (13.3 KB) - added by tobiasz.cudnik 6 years ago.
5599_3.patch (11.7 KB) - added by tobiasz.cudnik 6 years ago.
5599_4.patch (11.6 KB) - added by tobiasz.cudnik 6 years ago.
Coding standard fixes.
5599_5.patch (13.1 KB) - added by tobiasz.cudnik 6 years ago.
5599_6.patch (14.2 KB) - added by tobiasz.cudnik 6 years ago.
5599_7.patch (13.6 KB) - added by tobiasz.cudnik 6 years ago.
5599_8.patch (13.4 KB) - added by tobiasz.cudnik 6 years ago.
5599_9.patch (13.6 KB) - added by tobiasz.cudnik 6 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 7 years ago by garry.yao

  • Milestone changed from CKEditor 3.3 to CKEditor 3.4

comment:2 Changed 6 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik

Changed 6 years ago by tobiasz.cudnik

comment:3 Changed 6 years ago by tobiasz.cudnik

  • Keywords Review? added
  • Status changed from new to assigned

comment:4 Changed 6 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 6 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 6 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 6 years ago by fredck

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 6 years ago by tobiasz.cudnik

comment:8 Changed 6 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 6 years ago by tobiasz.cudnik

comment:9 Changed 6 years ago by tobiasz.cudnik

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

Changed 6 years ago by tobiasz.cudnik

Coding standard fixes.

comment:10 Changed 6 years ago by alfonsoml

  • 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 6 years ago by tobiasz.cudnik

  • Keywords Review? added; Review- removed

Ive fixed those 2 things in last patch.

comment:12 Changed 6 years ago by fredck

  • 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 6 years ago by tobiasz.cudnik

comment:13 Changed 6 years ago by tobiasz.cudnik

  • Keywords Review? added; Review- removed

comment:14 Changed 6 years ago by fredck

  • Milestone changed from CKEditor 3.4 to CKEditor 3.5

comment:15 Changed 6 years ago by fredck

  • Milestone changed from CKEditor 3.4.1 to CKEditor 3.5

comment:16 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

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

Changed 6 years ago by tobiasz.cudnik

comment:17 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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 6 years ago by garry.yao

  • Status changed from review to review_failed

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

comment:19 Changed 6 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 6 years ago by tobiasz.cudnik

comment:20 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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

comment:21 Changed 6 years ago by garry.yao

  • Status changed from review to review_passed

comment:22 Changed 6 years ago by fredck

  • Status changed from review_passed to review_failed

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

Changed 6 years ago by tobiasz.cudnik

comment:23 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

comment:24 Changed 6 years ago by james c

  • Cc jamcunni@… added

comment:25 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

Pls change config initialization to our convention.

Changed 6 years ago by tobiasz.cudnik

comment:26 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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

comment:27 Changed 6 years ago by garry.yao

  • Status changed from review to review_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 6 years ago by tobiasz.cudnik

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

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

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